Avro
  1. Avro
  2. AVRO-1046

ReflectDatumReader doesn't work with SpecificRecords containing an array of values

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 1.6.2
    • Fix Version/s: 1.7.0
    • Component/s: java
    • Labels:
      None

      Description

      When a ReflectDatumReader is used to read implementations of SpecificRecord, it fails if the SpecificRecord includes an (avro) array. This appears to be due to the fact that the newArray method in ReflectDatumReader works differently (based on a reflection-based schema) than the newArray method in GenericDatumReader (which is the base class of the DatumReaders).

      The included patch simply removes the delegation to SpecificData for the creation of a schema in the case of a SpecificRecord. My assumption is that the delegation to SpecificData is for performance reasons.

      The rationale in removing the delegation to SpecificData is that a) it doesn't work completely correctly, as demonstrated, and b) if a user is using ReflectDatumReader (instead of SpecificDatumReader) to read SpecificRecords, there may be other underlying reasons that reflection is specifically chosen, and so this intention should not be undermined by the delegation to SpecificData.

      1. AVRO-1046.patch
        18 kB
        Doug Cutting
      2. AVRO-1046.patch
        16 kB
        Gabriel Reid

        Activity

        Transition Time In Source Status Execution Times Last Executer Last Execution Date
        Open Open Patch Available Patch Available
        7d 36m 1 Gabriel Reid 23/Mar/12 14:47
        Patch Available Patch Available Resolved Resolved
        55d 6h 25m 1 Doug Cutting 17/May/12 22:13
        Resolved Resolved Closed Closed
        24d 22h 48m 1 Doug Cutting 11/Jun/12 21:01
        Doug Cutting made changes -
        Status Resolved [ 5 ] Closed [ 6 ]
        Doug Cutting made changes -
        Status Patch Available [ 10002 ] Resolved [ 5 ]
        Resolution Fixed [ 1 ]
        Hide
        Doug Cutting added a comment -

        I committed this. Thanks, Gabriel!

        Show
        Doug Cutting added a comment - I committed this. Thanks, Gabriel!
        Hide
        Doug Cutting added a comment -

        Great. I'll commit this soon unless someone objects.

        Show
        Doug Cutting added a comment - Great. I'll commit this soon unless someone objects.
        Hide
        Gabriel Reid added a comment -

        Yes, that's excellent, thanks!

        I was originally looking down the path of updating ReflectDatumReader, but clearly didn't realize how (relatively) easy it could be.

        BTW, sorry for the failing unit tests – I thought I was in the clear as all of the tests in the avro sub-project were passing, so I didn't see that the ipc sub-project had failing tests (assuming it was there that you were getting failing tests?)

        Show
        Gabriel Reid added a comment - Yes, that's excellent, thanks! I was originally looking down the path of updating ReflectDatumReader, but clearly didn't realize how (relatively) easy it could be. BTW, sorry for the failing unit tests – I thought I was in the clear as all of the tests in the avro sub-project were passing, so I didn't see that the ipc sub-project had failing tests (assuming it was there that you were getting failing tests?)
        Doug Cutting made changes -
        Assignee Doug Cutting [ cutting ]
        Fix Version/s 1.7.0 [ 12318848 ]
        Affects Version/s 1.7.0 [ 12318848 ]
        Doug Cutting made changes -
        Attachment AVRO-1046.patch [ 12526588 ]
        Hide
        Doug Cutting added a comment -

        ReflectDatumReader is intended to permit reading specific and generic objects too, so this would be an incompatible change. Your patch causes existing tests to fail.

        Here's a patch that instead fixes ReflectDatumReader to be able to better determine what kind of array to create.

        Does this work for you? All tests pass for me, including the ones you provided with your patch.

        Show
        Doug Cutting added a comment - ReflectDatumReader is intended to permit reading specific and generic objects too, so this would be an incompatible change. Your patch causes existing tests to fail. Here's a patch that instead fixes ReflectDatumReader to be able to better determine what kind of array to create. Does this work for you? All tests pass for me, including the ones you provided with your patch.
        Gabriel Reid made changes -
        Status Open [ 1 ] Patch Available [ 10002 ]
        Hide
        Gabriel Reid added a comment -

        Patch that fixes the issue by not delegating to the SpecificDatumReader when reading a SpecificData via the ReflectDatumReader (rationale for the patch is included in the description of the issue).

        Show
        Gabriel Reid added a comment - Patch that fixes the issue by not delegating to the SpecificDatumReader when reading a SpecificData via the ReflectDatumReader (rationale for the patch is included in the description of the issue).
        Gabriel Reid made changes -
        Description When a ReflectDatumReader is used to read implementations of SpecificRecord, it fails if the SpecificRecord includes an (avro) array. This appears to be due to the fact that the newArray method in ReflectDatumReader works differently (based on a reflection-based schema) than the newArray method in GenericDatumReader (which is the base class of the DatumReaders).

        I'll include a patch with unit tests that demonstrate the issue, as well as a fix for it.
        When a ReflectDatumReader is used to read implementations of SpecificRecord, it fails if the SpecificRecord includes an (avro) array. This appears to be due to the fact that the newArray method in ReflectDatumReader works differently (based on a reflection-based schema) than the newArray method in GenericDatumReader (which is the base class of the DatumReaders).

        The included patch simply removes the delegation to SpecificData for the creation of a schema in the case of a SpecificRecord. My assumption is that the delegation to SpecificData is for performance reasons.

        The rationale in removing the delegation to SpecificData is that a) it doesn't work completely correctly, as demonstrated, and b) if a user is using ReflectDatumReader (instead of SpecificDatumReader) to read SpecificRecords, there may be other underlying reasons that reflection is specifically chosen, and so this intention should not be undermined by the delegation to SpecificData.
        Gabriel Reid made changes -
        Field Original Value New Value
        Attachment AVRO-1046.patch [ 12518670 ]
        Hide
        Gabriel Reid added a comment -

        Patch which demonstrates and resolves an issue where the ReflectDatumReader fails when reading a SpecificData instance.

        Show
        Gabriel Reid added a comment - Patch which demonstrates and resolves an issue where the ReflectDatumReader fails when reading a SpecificData instance.
        Gabriel Reid created issue -

          People

          • Assignee:
            Doug Cutting
            Reporter:
            Gabriel Reid
          • Votes:
            0 Vote for this issue
            Watchers:
            2 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development