AVRO-1340: Added Enum Defaults and unit tests. #298

bellemare · 2018-03-11T18:54:41Z

Added Defaults to Enums.
Enum Defaults are only applied to values which don't match the Reader Enum symbols.
Enum Defaults do not override any Field Default behaviour. Field Defaults are used when a reader reads a datum that does not have the Field defined (as is current behaviour).

To see an example table of this behaviour, see Final Default & Fallback tab in:
https://docs.google.com/spreadsheets/d/1YkTBnHCBPGz2t0gQI3OioKS_xP9h9lq_a-gS2jSGFAc/edit?usp=sharing

cutting

This looks great. I like the simple, minimal approach taken. I've identified a few improvements, namely builder and IDL integration. These could be done as followup issues, but it might be better to get them in from the start. Thanks!

cutting · 2018-03-13T19:30:14Z

lang/java/avro/src/main/java/org/apache/avro/Schema.java

        && symbols.equals(that.symbols)
        && props.equals(that.props);
    }
+    public String getEnumDefault() { return enumDefault; }


needs @OverRide?

cutting · 2018-03-13T19:58:09Z

lang/java/avro/src/main/java/org/apache/avro/Schema.java

      if (aliases.containsKey(name))
        result = Schema.createEnum(aliases.get(name).full, s.getDoc(), null,
-                                   s.getEnumSymbols());
+                                   s.getEnumSymbols(), s.getEnumDefault());


I scanned for other places where Schema.createEnum is called to see which need updates.
ResolvingVisitor.java also needs this same change. This is used to support IDL. We should also add support to IDL for enum defaults.
SchemaBuilder.java should be changed to permit specifying an enum default.

Okay yes, I can add that to this PR.

… at the idl.jj for enum default but tests not passing for simple.avdl

cutting · 2018-04-16T23:41:48Z

Here's a fix for the IDL problem. The grammar change seems to cause the parser to lookahead farther, so the last documentation comment that's been seen is too far along. I changed it to grab the doc string once it's seen the enum keyword and that seems to fix it.

