Avro
  1. Avro
  2. AVRO-1299

SpecificRecordBase implements GenericRecord

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 1.7.4
    • Fix Version/s: 1.7.5
    • Component/s: java
    • Labels:
      None

      Description

      Code written for generic records should be directly applicable on equivalent specific records.

      1. AVRO-1299.20130418-133947.patch
        4 kB
        Christophe Taton
      2. AVRO-1299.20130417-101039.patch
        3 kB
        Christophe Taton
      3. AVRO-1299.20130416-222225.patch
        6 kB
        Christophe Taton

        Activity

        Hide
        Hudson added a comment -

        Integrated in AvroJava #364 (See https://builds.apache.org/job/AvroJava/364/)
        AVRO-1299. Java: SpecificRecordBase implements GenericRecord. Contributed by Christophe Taton. (Revision 1471047)

        Result = SUCCESS
        cutting :
        Files :

        • /avro/trunk/CHANGES.txt
        • /avro/trunk/lang/java/avro/src/main/java/org/apache/avro/specific/SpecificRecordBase.java
        • /avro/trunk/lang/java/avro/src/test/java/org/apache/avro/specific/TestSpecificData.java
        Show
        Hudson added a comment - Integrated in AvroJava #364 (See https://builds.apache.org/job/AvroJava/364/ ) AVRO-1299 . Java: SpecificRecordBase implements GenericRecord. Contributed by Christophe Taton. (Revision 1471047) Result = SUCCESS cutting : Files : /avro/trunk/CHANGES.txt /avro/trunk/lang/java/avro/src/main/java/org/apache/avro/specific/SpecificRecordBase.java /avro/trunk/lang/java/avro/src/test/java/org/apache/avro/specific/TestSpecificData.java
        Hide
        Doug Cutting added a comment -

        I committed this. Thanks, Christophe!

        Show
        Doug Cutting added a comment - I committed this. Thanks, Christophe!
        Hide
        Scott Carey added a comment -

        I agree, 101039 seems best. If we didn't already have IndexedRecord.put(), I might want to split the interfaces up now, but there is no reason to roll that in in this JIRA as is.

        Thanks Christophe!

        Note – it will probably be slower to serialize a SpecificRecord via its GenericRecord interface, but probably only a little since there is quite a bit of overhead in both at the moment anyway.

        Show
        Scott Carey added a comment - I agree, 101039 seems best. If we didn't already have IndexedRecord.put(), I might want to split the interfaces up now, but there is no reason to roll that in in this JIRA as is. Thanks Christophe! Note – it will probably be slower to serialize a SpecificRecord via its GenericRecord interface, but probably only a little since there is quite a bit of overhead in both at the moment anyway.
        Hide
        Doug Cutting added a comment -

        I think we agree. If we want to improve things for immutability then we might refactor the interfaces, but throwing an exception for mutable records seems like it accomplishes little. So my vote is for the 101039 version of the patch.

        Show
        Doug Cutting added a comment - I think we agree. If we want to improve things for immutability then we might refactor the interfaces, but throwing an exception for mutable records seems like it accomplishes little. So my vote is for the 101039 version of the patch.
        Hide
        Scott Carey added a comment -

        I guess only my sense of purity – an Immutable object that throws an exception on set feels very dirty and a violation of the contract. At this point, we already have a put() via IndexedRecord interface so I don't see any harm in this.

        Show
        Scott Carey added a comment - I guess only my sense of purity – an Immutable object that throws an exception on set feels very dirty and a violation of the contract. At this point, we already have a put() via IndexedRecord interface so I don't see any harm in this.
        Hide
        Doug Cutting added a comment -

        I don't yet see how implementing set() in SpecificRecordBase now makes implementing immutable records impossible in the future. If this is called on an immutable record then it should throw an exception, but not on a mutable record. What am I missing?

        Show
        Doug Cutting added a comment - I don't yet see how implementing set() in SpecificRecordBase now makes implementing immutable records impossible in the future. If this is called on an immutable record then it should throw an exception, but not on a mutable record. What am I missing?
        Hide
        Scott Carey added a comment -

        I suppose IndexedRecord exists, and this is no different.

        The work required for immutable records is some cleanup and performance improvement to the builders (e.g. remove unnecessary boxing), then those need to be used by the DatumReader classes.

        There is no timeline, but I want to make sure we avoid making things harder in the future if we can and consider our options.

        Show
        Scott Carey added a comment - I suppose IndexedRecord exists, and this is no different. The work required for immutable records is some cleanup and performance improvement to the builders (e.g. remove unnecessary boxing), then those need to be used by the DatumReader classes. There is no timeline, but I want to make sure we avoid making things harder in the future if we can and consider our options.
        Hide
        Christophe Taton added a comment -

        Here is a patch in which put() is not supported (throws an AvroRuntimeException).

        We can also explore splitting the GenericRecord interface into getter and setter interfaces.

        Show
        Christophe Taton added a comment - Here is a patch in which put() is not supported (throws an AvroRuntimeException). We can also explore splitting the GenericRecord interface into getter and setter interfaces.
        Hide
        Christophe Taton added a comment -

        My previous question is actually irrelevant at this point. Is there documentation or a plan sketched as to how to bring immutable records?
        What's the timeline to remove setters?

        Show
        Christophe Taton added a comment - My previous question is actually irrelevant at this point. Is there documentation or a plan sketched as to how to bring immutable records? What's the timeline to remove setters?
        Hide
        Christophe Taton added a comment -

        We can disable the put() here (ie. let it throw a runtime exception).
        Should the same treatment happen with IndexedRecord.put() implemented by SpecificRecordBase then?

        Show
        Christophe Taton added a comment - We can disable the put() here (ie. let it throw a runtime exception). Should the same treatment happen with IndexedRecord.put() implemented by SpecificRecordBase then?
        Hide
        Scott Carey added a comment -

        I'm uncomfortable with the mutator here. (put) This may make immutable records impossible in the future. Currently the only mutators are generated from the template, can these be optional from the compile template as well? Or can we break up the generic record interface into read/write bits and only expose the accessor by default?

        Show
        Scott Carey added a comment - I'm uncomfortable with the mutator here. (put) This may make immutable records impossible in the future. Currently the only mutators are generated from the template, can these be optional from the compile template as well? Or can we break up the generic record interface into read/write bits and only expose the accessor by default?
        Hide
        Doug Cutting added a comment -

        +1 This looks good to me. I'll commit it soon unless there are objections.

        Show
        Doug Cutting added a comment - +1 This looks good to me. I'll commit it soon unless there are objections.
        Hide
        Christophe Taton added a comment -

        Sorry for that, here is an updated diff against the latest trunk.

        Show
        Christophe Taton added a comment - Sorry for that, here is an updated diff against the latest trunk.
        Hide
        Doug Cutting added a comment -

        Patch looks good but also includes changes for AVRO-1295.

        Show
        Doug Cutting added a comment - Patch looks good but also includes changes for AVRO-1295 .
        Hide
        Christophe Taton added a comment -

        This patch updates SpecificRecordBase to implement GenericRecord.

        Show
        Christophe Taton added a comment - This patch updates SpecificRecordBase to implement GenericRecord.

          People

          • Assignee:
            Christophe Taton
            Reporter:
            Christophe Taton
          • Votes:
            0 Vote for this issue
            Watchers:
            4 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development