[BEAM-813] support metadata in Avro sink #1177

nevillelyh · 2016-10-24T23:04:00Z

No description provided.

lukecwik · 2016-10-25T17:59:01Z

lukecwik · 2016-10-25T17:58:41Z

sdks/java/core/src/main/java/org/apache/beam/sdk/io/AvroIO.java

@@ -455,6 +458,13 @@ private Read() {}
    }

    /**
+     * Returns a {@link PTransform} that writes Avro file(s) with specified metadata.
+     */
+    public static Bound<GenericRecord> withMetadata(Map<String, String> metadata) {


We should also allow byte[] and long by making the value type of the map an Object.

In the Bound constructor we can validate that the map that the user passed in only contain these types. We should explicitly say in the javadoc that only these three types are supported.

Changed it to Map<String, Object> and added Javadoc.

lukecwik · 2016-10-25T18:07:18Z

sdks/java/core/src/test/java/org/apache/beam/sdk/io/AvroIOTest.java

+
+    DataFileStream dataFileStream = new DataFileStream(new FileInputStream(outputFile),
+        new GenericDatumReader());
+    assertTrue(dataFileStream.getMetaString("key").equals("value"));


Please use assertEquals because it will give a better error message on failure.
assertEquals("value", dataFileStream.getMetaString("key"));

lukecwik · 2016-10-25T18:10:12Z

sdks/java/core/src/main/java/org/apache/beam/sdk/io/AvroIO.java

+        for (Map.Entry<String, String> entry : metadata.entrySet()) {
+          builder.add(DisplayData.item(entry.getKey(), entry.getValue()).withLabel("Metadata"));
+        }
+


nit: remove this extra white space.

lukecwik · 2016-10-25T18:10:55Z

sdks/java/core/src/main/java/org/apache/beam/sdk/io/AvroIO.java

+
+      /**
+       * Returns a new {@link PTransform} that's like this one but
+       * that writes to Avro file(s) with specified metadata.


with specified metadata. -> with the specified metadata.

lukecwik · 2016-10-25T18:11:15Z

sdks/java/core/src/main/java/org/apache/beam/sdk/io/AvroIO.java

@@ -455,6 +458,13 @@ private Read() {}
    }

    /**
+     * Returns a {@link PTransform} that writes Avro file(s) with specified metadata.


with specified metadata. -> with the specified metadata.

lukecwik · 2016-10-25T18:12:50Z

sdks/java/core/src/main/java/org/apache/beam/sdk/io/AvroIO.java

@@ -519,6 +532,7 @@ private Read() {}
        this.schema = schema;
        this.validate = validate;
        this.codec = codec;
+        this.metadata = ImmutableMap.copyOf(metadata);


It seems like we are duplicating the map copy multiple times. It makes sense that its only done within the withMetadata call.

Good catch. Done.

lukecwik · 2016-10-25T18:27:08Z

sdks/java/core/src/main/java/org/apache/beam/sdk/io/AvroIO.java

@@ -779,6 +823,10 @@ public void populateDisplayData(DisplayData.Builder builder) {
            .addIfNotDefault(DisplayData.item("codec", codec.toString())
                .withLabel("Avro Compression Codec"),
                DEFAULT_CODEC.toString());
+        for (Map.Entry<String, String> entry : metadata.entrySet()) {
+          builder.add(DisplayData.item(entry.getKey(), entry.getValue()).withLabel("Metadata"));


This will make all metadata have the same label called "Metadata" irrespective of the key which is not what I think you want.

I think you want to do:

builder.include("Metadata", new Metadata());

and add the following private class inside Bound:

private class Metadata implements HasDisplayData { void populateDisplayData(DisplayData.Builder builder) { for (Map... entry : ...) { String valueString = // convert entry.getValue() to string builder.add(DisplayData.item(entry.getKey(), entryString); } } }

This allows for all the metadata to be grouped under a single namespace component and be logically related.
For the binary data, it won't be human readable and doesn't really fit as information that would be consumed, so I suggest that either:

skip over specifying binary fields

add them and make the value a word like "..." to mark that its specified

add them and make the value the hex or base64 representation of a fixed prefix of the bytes followed by "..." if the bytes is long otherwise the entire hex/base64 representation.

add them and make the value a checksum of the bytes

@nevillelyh @dhalperi @swegner What kinds of binary data do people add to Avro files and is its presence in the DisplayData genuinely useful?
@nevillelyh @dhalperi @swegner any specific preference for the above?

+1. You may also find it useful to use DisplayData.inferType(..) to easily pull out String/Long values and special-case others:

for (Map... entry : ...) { DisplayData.Type type = DisplayData.inferFrom(entry.getValue()); if (type != null) { builder.add(DisplayData.item(entry.getKey(), type, entry.getValue()); } else { // handle binary data } }

Unless we know there is binary metadata which would be valuable to display, I recommend going the simple route and substituting a marker word ("...") or using the Object.toString().

Object.toString on a byte[] will not provide any meaningfully useful information since it will just print out the address of the byte[] in memory.

We're only interested in String metadata but base64 with optional "..." sounds good to me.

nevillelyh · 2016-10-26T19:16:17Z

@lukecwik addressed your comments. LMK what you think.

lukecwik

Minor comments, and then LGTM

Note in the future, please don't squash your additional commits into existing reviewed commits. It makes it much easier if a reviewer can look at the diff between the last reviewed version. If you prefix your commit message with fixup! or squash!, a committer will use git to clean-up the commit history. Contribution guide needs some updating to add the fixup!/squash! stuff to it but this is the practice that has been followed. http://beam.incubator.apache.org/contribute/contribution-guide/#code-review-and-revision

lukecwik · 2016-10-26T20:25:11Z

sdks/java/core/src/main/java/org/apache/beam/sdk/io/AvroIO.java

+          }
+        }
+        checkArgument(
+            badKeys.isEmpty(), "Metadata value must be String, Long, or byte[]. {}", badKeys);


Metadata value must be String, Long, or byte[]. {} -> Metadata value type must be one of String, Long, or byte[]. Found {}

lukecwik · 2016-10-26T20:31:10Z

sdks/java/core/src/main/java/org/apache/beam/sdk/io/AvroIO.java

+              builder.add(DisplayData.item(entry.getKey(), type, entry.getValue()));
+            } else {
+              String base64 = BaseEncoding.base64().encode((byte[]) entry.getValue());
+              String repr = base64.length() <= 40 ? base64 : base64.substring(0, 40) + "...";


nice, extract the 40 to a constant, also make a comment for it that it should be a multiple of 4 to not get a partial encoded byte.

lukecwik · 2016-10-26T20:32:53Z

sdks/java/core/src/main/java/org/apache/beam/sdk/io/AvroIO.java

+            dataFileWriter.setMeta(entry.getKey(), (Long) v);
+          } else if (v instanceof byte[]) {
+            dataFileWriter.setMeta(entry.getKey(), (byte[]) v);
+          }


Even though this case is guarded against by the Bound class, its best to have an else block that throws an IllegalStateException, This will prevent code rot if someone only updates the one list and not the other instead of silently ignoring it.

lukecwik · 2016-10-26T21:26:04Z

LGTM

Thanks for the contribution.

lukecwik · 2016-10-27T00:00:33Z

This was merged here but failed to close the PR: eba099f

Please close this PR.

nevillelyh mentioned this pull request Oct 24, 2016

Add support for introspecting avro-wrapped protobufs spotify/scio#265

Closed

lukecwik requested changes Oct 25, 2016

View reviewed changes

[BEAM-813] support metadata in Avro sink

d351ded

lukecwik requested changes Oct 26, 2016

View reviewed changes

fixup review comments

5ec2965

nevillelyh mentioned this pull request Oct 26, 2016

save protobuf generic schema as Avro metadata #265 spotify/scio#310

Merged

nevillelyh closed this Oct 27, 2016

nevillelyh deleted the neville/avro-metadata branch October 31, 2016 08:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[BEAM-813] support metadata in Avro sink #1177

[BEAM-813] support metadata in Avro sink #1177

nevillelyh commented Oct 24, 2016

lukecwik commented Oct 25, 2016

lukecwik Oct 25, 2016

nevillelyh Oct 26, 2016

lukecwik Oct 25, 2016

nevillelyh Oct 25, 2016

lukecwik Oct 25, 2016

nevillelyh Oct 25, 2016

lukecwik Oct 25, 2016

nevillelyh Oct 26, 2016

lukecwik Oct 25, 2016

nevillelyh Oct 25, 2016

lukecwik Oct 25, 2016

nevillelyh Oct 25, 2016

lukecwik Oct 25, 2016

swegner Oct 25, 2016

lukecwik Oct 25, 2016

nevillelyh Oct 25, 2016

nevillelyh Oct 26, 2016

nevillelyh commented Oct 26, 2016

lukecwik left a comment •

edited

Loading

lukecwik Oct 26, 2016

nevillelyh Oct 26, 2016

lukecwik Oct 26, 2016

nevillelyh Oct 26, 2016

lukecwik Oct 26, 2016

nevillelyh Oct 26, 2016

lukecwik commented Oct 26, 2016

lukecwik commented Oct 27, 2016

[BEAM-813] support metadata in Avro sink #1177

[BEAM-813] support metadata in Avro sink #1177

Conversation

nevillelyh commented Oct 24, 2016

lukecwik commented Oct 25, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nevillelyh commented Oct 26, 2016

lukecwik left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lukecwik commented Oct 26, 2016

lukecwik commented Oct 27, 2016

lukecwik left a comment •

edited

Loading