svn commit: r1628335 - in /avro/trunk: ./ lang/java/avro/src/main/java/org/apache/avro/io/parsing/ lang/java/avro/src/test/java/org/apache/avro/ lang/java/avro/src/test/java/org/apache/avro/io/parsing/

[cu...@apache.org]

Author: cutting
Date: Mon Sep 29 22:58:29 2014
New Revision: 1628335

URL: http://svn.apache.org/r1628335
Log:
AVRO-1590. Java: In resolving records in unions, permit structural and shortname matches when fullname matching fails.  Contributed by Ryan Blue.

Modified:
    avro/trunk/CHANGES.txt
    avro/trunk/lang/java/avro/src/main/java/org/apache/avro/io/parsing/ResolvingGrammarGenerator.java
    avro/trunk/lang/java/avro/src/test/java/org/apache/avro/TestSchemaValidation.java
    avro/trunk/lang/java/avro/src/test/java/org/apache/avro/io/parsing/TestResolvingGrammarGenerator2.java

Modified: avro/trunk/CHANGES.txt
URL: http://svn.apache.org/viewvc/avro/trunk/CHANGES.txt?rev=1628335&r1=1628334&r2=1628335&view=diff
==============================================================================
--- avro/trunk/CHANGES.txt (original)
+++ avro/trunk/CHANGES.txt Mon Sep 29 22:58:29 2014
@@ -13,6 +13,10 @@ Trunk (not yet released)
     AVRO-739. Add date, time, timestamp, and duration binary types to
     specification. (Dmitry Kovalev and Ryan Blue via tomwhite)
 
+    AVRO-1590. Java: In resolving records in unions, permit structural
+    and shortname matches when fullname matching fails.
+    (Ryan Blue via cutting)
+
   OPTIMIZATIONS
 
   IMPROVEMENTS

Modified: avro/trunk/lang/java/avro/src/main/java/org/apache/avro/io/parsing/ResolvingGrammarGenerator.java
URL: http://svn.apache.org/viewvc/avro/trunk/lang/java/avro/src/main/java/org/apache/avro/io/parsing/ResolvingGrammarGenerator.java?rev=1628335&r1=1628334&r2=1628335&view=diff
==============================================================================
--- avro/trunk/lang/java/avro/src/main/java/org/apache/avro/io/parsing/ResolvingGrammarGenerator.java (original)
+++ avro/trunk/lang/java/avro/src/main/java/org/apache/avro/io/parsing/ResolvingGrammarGenerator.java Mon Sep 29 22:58:29 2014
@@ -166,7 +166,7 @@ public class ResolvingGrammarGenerator e
         break;
   
       case UNION:
-        int j = bestBranch(reader, writer);
+        int j = bestBranch(reader, writer, seen);
         if (j >= 0) {
           Symbol s = generate(writer, reader.getTypes().get(j), seen);
           return Symbol.seq(Symbol.unionAdjustAction(j, s), Symbol.UNION);
@@ -426,24 +426,64 @@ public class ResolvingGrammarGenerator e
     return Symbol.enumAdjustAction(rsymbols.size(), adjustments);
   }
 