+++ b/lang/java/compiler/src/main/javacc/org/apache/avro/compiler/idl/idl.jj
@@ -1102,12 +1102,12 @@ Schema EnumDeclaration():
   String defaultSymbol = null;
 }
 {
-  "enum"
+  "enum" {   String doc = getDoc(); }
   name = Identifier()
   symbols = EnumBody()
       [ <EQUALS> defaultSymbol=Identifier() <SEMICOLON>]
   {
-    Schema s = Schema.createEnum(name, getDoc(), this.namespace, symbols,
+    Schema s = Schema.createEnum(name, doc, this.namespace, symbols,
                                  defaultSymbol);
     names.put(s.getFullName(), s);
     return s;

Patch otherwise looks good. We should also test the IDL change. Maybe change simple.avdl to specify an enum default, then update simple.avpr to contain the new output? (You can use 'mvn package -dskipTests' to create a tools.jar so 'java -jar tools.jar idl ...' can be used to create the new compiler test output file.)

…ing. Added test case to simple.avdl and simple.avpr

bellemare · 2018-04-17T15:50:44Z

@cutting - Thanks Doug. I applied the patch, added a test and built the tooling to validate that it works. Is there anything else you forsee being required? Also, do we need additional eyes on this? I recall that someone mentioned in the JIRA thread about the "@default" annotation - I don't have any idea how it would interact with this.

cutting · 2018-04-18T17:23:47Z

We're almost there. I just looked at the builder integration, and think it would be better if we instead added defaultSymbol(String) method to EnumBuilder. We also need javadoc for this new method, and should update the EnumBuilder example in the SchemaBuilder class javadoc to specify a default symbol.

Also, we need to update doc/src/content/xdocs/idl.xml to describe enum defaults. This can be in the "Defining and Enumeration" section. We can update the example to include a default.

Additional eyes are not required. Folks have the opportunity to follow here & object. I'll commit this once we're happy with it. We could add an @default annotation now, or commit this now and add that later, I don't have a strong feeling.

BirdHowl · 2018-04-18T18:04:05Z

We could add an @default annotation now, or commit this now and add that later, I don't have a strong feeling.

I think there's some misunderstanding here - @default already works, since IDL lets any JSON property be set through annotations (although that behavior isn't well-documented). Anything that doesn't work as an annotation doesn't work because of being a reserved property. default, however is a reserved property only for fields.

If I take this a protocol file named test.avdl:

protocol Test {
  @default("UNKNOWN")
  enum Status {
      SUCCESS,
      FAILURE,
      UNKNOWN
  }
}

And then run avro-tools.jar idl test.avdl (tested on both this branch and master), I get this output:

{
  "protocol" : "Test",
  "namespace" : null,
  "types" : [ {
    "type" : "enum",
    "name" : "Status",
    "symbols" : [ "SUCCESS", "FAILURE", "UNKNOWN" ],
    "default" : "UNKNOWN"
  } ],
  "messages" : { }
}

In my JIRA post, I was attempting to provide reasonable answers to the question: "how should we handle cases when both @default and the new = syntax are present."

(Currently on this branch, if I use both I get JSON like:

{
  "protocol" : "Test",
  "namespace" : null,
  "types" : [ {
    "type" : "enum",
    "name" : "Status",
    "symbols" : [ "SUCCESS", "FAILURE", "UNKNOWN" ],
    "default" : "FAILURE",
    "default" : "UNKOWN"
  } ],
  "messages" : { }
}

)

@default

…hat the @default annotation no longer works on enum avdl definitions. Updated the way that SchemaBuilder allows an enum to be built sith a default by separating out the default setter.

…adable and well-formatted.

bellemare · 2018-04-28T02:52:55Z

lang/java/avro/src/test/java/org/apache/avro/TestSchemaBuilder.java

+    Schema expected = Schema.createEnum("myenum", null, null, symbols, enumDefault);
+    expected.addProp("p", "v");
+    Schema schema = SchemaBuilder.enumeration("myenum")
+      .prop("p", "v").defaultSymbol(enumDefault).symbols("a", "b");


So this particular code decision means that the default must be set before calling .symbols(). It will not work the other way around.

bellemare · 2018-04-28T02:55:48Z

lang/java/avro/src/main/java/org/apache/avro/Schema.java

+      Set reserved = SCHEMA_RESERVED;
+      if (type.equals("enum")) {
+        reserved = ENUM_RESERVED;
+      }


The intention here was to not cause any unintended side effects.

bellemare · 2018-04-28T02:59:36Z

One more quick statement - I noticed that there is next to no testing around the usage of the "@default" notation. I know that my changes work only because I cannot use the "@default" notation on my enums anymore. However, I do not have any tests to prove that. Should this also be part of this JIRA?

cutting · 2018-05-01T16:38:05Z

lang/java/avro/src/main/java/org/apache/avro/Schema.java

      Iterator<String> i = schema.getFieldNames();
+
+      Set reserved = SCHEMA_RESERVED;
+      if (type.equals("enum")) {


The test should instead be 'type == ENUM', no?

I just kept it consistent with the other references to it in this file. See line 1274 and 1330. "type" is actually a string here, believe it or not.

Sorry, I somehow tagged the wrong line. I meant line 105, not this one.

cutting · 2018-05-01T16:40:17Z

doc/src/content/xdocs/idl.xml

+      <source>
+        enum Shapes {
+          SQUARE, TRIANGLE, CIRCLE
+        } = CIRCLE


This needs a semicolon. I don't know if that's the best syntax, but it is what we implemented!

cutting · 2018-05-01T16:47:30Z

As far as @default, I think it's probably a bug that this was not a reserved word in IDL from the start. We could fix that, but it might break folks who've relied on it (although you could not have used it to set field defaults previously, only to provide a property named "default" on other kinds of schemas). So only breaking folks who used it on enums is probably best, since we're changing the semantics of that case anyway. Does that seem reasonable?

bellemare · 2018-05-01T17:40:02Z

My preference is to impose a minimal change centered around the functionality. In this case I would prefer to leave the @default behaviour as it currently is except for changing it for enum. That being said, I would be happy to file a bug ticket for it as a separate fix - that way if there is a bunch of issues with it that it would be addressable on its own and not affect this commit.

cutting · 2018-05-01T18:14:19Z

It sounds like we agree about @default. With the current patch its behavior is only changed for enums, and that this is appropriate.

cutting

Once the semicolon is added to the example in the documentation, I'm ready to commit this.

cutting · 2018-05-01T20:06:35Z

doc/src/content/xdocs/idl.xml

+      <source>
+enum Shapes {
+  SQUARE, TRIANGLE, CIRCLE
+} = CIRCLE


need a semicolon at EOL

bellemare · 2018-05-02T17:05:37Z

@cutting Thanks for your help Doug, I really appreciate all the feedback and assistance.

cutting requested changes Mar 13, 2018

View reviewed changes

Adam Bellemare added 3 commits April 8, 2018 13:44

AVRO-1340: Added Enum Defaults and unit tests.

d9c81a9

AVRO-1340: Added support for enum defaults to SchemaBuilder

e5c978e

AVRO-1340: Added ResolvingVisitor enum default, introduces an attempt…

739cce8

… at the idl.jj for enum default but tests not passing for simple.avdl

bellemare force-pushed the AVRO-1340-alt branch from e2ff000 to 739cce8 Compare April 14, 2018 15:33

AVRO-1340: Added override annotation to getEnumDefault() implementation

a501944

AVRO-1340: Added Doug Cutting\'s fix to the idl.jj for Avro enum pars…

c3be4cd

…ing. Added test case to simple.avdl and simple.avpr

Adam Bellemare added 2 commits April 27, 2018 22:33

AVRO-1340: Updated docs, updated the reserved keywords for enums so t…

8216b1d

…hat the @default annotation no longer works on enum avdl definitions. Updated the way that SchemaBuilder allows an enum to be built sith a default by separating out the default setter.

AVRO-1340: Updated the docs and verified that the generated doc is re…

621775d

…adable and well-formatted.

bellemare commented Apr 28, 2018

View reviewed changes

cutting requested changes May 1, 2018

View reviewed changes

AVRO-1340: Updated the enum type check to a reasonable, sane check.

3263b82

cutting requested changes May 1, 2018

View reviewed changes

doc/src/content/xdocs/idl.xml Outdated

<source>

enum Shapes {

SQUARE, TRIANGLE, CIRCLE

} = CIRCLE

Copy link

Contributor

cutting May 1, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

need a semicolon at EOL

AVRO-1340: Added the missing semicolon to enum doc

0e9a940

cutting approved these changes May 2, 2018

View reviewed changes

cutting merged commit 238714d into apache:master May 2, 2018

raphaelauv mentioned this pull request May 16, 2019

Incompatible enum changes are allowed in forward compatibility mode confluentinc/schema-registry#601

Closed

AVRO-1340: Added Enum Defaults and unit tests. #298

AVRO-1340: Added Enum Defaults and unit tests. #298

Uh oh!

Conversation

bellemare commented Mar 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cutting left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cutting commented Apr 16, 2018

Uh oh!

bellemare commented Apr 17, 2018

Uh oh!

cutting commented Apr 18, 2018

Uh oh!

BirdHowl commented Apr 18, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bellemare commented Apr 28, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cutting commented May 1, 2018

Uh oh!

bellemare commented May 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cutting commented May 1, 2018

Uh oh!

cutting left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bellemare commented May 2, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

bellemare commented Mar 11, 2018 •

edited

Loading

bellemare commented May 1, 2018 •

edited

Loading