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
        16 kB
        Gabriel Reid
      2. AVRO-1046.patch
        18 kB
        Doug Cutting

        Activity

        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?)
        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.
        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).
        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.

          People

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

            Dates

            • Created:
              Updated:
              Resolved:

              Development