Uploaded image for project: 'Avro'
  1. Avro
  2. AVRO-2072

ResolvingGrammarGenerator doesn't implement schema resolution correctly for unions

    Details

    • Type: Bug
    • Status: Resolved
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: 1.7.7, 1.8.1
    • Fix Version/s: 1.9.0
    • Component/s: java
    • Labels:
      None
    • Hadoop Flags:
      Incompatible change, Reviewed
    • Release Note:
      However, schema resolution is now working according to spec it is a backward incompatible change in the behaviour that int is promotable to float (in addition to long and double) and log to float (in addition to double) when used in unions.

      Description

      According to specification, int and long is promotable to float, but when using SchemaValidator, a union with a single int or long branch is not readable by an union with a float branch.

      1. 0001-AVRO-1931-Additional-test-cases.patch
        12 kB
        Anders Sundelin
      2. AVRO-2072_2.patch
        10 kB
        Nandor Kollar
      3. AVRO-2072_3.patch
        10 kB
        Nandor Kollar
      4. AVRO-2072.patch
        0.9 kB
        Nandor Kollar

        Issue Links

          Activity

          Hide
          zolyfarkas Zoltan Farkas added a comment -

          Applying the patch results in some test failures:

          Failed tests:
          TestReadingWritingDataInEvolvedSchemas.intUnionIsNotConvertedToFloatUnion Expected test to throw (an instance of org.apache.avro.AvroTypeException and exception with message a string containing "Found int, expecting union")
          TestReadingWritingDataInEvolvedSchemas.longUnionIsNotConvertedToFloatUnion Expected test to throw (an instance of org.apache.avro.AvroTypeException and exception with message a string containing "Found long, expecting union")
          TestReadingWritingDataInEvolvedSchemas.longWrittenWithUnionSchemaIsConvertedToFloatDoubleUnionSchema:205 expected: java.lang.Double<42.0> but was: java.lang.Float<42.0>

          Show
          zolyfarkas Zoltan Farkas added a comment - Applying the patch results in some test failures: Failed tests: TestReadingWritingDataInEvolvedSchemas.intUnionIsNotConvertedToFloatUnion Expected test to throw (an instance of org.apache.avro.AvroTypeException and exception with message a string containing "Found int, expecting union") TestReadingWritingDataInEvolvedSchemas.longUnionIsNotConvertedToFloatUnion Expected test to throw (an instance of org.apache.avro.AvroTypeException and exception with message a string containing "Found long, expecting union") TestReadingWritingDataInEvolvedSchemas.longWrittenWithUnionSchemaIsConvertedToFloatDoubleUnionSchema:205 expected: java.lang.Double<42.0> but was: java.lang.Float<42.0>
          Hide
          nkollar Nandor Kollar added a comment -

          Thanks Zoltan, I'll have a look at the failing tests!

          Show
          nkollar Nandor Kollar added a comment - Thanks Zoltan, I'll have a look at the failing tests!
          Hide
          nkollar Nandor Kollar added a comment -

          Zoltan Farkas I don't see any test failure, guess you executed tests on your forked version instead of Apache master. Looks like intUnionIsNotConvertedToFloatUnion and longUnionIsNotConvertedToFloatUnion are negative test: testing exactly what this Jira intends to fix right? Are these tests your own test cases, or this was merged with a patch available Jira, which is not yet merged to upstream trunk? As of the last test case, I'm not sure what is the problem.

          Show
          nkollar Nandor Kollar added a comment - Zoltan Farkas I don't see any test failure, guess you executed tests on your forked version instead of Apache master. Looks like intUnionIsNotConvertedToFloatUnion and longUnionIsNotConvertedToFloatUnion are negative test: testing exactly what this Jira intends to fix right? Are these tests your own test cases, or this was merged with a patch available Jira, which is not yet merged to upstream trunk? As of the last test case, I'm not sure what is the problem.
          Hide
          zolyfarkas Zoltan Farkas added a comment - - edited

          I executed this against my branch... which is ahead of the official branch in certain places and behind in others...
          THeese failures do highligh the need for this patch to contains some tests to test the functionality.

          TestReadingWritingDataInEvolvedSchemas.longWrittenWithUnionSchemaIsConvertedToFloatDoubleUnionSchema

          highlights actually an interesting evolution case which the patch does not cover:

          int field -> union {float, double} field;
          

          The test validates that int are promoted to double... meanwhile the patch promotes it to float (first compatible type in the union)

          I think the spec should be undated to clarify what needs to be done here...

          Show
          zolyfarkas Zoltan Farkas added a comment - - edited I executed this against my branch... which is ahead of the official branch in certain places and behind in others... THeese failures do highligh the need for this patch to contains some tests to test the functionality. TestReadingWritingDataInEvolvedSchemas.longWrittenWithUnionSchemaIsConvertedToFloatDoubleUnionSchema highlights actually an interesting evolution case which the patch does not cover: int field -> union { float , double } field; The test validates that int are promoted to double... meanwhile the patch promotes it to float (first compatible type in the union) I think the spec should be undated to clarify what needs to be done here...
          Hide
          nkollar Nandor Kollar added a comment -

          Zoltan Farkas I think this is fine, the test should expect float instead of Double in this case, since the spec says:

          if reader's is a union, but writer's is not
          The first schema in the reader's union that matches the writer's schema is recursively resolved against it. If none match, an error is signalled.

          and int field will (with the attach patch) match with float branch now, and not with double. I think this is exactly what the spec says, no?

          Show
          nkollar Nandor Kollar added a comment - Zoltan Farkas I think this is fine, the test should expect float instead of Double in this case, since the spec says: if reader's is a union, but writer's is not The first schema in the reader's union that matches the writer's schema is recursively resolved against it. If none match, an error is signalled. and int field will (with the attach patch) match with float branch now, and not with double. I think this is exactly what the spec says, no?
          Hide
          zolyfarkas Zoltan Farkas added a comment -

          Nandor Kollar Missed that part of the spec, the patch behavior is compliant with the spec... an update to the unit test is needed to comply with the patch.

          BTW these unit tests have been added in AVRO-1931: https://issues.apache.org/jira/secure/attachment/12832657/AVRO-1931-2.patch

          Show
          zolyfarkas Zoltan Farkas added a comment - Nandor Kollar Missed that part of the spec, the patch behavior is compliant with the spec... an update to the unit test is needed to comply with the patch. BTW these unit tests have been added in AVRO-1931 : https://issues.apache.org/jira/secure/attachment/12832657/AVRO-1931-2.patch
          Hide
          nkollar Nandor Kollar added a comment -

          Ah okay, I think the pull request for AVRO-1931 didn't include these tests, just the attached patch, that's why it was closed but the tests are not there. Anders Sundelin could you please help identifying the difference between your attached patch and your PR? Looks like some test cases are missing.

          Show
          nkollar Nandor Kollar added a comment - Ah okay, I think the pull request for AVRO-1931 didn't include these tests, just the attached patch, that's why it was closed but the tests are not there. Anders Sundelin could you please help identifying the difference between your attached patch and your PR? Looks like some test cases are missing.
          Hide
          epkanol Anders Sundelin added a comment -

          Hi Nandor and others,

          My bad, I based my pull request on my original patch, so the additional
          test cases are not there, hence, are not merged into master.

          Can I add the additional test cases to AVRO-2072, or should we create a
          separate issue for this?

          There is no semantic difference (only white-space) in the
          SchemaCompatibility class itself, comparing the AVRO-1931.patch and
          AVRO-1931-2.patch.

          As I also have another PR open (AVRO-1933), we certainly could add the
          tests there, (this patch also deals with SchemaCompatibility). But that
          patch is slightly more involved, and has no real bearing on
          TestReadingWritingDataInEvolvedSchemas.

          I seem to recall how I ended up like this - first I discovered the
          problem, created the fix and some elementary tests, which was added as a
          Jira. Then, I expanded on the tests, and updated the patch (hence, the
          -2 suffix), but this was apparently forgotten when I created the github
          fork. The perils of mixing patches and github :-/ I totally agree that
          we should stick to github (i.e. one way of doing patches)

          BR

          /Anders

          Show
          epkanol Anders Sundelin added a comment - Hi Nandor and others, My bad, I based my pull request on my original patch, so the additional test cases are not there, hence, are not merged into master. Can I add the additional test cases to AVRO-2072 , or should we create a separate issue for this? There is no semantic difference (only white-space) in the SchemaCompatibility class itself, comparing the AVRO-1931 .patch and AVRO-1931 -2.patch. As I also have another PR open ( AVRO-1933 ), we certainly could add the tests there, (this patch also deals with SchemaCompatibility). But that patch is slightly more involved, and has no real bearing on TestReadingWritingDataInEvolvedSchemas. I seem to recall how I ended up like this - first I discovered the problem, created the fix and some elementary tests, which was added as a Jira. Then, I expanded on the tests, and updated the patch (hence, the -2 suffix), but this was apparently forgotten when I created the github fork. The perils of mixing patches and github :-/ I totally agree that we should stick to github (i.e. one way of doing patches) BR /Anders
          Hide
          epkanol Anders Sundelin added a comment -

          Attached the additional test cases (compared with current Avro master), that I forgot in PR for AVRO-1931 (but was included in the JIRA patch for said issue).
          Note! I have not modified your original AVRO-2072 patch, so it is likely that these tests will break when applied on AVRO-2072 patch (as indicated by Zoltan Farkas in above comment)

          Show
          epkanol Anders Sundelin added a comment - Attached the additional test cases (compared with current Avro master), that I forgot in PR for AVRO-1931 (but was included in the JIRA patch for said issue). Note! I have not modified your original AVRO-2072 patch, so it is likely that these tests will break when applied on AVRO-2072 patch (as indicated by Zoltan Farkas in above comment)
          Hide
          epkanol Anders Sundelin added a comment -

          Please feel free to update the tests accordingly

          Show
          epkanol Anders Sundelin added a comment - Please feel free to update the tests accordingly
          Hide
          nkollar Nandor Kollar added a comment -

          Thanks Anders Sundelin, I uploaded AVRO-2072_2.patch with your additional test cases, modified the failing ones in TestReadingWritingDataInEvolvedSchemas which shouldn't fail after the fix in ResolvingGrammarGenerator, and deleted the ignored case from TestSchemaCompatibility, since we should support those promotions: changing the specification is not the best idea according to Doug Cutting and Sean Busbey, so we should allow this promotion, thus this is a bug in the implementation (see discussion on Avro user's mailing list). Anders, could you please help with a review?

          Show
          nkollar Nandor Kollar added a comment - Thanks Anders Sundelin , I uploaded AVRO-2072 _2.patch with your additional test cases, modified the failing ones in TestReadingWritingDataInEvolvedSchemas which shouldn't fail after the fix in ResolvingGrammarGenerator , and deleted the ignored case from TestSchemaCompatibility , since we should support those promotions: changing the specification is not the best idea according to Doug Cutting and Sean Busbey , so we should allow this promotion, thus this is a bug in the implementation (see discussion on Avro user's mailing list). Anders, could you please help with a review?
          Hide
          epkanol Anders Sundelin added a comment -

          I have reviewed your patch, Nandor Kollar, and it certainly looks OK with me. All the relevant test cases are there, and you updated the failing ones, like you said.
          My only gripe is that the case statements are on the same line (this was also the case before your patch, so I understand why you kept the style). This is not according to the Avro conventions (which says that Sun Java conventions, with two-space indent) shall be used.
          Also, I think that the method "ResolvingGrammarGenerator.bestBranch" does not really live up to its name - it will find the first matching branch, not necessarily the best one (e.g. when the writer schema is [long] and the reader is [float, double], in that order, it will pick float to read the value, instead of double, which would prevent data-loss. Easily verified by the following test case (placed in TestReadingWritingDataInEvolvedSchemas):
          {{
          @Test
          public void longIsReadByUnionFloatDoubleSchema() throws Exception

          { Schema writer = LONG_RECORD; Record record = defaultRecordWithSchema(writer, FIELD_A, 42L); byte[] encoded = encodeGenericBlob(record); assertEquals(42.0f, decodeGenericBlob(UNION_FLOAT_DOUBLE_RECORD, writer, encoded).get(FIELD_A)); }

          }}
          But this method was called that before your patch as well, so I would not fail the patch because of this.
          +1 from me

          Show
          epkanol Anders Sundelin added a comment - I have reviewed your patch, Nandor Kollar , and it certainly looks OK with me. All the relevant test cases are there, and you updated the failing ones, like you said. My only gripe is that the case statements are on the same line (this was also the case before your patch, so I understand why you kept the style). This is not according to the Avro conventions (which says that Sun Java conventions, with two-space indent) shall be used. Also, I think that the method "ResolvingGrammarGenerator.bestBranch" does not really live up to its name - it will find the first matching branch, not necessarily the best one (e.g. when the writer schema is [long] and the reader is [float, double] , in that order, it will pick float to read the value, instead of double, which would prevent data-loss. Easily verified by the following test case (placed in TestReadingWritingDataInEvolvedSchemas): {{ @Test public void longIsReadByUnionFloatDoubleSchema() throws Exception { Schema writer = LONG_RECORD; Record record = defaultRecordWithSchema(writer, FIELD_A, 42L); byte[] encoded = encodeGenericBlob(record); assertEquals(42.0f, decodeGenericBlob(UNION_FLOAT_DOUBLE_RECORD, writer, encoded).get(FIELD_A)); } }} But this method was called that before your patch as well, so I would not fail the patch because of this. +1 from me
          Hide
          epkanol Anders Sundelin added a comment -

          Please substitute [long] with [int] in my prior comment - long-to-double conversion would always be lossy (both are 64-bit quantities). But my argument about the method name still remains, it will pick the "first match", not necessarily the "best match" (though its behaviour is according to the Avro spec, afaik)

          Show
          epkanol Anders Sundelin added a comment - Please substitute [long] with [int] in my prior comment - long-to-double conversion would always be lossy (both are 64-bit quantities). But my argument about the method name still remains, it will pick the "first match", not necessarily the "best match" (though its behaviour is according to the Avro spec, afaik)
          Hide
          nkollar Nandor Kollar added a comment -

          Anders Sundelin thanks for the review, in AVRO-2072_3.patch I modified the indentation according to your comment, and renamed the method to firstMatchingBranch to reflect the logic better.

          Show
          nkollar Nandor Kollar added a comment - Anders Sundelin thanks for the review, in AVRO-2072 _3.patch I modified the indentation according to your comment, and renamed the method to firstMatchingBranch to reflect the logic better.
          Hide
          epkanol Anders Sundelin added a comment -

          +1 for patch 3, I have no further comments, it looks good to me.

          Show
          epkanol Anders Sundelin added a comment - +1 for patch 3, I have no further comments, it looks good to me.
          Hide
          gszadovszky Gabor Szadovszky added a comment -

          +1
          Will push it soon

          Show
          gszadovszky Gabor Szadovszky added a comment - +1 Will push it soon
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit 50eebb8a0b96565641bb56a0f11e326b3f5cc50b in avro's branch refs/heads/master from Nandor Kollar
          [ https://git-wip-us.apache.org/repos/asf?p=avro.git;h=50eebb8 ]

          AVRO-2072: ResolvingGrammarGenerator doesn't implement schema resolution correctly for unions

          Signed-off-by: Anders Sundelin <anders.sundelin@ericsson.com>
          Signed-off-by: Gabor Szadovszky <gabor@apache.org>

          Show
          jira-bot ASF subversion and git services added a comment - Commit 50eebb8a0b96565641bb56a0f11e326b3f5cc50b in avro's branch refs/heads/master from Nandor Kollar [ https://git-wip-us.apache.org/repos/asf?p=avro.git;h=50eebb8 ] AVRO-2072 : ResolvingGrammarGenerator doesn't implement schema resolution correctly for unions Signed-off-by: Anders Sundelin <anders.sundelin@ericsson.com> Signed-off-by: Gabor Szadovszky <gabor@apache.org>
          Hide
          nkollar Nandor Kollar added a comment -

          Thanks Gabor Szadovszky and Anders Sundelin for reviewing and committing this patch!

          Show
          nkollar Nandor Kollar added a comment - Thanks Gabor Szadovszky and Anders Sundelin for reviewing and committing this patch!

            People

            • Assignee:
              nkollar Nandor Kollar
              Reporter:
              nkollar Nandor Kollar
            • Votes:
              0 Vote for this issue
              Watchers:
              5 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development