From 779a39291469c276dff991995e558df5796c8d84 Mon Sep 17 00:00:00 2001 From: Scott Wegner Date: Thu, 31 Mar 2016 15:04:26 -0700 Subject: [PATCH 1/4] Refactor IsmFormat value classes to use AutoValue --- sdks/java/core/pom.xml | 6 + .../beam/sdk/runners/worker/IsmFormat.java | 244 +++++------------- 2 files changed, 67 insertions(+), 183 deletions(-) diff --git a/sdks/java/core/pom.xml b/sdks/java/core/pom.xml index 859e07a5dac7..c634e9c61d3d 100644 --- a/sdks/java/core/pom.xml +++ b/sdks/java/core/pom.xml @@ -722,6 +722,12 @@ 1.0-rc2 true + + com.google.auto.value + auto-value + 1.1 + provided + diff --git a/sdks/java/core/src/main/java/org/apache/beam/sdk/runners/worker/IsmFormat.java b/sdks/java/core/src/main/java/org/apache/beam/sdk/runners/worker/IsmFormat.java index 63ac59a26546..1cdd0b607ba9 100644 --- a/sdks/java/core/src/main/java/org/apache/beam/sdk/runners/worker/IsmFormat.java +++ b/sdks/java/core/src/main/java/org/apache/beam/sdk/runners/worker/IsmFormat.java @@ -38,6 +38,7 @@ import org.apache.beam.sdk.util.VarInt; import org.apache.beam.sdk.values.PCollection; +import com.google.auto.value.AutoValue; import com.google.common.base.MoreObjects; import com.google.common.base.MoreObjects.ToStringHelper; import com.google.common.base.Objects; @@ -121,44 +122,38 @@ public class IsmFormat { * keys are used to create a shard id via hashing. See {@link IsmRecordCoder#hash(List)} for * further details. */ - public static class IsmRecord { + @AutoValue + public abstract static class IsmRecord { + abstract List keyComponents(); + @Nullable abstract V value(); + @Nullable abstract byte[] metadata(); + + IsmRecord() {} + /** Returns an IsmRecord with the specified key components and value. */ public static IsmRecord of(List keyComponents, V value) { - checkNotNull(keyComponents); checkArgument(!keyComponents.isEmpty(), "Expected non-empty list of key components."); checkArgument(!isMetadataKey(keyComponents), "Expected key components to not contain metadata key."); - return new IsmRecord<>(keyComponents, value, null); + return new AutoValue_IsmFormat_IsmRecord(keyComponents, value, null); } public static IsmRecord meta(List keyComponents, byte[] metadata) { - checkNotNull(keyComponents); checkNotNull(metadata); checkArgument(!keyComponents.isEmpty(), "Expected non-empty list of key components."); checkArgument(isMetadataKey(keyComponents), "Expected key components to contain metadata key."); - return new IsmRecord(keyComponents, null, metadata); - } - - private final List keyComponents; - @Nullable - private final V value; - @Nullable - private final byte[] metadata; - private IsmRecord(List keyComponents, V value, byte[] metadata) { - this.keyComponents = keyComponents; - this.value = value; - this.metadata = metadata; + return new AutoValue_IsmFormat_IsmRecord(keyComponents, null, metadata); } /** Returns the list of key components. */ public List getKeyComponents() { - return keyComponents; + return keyComponents(); } /** Returns the key component at the specified index. */ public Object getKeyComponent(int index) { - return keyComponents.get(index); + return keyComponents().get(index); } /** @@ -166,9 +161,9 @@ public Object getKeyComponent(int index) { * value record. */ public V getValue() { - checkState(!isMetadataKey(keyComponents), + checkState(!isMetadataKey(keyComponents()), "This is a metadata record and not a value record."); - return value; + return value(); } /** @@ -176,37 +171,9 @@ public V getValue() { * metadata record. */ public byte[] getMetadata() { - checkState(isMetadataKey(keyComponents), + checkState(isMetadataKey(keyComponents()), "This is a value record and not a metadata record."); - return metadata; - } - - @Override - public boolean equals(Object obj) { - if (!(obj instanceof IsmRecord)) { - return false; - } - IsmRecord other = (IsmRecord) obj; - return Objects.equal(keyComponents, other.keyComponents) - && Objects.equal(value, other.value) - && Arrays.equals(metadata, other.metadata); - } - - @Override - public int hashCode() { - return Objects.hashCode(keyComponents, value, Arrays.hashCode(metadata)); - } - - @Override - public String toString() { - ToStringHelper builder = MoreObjects.toStringHelper(IsmRecord.class) - .add("keyComponents", keyComponents); - if (isMetadataKey(keyComponents)) { - builder.add("metadata", metadata); - } else { - builder.add("value", value); - } - return builder.toString(); + return metadata(); } } @@ -589,14 +556,17 @@ public void verifyDeterministic() throws NonDeterministicException { * A shard descriptor containing shard id, the data block offset, and the index offset for the * given shard. */ - public static class IsmShard { - private final int id; - private final long blockOffset; - private final long indexOffset; + @AutoValue + public abstract static class IsmShard { + abstract int id(); + abstract long blockOffset(); + abstract long indexOffset(); + + IsmShard() {} /** Returns an IsmShard with the given id, block offset and no index offset. */ public static IsmShard of(int id, long blockOffset) { - IsmShard ismShard = new IsmShard(id, blockOffset, -1); + IsmShard ismShard = new AutoValue_IsmFormat_IsmShard(id, blockOffset, -1); checkState(id >= 0, "%s attempting to be written with negative shard id.", ismShard); @@ -608,7 +578,7 @@ public static IsmShard of(int id, long blockOffset) { /** Returns an IsmShard with the given id, block offset, and index offset. */ public static IsmShard of(int id, long blockOffset, long indexOffset) { - IsmShard ismShard = new IsmShard(id, blockOffset, indexOffset); + IsmShard ismShard = new AutoValue_IsmFormat_IsmShard(id, blockOffset, indexOffset); checkState(id >= 0, "%s attempting to be written with negative shard id.", ismShard); @@ -621,20 +591,14 @@ public static IsmShard of(int id, long blockOffset, long indexOffset) { return ismShard; } - private IsmShard(int id, long blockOffset, long indexOffset) { - this.id = id; - this.blockOffset = blockOffset; - this.indexOffset = indexOffset; - } - /** Return the shard id. */ public int getId() { - return id; + return id(); } /** Return the absolute position within the Ism file where the data block begins. */ public long getBlockOffset() { - return blockOffset; + return blockOffset(); } /** @@ -642,39 +606,14 @@ public long getBlockOffset() { * Throws {@link IllegalStateException} if the index offset was never specified. */ public long getIndexOffset() { - checkState(indexOffset >= 0, + checkState(indexOffset() >= 0, "Unable to fetch index offset because it was never specified."); - return indexOffset; + return indexOffset(); } /** Returns a new IsmShard like this one with the specified index offset. */ public IsmShard withIndexOffset(long indexOffset) { - return of(id, blockOffset, indexOffset); - } - - @Override - public String toString() { - return MoreObjects.toStringHelper(IsmShard.class) - .add("id", id) - .add("blockOffset", blockOffset) - .add("indexOffset", indexOffset) - .toString(); - } - - @Override - public boolean equals(Object obj) { - if (!(obj instanceof IsmShard)) { - return false; - } - IsmShard other = (IsmShard) obj; - return Objects.equal(id, other.id) - && Objects.equal(blockOffset, other.blockOffset) - && Objects.equal(indexOffset, other.indexOffset); - } - - @Override - public int hashCode() { - return Objects.hashCode(id, blockOffset, indexOffset); + return of(id(), blockOffset(), indexOffset); } } @@ -746,47 +685,15 @@ public boolean consistentWithEquals() { *
  • number of unshared key bytes (variable length integer coding)
    • * */ - public static class KeyPrefix { - private final int sharedKeySize; - private final int unsharedKeySize; - - public KeyPrefix(int sharedBytes, int unsharedBytes) { - this.sharedKeySize = sharedBytes; - this.unsharedKeySize = unsharedBytes; - } + @AutoValue + public abstract static class KeyPrefix { + public abstract int getSharedKeySize(); + public abstract int getUnsharedKeySize(); - public int getSharedKeySize() { - return sharedKeySize; - } - - public int getUnsharedKeySize() { - return unsharedKeySize; - } + KeyPrefix() {} - @Override - public int hashCode() { - return Objects.hashCode(sharedKeySize, unsharedKeySize); - } - - @Override - public boolean equals(Object other) { - if (other == this) { - return true; - } - if (!(other instanceof KeyPrefix)) { - return false; - } - KeyPrefix keyPrefix = (KeyPrefix) other; - return sharedKeySize == keyPrefix.sharedKeySize - && unsharedKeySize == keyPrefix.unsharedKeySize; - } - - @Override - public String toString() { - return MoreObjects.toStringHelper(this) - .add("sharedKeySize", sharedKeySize) - .add("unsharedKeySize", unsharedKeySize) - .toString(); + public static KeyPrefix of(int sharedKeySize, int unsharedKeySize) { + return new AutoValue_IsmFormat_KeyPrefix(sharedKeySize, unsharedKeySize); } } @@ -802,14 +709,14 @@ public static KeyPrefixCoder of() { @Override public void encode(KeyPrefix value, OutputStream outStream, Coder.Context context) throws CoderException, IOException { - VarInt.encode(value.sharedKeySize, outStream); - VarInt.encode(value.unsharedKeySize, outStream); + VarInt.encode(value.getSharedKeySize(), outStream); + VarInt.encode(value.getUnsharedKeySize(), outStream); } @Override public KeyPrefix decode(InputStream inStream, Coder.Context context) throws CoderException, IOException { - return new KeyPrefix(VarInt.decodeInt(inStream), VarInt.decodeInt(inStream)); + return KeyPrefix.of(VarInt.decodeInt(inStream), VarInt.decodeInt(inStream)); } @Override @@ -826,7 +733,8 @@ public boolean isRegisterByteSizeObserverCheap(KeyPrefix value, Coder.Context co public long getEncodedElementByteSize(KeyPrefix value, Coder.Context context) throws Exception { Preconditions.checkNotNull(value); - return VarInt.getLength(value.sharedKeySize) + VarInt.getLength(value.unsharedKeySize); + return VarInt.getLength(value.getSharedKeySize()) + + VarInt.getLength(value.getUnsharedKeySize()); } } @@ -842,59 +750,29 @@ public long getEncodedElementByteSize(KeyPrefix value, Coder.Context context) *
  • 0x01 (version key as a single byte)
    • * */ - public static class Footer { - public static final int LONG_BYTES = 8; - public static final int FIXED_LENGTH = 3 * LONG_BYTES + 1; - public static final byte VERSION = 2; - - private final long indexPosition; - private final long bloomFilterPosition; - private final long numberOfKeys; - - public Footer(long indexPosition, long bloomFilterPosition, long numberOfKeys) { - this.indexPosition = indexPosition; - this.bloomFilterPosition = bloomFilterPosition; - this.numberOfKeys = numberOfKeys; - } - - public long getIndexPosition() { - return indexPosition; - } - - public long getBloomFilterPosition() { - return bloomFilterPosition; - } + @AutoValue + public abstract static class Footer { + static final int LONG_BYTES = 8; + static final int FIXED_LENGTH = 3 * LONG_BYTES + 1; + static final byte VERSION = 2; - public long getNumberOfKeys() { - return numberOfKeys; - } + public abstract long getIndexPosition(); + public abstract long getBloomFilterPosition(); + public abstract long getNumberOfKeys(); - @Override - public boolean equals(Object other) { - if (other == this) { - return true; - } - if (!(other instanceof Footer)) { - return false; - } - Footer footer = (Footer) other; - return indexPosition == footer.indexPosition - && bloomFilterPosition == footer.bloomFilterPosition - && numberOfKeys == footer.numberOfKeys; - } + Footer() {} - @Override - public int hashCode() { - return Objects.hashCode(indexPosition, bloomFilterPosition, numberOfKeys); + public static Footer of(long indexPosition, long bloomFilterPosition, long numberOfKeys) { + return new AutoValue_IsmFormat_Footer(indexPosition, bloomFilterPosition, numberOfKeys); } @Override public String toString() { return MoreObjects.toStringHelper(this) .add("version", Footer.VERSION) - .add("indexPosition", indexPosition) - .add("bloomFilterPosition", bloomFilterPosition) - .add("numberOfKeys", numberOfKeys) + .add("indexPosition", getIndexPosition()) + .add("bloomFilterPosition", getBloomFilterPosition()) + .add("numberOfKeys", getNumberOfKeys()) .toString(); } } @@ -912,9 +790,9 @@ public static FooterCoder of() { public void encode(Footer value, OutputStream outStream, Coder.Context context) throws CoderException, IOException { DataOutputStream dataOut = new DataOutputStream(outStream); - dataOut.writeLong(value.indexPosition); - dataOut.writeLong(value.bloomFilterPosition); - dataOut.writeLong(value.numberOfKeys); + dataOut.writeLong(value.getIndexPosition()); + dataOut.writeLong(value.getBloomFilterPosition()); + dataOut.writeLong(value.getNumberOfKeys()); dataOut.write(Footer.VERSION); } @@ -922,7 +800,7 @@ public void encode(Footer value, OutputStream outStream, Coder.Context context) public Footer decode(InputStream inStream, Coder.Context context) throws CoderException, IOException { DataInputStream dataIn = new DataInputStream(inStream); - Footer footer = new Footer(dataIn.readLong(), dataIn.readLong(), dataIn.readLong()); + Footer footer = Footer.of(dataIn.readLong(), dataIn.readLong(), dataIn.readLong()); int version = dataIn.read(); if (version != Footer.VERSION) { throw new IOException("Unknown version " + version + ". " From 069266a5f20ed66fc2ecbbc3b0a688a3b3e706d2 Mon Sep 17 00:00:00 2001 From: Scott Wegner Date: Thu, 31 Mar 2016 15:58:00 -0700 Subject: [PATCH 2/4] Fix checkstyle errors --- .../java/org/apache/beam/sdk/runners/worker/IsmFormat.java | 3 --- 1 file changed, 3 deletions(-) diff --git a/sdks/java/core/src/main/java/org/apache/beam/sdk/runners/worker/IsmFormat.java b/sdks/java/core/src/main/java/org/apache/beam/sdk/runners/worker/IsmFormat.java index 1cdd0b607ba9..f11f67f36d25 100644 --- a/sdks/java/core/src/main/java/org/apache/beam/sdk/runners/worker/IsmFormat.java +++ b/sdks/java/core/src/main/java/org/apache/beam/sdk/runners/worker/IsmFormat.java @@ -40,8 +40,6 @@ import com.google.auto.value.AutoValue; import com.google.common.base.MoreObjects; -import com.google.common.base.MoreObjects.ToStringHelper; -import com.google.common.base.Objects; import com.google.common.base.Preconditions; import com.google.common.collect.ImmutableList; import com.google.common.hash.HashFunction; @@ -56,7 +54,6 @@ import java.io.InputStream; import java.io.OutputStream; import java.util.ArrayList; -import java.util.Arrays; import java.util.List; import javax.annotation.Nullable; From 649b00600eb53a404a33b24165a8df8425204f8f Mon Sep 17 00:00:00 2001 From: Scott Wegner Date: Wed, 13 Apr 2016 09:24:43 -0700 Subject: [PATCH 3/4] fixup: cleanup based on review feedback --- .../beam/sdk/runners/worker/IsmFormat.java | 21 ++++--------------- 1 file changed, 4 insertions(+), 17 deletions(-) diff --git a/sdks/java/core/src/main/java/org/apache/beam/sdk/runners/worker/IsmFormat.java b/sdks/java/core/src/main/java/org/apache/beam/sdk/runners/worker/IsmFormat.java index f11f67f36d25..8b23e0a0f795 100644 --- a/sdks/java/core/src/main/java/org/apache/beam/sdk/runners/worker/IsmFormat.java +++ b/sdks/java/core/src/main/java/org/apache/beam/sdk/runners/worker/IsmFormat.java @@ -39,7 +39,6 @@ import org.apache.beam.sdk.values.PCollection; import com.google.auto.value.AutoValue; -import com.google.common.base.MoreObjects; import com.google.common.base.Preconditions; import com.google.common.collect.ImmutableList; import com.google.common.hash.HashFunction; @@ -125,7 +124,7 @@ public abstract static class IsmRecord { @Nullable abstract V value(); @Nullable abstract byte[] metadata(); - IsmRecord() {} + IsmRecord() {} // Prevent public constructor /** Returns an IsmRecord with the specified key components and value. */ public static IsmRecord of(List keyComponents, V value) { @@ -687,8 +686,6 @@ public abstract static class KeyPrefix { public abstract int getSharedKeySize(); public abstract int getUnsharedKeySize(); - KeyPrefix() {} - public static KeyPrefix of(int sharedKeySize, int unsharedKeySize) { return new AutoValue_IsmFormat_KeyPrefix(sharedKeySize, unsharedKeySize); } @@ -753,24 +750,14 @@ public abstract static class Footer { static final int FIXED_LENGTH = 3 * LONG_BYTES + 1; static final byte VERSION = 2; + public abstract byte getVersion(); public abstract long getIndexPosition(); public abstract long getBloomFilterPosition(); public abstract long getNumberOfKeys(); - Footer() {} - public static Footer of(long indexPosition, long bloomFilterPosition, long numberOfKeys) { - return new AutoValue_IsmFormat_Footer(indexPosition, bloomFilterPosition, numberOfKeys); - } - - @Override - public String toString() { - return MoreObjects.toStringHelper(this) - .add("version", Footer.VERSION) - .add("indexPosition", getIndexPosition()) - .add("bloomFilterPosition", getBloomFilterPosition()) - .add("numberOfKeys", getNumberOfKeys()) - .toString(); + return new AutoValue_IsmFormat_Footer( + VERSION, indexPosition, bloomFilterPosition, numberOfKeys); } } From 387ba13db24eec57020d499ffc7ae4a4b8052be0 Mon Sep 17 00:00:00 2001 From: Scott Wegner Date: Wed, 20 Apr 2016 13:45:32 -0700 Subject: [PATCH 4/4] Exclude generated AutoValue source from code coverage tooling --- pom.xml | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/pom.xml b/pom.xml index 7962ed9c22af..6753987ded89 100644 --- a/pom.xml +++ b/pom.xml @@ -276,6 +276,11 @@ + + + **/AutoValue_*.java + +