Avro
  1. Avro
  2. AVRO-1443

SpecificRecord builders should share more functionality with GenericRecord builders

    Details

    • Type: Bug Bug
    • Status: Patch Available
    • Priority: Major Major
    • Resolution: Unresolved
    • Affects Version/s: 1.7.5, 1.7.6
    • Fix Version/s: 1.8.0
    • Component/s: java
    • Labels:
      None

      Description

      Ideally, wherever a generic record builder is expected, one should be able to supply a builder for a specific record.

      That could happen by making SpecificRecordBuilderBase (and SpecificErrorBuilderBase?) a subclass of GenericRecordBuilder.

      Since SpecificRecordBase implements GenericRecord, it should be possible with minimal effort.

      1. AVRO-1443.20140204-124549-0800.diff
        44 kB
        Christophe Taton
      2. AVRO-1443.20140202-182611-0800.svndiff
        82 kB
        Christophe Taton
      3. AVRO-1443.20140126-182111-0800.diff
        91 kB
        Christophe Taton

        Activity

        Hide
        Christophe Taton added a comment -

        With this change:

        • All builders (generic and specific) extend generic records
        • Specific record builders do NOT extend GenericRecordBuilder,
          but implement the generic record builders interface.
        • Introduce interfaces for indexed/generic record builders.
        • Introduce interfaces for immutable indexed/generic records.
        • Update velocity template to add the new methods required for builders, with some formatting tweak to make the generated easier to read.
        • Deleted FooBarSpecificRecord (apparently unused, no schema definition available).

        Notes:
        It turns out that making SpecificRecordBuilderBase a subclass of GenericRecordBuilder is fairly impractical.
        Instead I added pure interfaces for generic record builders.
        However, I am not happy with the name of these interface (IGenericRecordBuilder and IIndexedRecordBuilder).

        Tests are passing.

        Show
        Christophe Taton added a comment - With this change: All builders (generic and specific) extend generic records Specific record builders do NOT extend GenericRecordBuilder, but implement the generic record builders interface. Introduce interfaces for indexed/generic record builders. Introduce interfaces for immutable indexed/generic records. Update velocity template to add the new methods required for builders, with some formatting tweak to make the generated easier to read. Deleted FooBarSpecificRecord (apparently unused, no schema definition available). Notes: It turns out that making SpecificRecordBuilderBase a subclass of GenericRecordBuilder is fairly impractical. Instead I added pure interfaces for generic record builders. However, I am not happy with the name of these interface (IGenericRecordBuilder and IIndexedRecordBuilder). Tests are passing.
        Hide
        Christophe Taton added a comment -

        Just wondering, is it possible to have Avro patches on https://reviews.apache.org/ for review?

        Show
        Christophe Taton added a comment - Just wondering, is it possible to have Avro patches on https://reviews.apache.org/ for review?
        Hide
        Christophe Taton added a comment -

        I finally managed to create a review on https://reviews.apache.org/r/17549/
        Any feedback is welcome!

        Show
        Christophe Taton added a comment - I finally managed to create a review on https://reviews.apache.org/r/17549/ Any feedback is welcome!
        Hide
        Christophe Taton added a comment -

        New patch includes renamed interface:

        • IndexRecordBuilderInterface
        • GenericRecordBuilderInterface
        Show
        Christophe Taton added a comment - New patch includes renamed interface: IndexRecordBuilderInterface GenericRecordBuilderInterface
        Hide
        Doug Cutting added a comment -

        This patch has a lot of cosmetic changes, many in code that's otherwise not changed. It removes end-of-line spaces, changes the order of imports, reformats code, adds redundant @inheritDoc tags, adds section-heading comments, etc. These might all be fine changes, but they make it much more difficult for me to review the patch's functional changes, since I cannot focus on them. I much prefer cosmetic changes as separate patches. If an IDE is automatically making some of these changes for you then I encourage you to reconfigure the IDE.

        It appears that this change is incompatible since it will require re-compilation rather than simply a jar update. Is that right? If so, then we should include it in 1.8.0, not 1.7.7.

        Show
        Doug Cutting added a comment - This patch has a lot of cosmetic changes, many in code that's otherwise not changed. It removes end-of-line spaces, changes the order of imports, reformats code, adds redundant @inheritDoc tags, adds section-heading comments, etc. These might all be fine changes, but they make it much more difficult for me to review the patch's functional changes, since I cannot focus on them. I much prefer cosmetic changes as separate patches. If an IDE is automatically making some of these changes for you then I encourage you to reconfigure the IDE. It appears that this change is incompatible since it will require re-compilation rather than simply a jar update. Is that right? If so, then we should include it in 1.8.0, not 1.7.7.
        Hide
        Christophe Taton added a comment -

        Hey Doug,
        Sorry for the cosmetic changes, here is a new patch stripped to the bare minimum.
        Now that I think about it, yes, that will actually require users to regenerate their specific class definitions and recompile these, so that's a 1.8.0 change.

        Is it OK for me to send a separate change with cosmetic changes only?

        Show
        Christophe Taton added a comment - Hey Doug, Sorry for the cosmetic changes, here is a new patch stripped to the bare minimum. Now that I think about it, yes, that will actually require users to regenerate their specific class definitions and recompile these, so that's a 1.8.0 change. Is it OK for me to send a separate change with cosmetic changes only?
        Hide
        Doug Cutting added a comment -

        A separate change with cosmetic changes would be better. Thanks!

        Does /**

        {@inheritDoc}

        */ actually change anything? The javadoc is already inherited, no?

        I'll try to make a more detailed review ASAP.

        Show
        Doug Cutting added a comment - A separate change with cosmetic changes would be better. Thanks! Does /** {@inheritDoc} */ actually change anything? The javadoc is already inherited, no? I'll try to make a more detailed review ASAP.
        Hide
        Christophe Taton added a comment -

        I realize I may need to add a method to the builder interface, since the build() method may have a different return type:

          GenericRecord buildGenericRecord();
        

        Does that seem reasonable?

        Also, after checking, you're correct, the inheritDoc directory is useless if you're not adding to the inherited documentation - I'll remove these.

        Show
        Christophe Taton added a comment - I realize I may need to add a method to the builder interface, since the build() method may have a different return type: GenericRecord buildGenericRecord(); Does that seem reasonable? Also, after checking, you're correct, the inheritDoc directory is useless if you're not adding to the inherited documentation - I'll remove these.
        Hide
        Doug Cutting added a comment -

        Re buildGenericRecord: what is the use case? More generally, I'm forgetting the overall use case here. The description above says, "wherever a generic record builder is expected, one should be able to supply a builder for a specific record". When would someone want to pass a specific builder where a generic builder is expected?

        Show
        Doug Cutting added a comment - Re buildGenericRecord: what is the use case? More generally, I'm forgetting the overall use case here. The description above says, "wherever a generic record builder is expected, one should be able to supply a builder for a specific record". When would someone want to pass a specific builder where a generic builder is expected?
        Hide
        Christophe Taton added a comment -

        Without this change, there is no way for an Avro user to set fields generically through a builder.

        There are several use-cases for this.
        The one I'm having right now is a library function that populates some record's fields through its builder.
        The callers of the library should ideally be able to use whichever kind of records they need indifferently.

        Show
        Christophe Taton added a comment - Without this change, there is no way for an Avro user to set fields generically through a builder. There are several use-cases for this. The one I'm having right now is a library function that populates some record's fields through its builder. The callers of the library should ideally be able to use whichever kind of records they need indifferently.

          People

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

            Dates

            • Created:
              Updated:

              Development