-  private static int bestBranch(Schema r, Schema w) {
+  /**
+   * This checks if the symbol itself is an error or if there is an error in
+   * its production.
+   *
+   * When the symbol is created for a record, this checks whether the record
+   * fields are present (the symbol is not an error action) and that all of the
+   * fields have a non-error action. Record fields may have nested error
+   * actions.
+   *
+   * @return true if the symbol is an error or if its production has an error
+   */
+  private boolean hasMatchError(Symbol sym) {
+    if (sym instanceof Symbol.ErrorAction) {
+      return true;
+    } else {
+      for (int i = 0; i < sym.production.length; i += 1) {
+        if (sym.production[i] instanceof Symbol.ErrorAction) {
+          return true;
+        }
+      }
+    }
+    return false;
+  }
+
+  private int bestBranch(Schema r, Schema w, Map<LitS, Symbol> seen) throws IOException {
     Schema.Type vt = w.getType();
       // first scan for exact match
       int j = 0;
+      int structureMatch = -1;
       for (Schema b : r.getTypes()) {
         if (vt == b.getType())
-          if (vt == Schema.Type.RECORD || vt == Schema.Type.ENUM || 
+          if (vt == Schema.Type.RECORD || vt == Schema.Type.ENUM ||
               vt == Schema.Type.FIXED) {
             String vname = w.getFullName();
             String bname = b.getFullName();
-            if ((vname != null && vname.equals(bname))
-                || vname == bname && vt == Schema.Type.RECORD)
+            // return immediately if the name matches exactly according to spec
+            if (vname != null && vname.equals(bname))
               return j;
+
+            if (vt == Schema.Type.RECORD &&
+                !hasMatchError(resolveRecords(w, b, seen))) {
+              String vShortName = w.getName();
+              String bShortName = b.getName();
+              // use the first structure match or one where the name matches
+              if ((structureMatch < 0) ||
+                  (vShortName != null && vShortName.equals(bShortName))) {
+                structureMatch = j;
+              }
+            }
           } else
             return j;
         j++;
       }
 
+      // if there is a record structure match, return it
+      if (structureMatch >= 0)
+        return structureMatch;
+
       // then scan match via numeric promotion
       j = 0;
       for (Schema b : r.getTypes()) {

Modified: avro/trunk/lang/java/avro/src/test/java/org/apache/avro/TestSchemaValidation.java
URL: http://svn.apache.org/viewvc/avro/trunk/lang/java/avro/src/test/java/org/apache/avro/TestSchemaValidation.java?rev=1628335&r1=1628334&r2=1628335&view=diff
==============================================================================
--- avro/trunk/lang/java/avro/src/test/java/org/apache/avro/TestSchemaValidation.java (original)
+++ avro/trunk/lang/java/avro/src/test/java/org/apache/avro/TestSchemaValidation.java Mon Sep 29 22:58:29 2014
@@ -20,6 +20,7 @@ package org.apache.avro;
 
 import java.util.ArrayList;
 
+import org.apache.avro.reflect.ReflectData;
 import org.junit.Assert;
 import org.junit.Test;
 
@@ -119,6 +120,48 @@ public class TestSchemaValidation {
     builder.strategy(null).validateAll();
   }
 
+  public static class Point {
+    double x;
+    double y;
+  }
+
+  public static class Circle {
+    Point center;
+    double radius;
+  }
+
+  public static final Schema circleSchema = SchemaBuilder.record("Circle")
+      .fields()
+      .name("center").type().record("Point")
+          .fields()
+          .requiredDouble("x")
+          .requiredDouble("y")
+          .endRecord().noDefault()
+      .requiredDouble("radius")
+      .endRecord();
+
+  public static final Schema circleSchemaDifferentNames = SchemaBuilder
+      .record("crcl").fields()
+      .name("center").type().record("pt")
+      .fields()
+      .requiredDouble("x")
+      .requiredDouble("y")
+      .endRecord().noDefault()
+      .requiredDouble("radius")
+      .endRecord();
+
+  @Test
+  public void testReflectMatchStructure() throws SchemaValidationException {
+    testValidatorPasses(builder.canBeReadStrategy().validateAll(),
+        circleSchemaDifferentNames, ReflectData.get().getSchema(Circle.class));
+  }
+
+  @Test
+  public void testReflectWithAllowNullMatchStructure() throws SchemaValidationException {
+    testValidatorPasses(builder.canBeReadStrategy().validateAll(),
+        circleSchemaDifferentNames, ReflectData.AllowNull.get().getSchema(Circle.class));
+  }
+
   private void testValidatorPasses(SchemaValidator validator,
       Schema schema, Schema... prev) throws SchemaValidationException {
     ArrayList<Schema> prior = new ArrayList<Schema>();

Modified: avro/trunk/lang/java/avro/src/test/java/org/apache/avro/io/parsing/TestResolvingGrammarGenerator2.java
URL: http://svn.apache.org/viewvc/avro/trunk/lang/java/avro/src/test/java/org/apache/avro/io/parsing/TestResolvingGrammarGenerator2.java?rev=1628335&r1=1628334&r2=1628335&view=diff
==============================================================================
--- avro/trunk/lang/java/avro/src/test/java/org/apache/avro/io/parsing/TestResolvingGrammarGenerator2.java (original)
+++ avro/trunk/lang/java/avro/src/test/java/org/apache/avro/io/parsing/TestResolvingGrammarGenerator2.java Mon Sep 29 22:58:29 2014
@@ -17,8 +17,12 @@
  */
 package org.apache.avro.io.parsing;
 
-import org.apache.avro.io.parsing.ResolvingGrammarGenerator;
+import java.util.Arrays;
+import org.apache.avro.SchemaBuilder;
+import org.apache.avro.SchemaValidationException;
+import org.apache.avro.SchemaValidatorBuilder;
 import org.apache.avro.Schema;
+import org.junit.Assert;
 import org.junit.Test;
 
 /** ResolvingGrammarGenerator tests that are not Parameterized.*/
@@ -31,4 +35,109 @@ public class TestResolvingGrammarGenerat
       (Schema.create(Schema.Type.BYTES),
        Schema.createFixed("MyFixed", null, null, 10));
   }
+
+  Schema point2dFullname = SchemaBuilder.record("Point").namespace("written")
+      .fields()
+      .requiredDouble("x")
+      .requiredDouble("y")
+      .endRecord();
+
+  Schema point3dNoDefault = SchemaBuilder.record("Point").fields()
+      .requiredDouble("x")
+      .requiredDouble("y")
+      .requiredDouble("z")
+      .endRecord();
+
+  Schema point2d = SchemaBuilder.record("Point2D")
+      .fields()
+      .requiredDouble("x")
+      .requiredDouble("y")
+      .endRecord();
+
+  Schema point3d = SchemaBuilder.record("Point3D").fields()
+      .requiredDouble("x")
+      .requiredDouble("y")
+      .name("z").type().doubleType().doubleDefault(0.0)
+      .endRecord();
+
+  Schema point3dMatchName = SchemaBuilder.record("Point").fields()
+      .requiredDouble("x")
+      .requiredDouble("y")
+      .name("z").type().doubleType().doubleDefault(0.0)
+      .endRecord();
+
+  @Test(expected=SchemaValidationException.class)
+  public void testUnionResolutionNoStructureMatch() throws Exception {
+    // there is a short name match, but the structure does not match
+    Schema read = Schema.createUnion(Arrays.asList(
+        Schema.create(Schema.Type.NULL),
+        point3dNoDefault));
+
+    new SchemaValidatorBuilder().canBeReadStrategy().validateAll()
+        .validate(point2dFullname, Arrays.asList(read));
+  }
+
+  @Test
+  public void testUnionResolutionFirstStructureMatch2d() throws Exception {
+    // multiple structure matches with no short or full name matches
+    Schema read = Schema.createUnion(Arrays.asList(
+        Schema.create(Schema.Type.NULL),
+        point3dNoDefault, point2d, point3d));
+
+    Symbol grammar = new ResolvingGrammarGenerator().generate(
+        point2dFullname, read);
+    Assert.assertTrue(grammar.production[1] instanceof Symbol.UnionAdjustAction);
+
+    Symbol.UnionAdjustAction action = (Symbol.UnionAdjustAction)
+        grammar.production[1];
+    Assert.assertEquals(2, action.rindex);
+  }
+
+  @Test
+  public void testUnionResolutionFirstStructureMatch3d() throws Exception {
+    // multiple structure matches with no short or full name matches
+    Schema read = Schema.createUnion(Arrays.asList(
+        Schema.create(Schema.Type.NULL),
+        point3dNoDefault, point3d, point2d));
+
+    Symbol grammar = new ResolvingGrammarGenerator().generate(
+        point2dFullname, read);
+    Assert.assertTrue(grammar.production[1] instanceof Symbol.UnionAdjustAction);
+
+    Symbol.UnionAdjustAction action = (Symbol.UnionAdjustAction)
+        grammar.production[1];
+    Assert.assertEquals(2, action.rindex);
+  }
+
+  @Test
+  public void testUnionResolutionNamedStructureMatch() throws Exception {
+    // multiple structure matches with a short name match
+    Schema read = Schema.createUnion(Arrays.asList(
+        Schema.create(Schema.Type.NULL),
+        point2d, point3dMatchName, point3d));
+
+    Symbol grammar = new ResolvingGrammarGenerator().generate(
+        point2dFullname, read);
+    Assert.assertTrue(grammar.production[1] instanceof Symbol.UnionAdjustAction);
+
+    Symbol.UnionAdjustAction action = (Symbol.UnionAdjustAction)
+        grammar.production[1];
+    Assert.assertEquals(2, action.rindex);
+  }
+
+  @Test
+  public void testUnionResolutionFullNameMatch() throws Exception {
+    // there is a full name match, so it should be chosen
+    Schema read = Schema.createUnion(Arrays.asList(
+        Schema.create(Schema.Type.NULL),
+        point2d, point3dMatchName, point3d, point2dFullname));
+
+    Symbol grammar = new ResolvingGrammarGenerator().generate(
+        point2dFullname, read);
+    Assert.assertTrue(grammar.production[1] instanceof Symbol.UnionAdjustAction);
+
+    Symbol.UnionAdjustAction action = (Symbol.UnionAdjustAction)
+        grammar.production[1];
+    Assert.assertEquals(4, action.rindex);
+  }
 }