Avro
  1. Avro
  2. AVRO-283

Change the new Python implementation from dictionaries to objects

    Details

    • Type: Improvement Improvement
    • Status: Open
    • Priority: Major Major
    • Resolution: Unresolved
    • Affects Version/s: None
    • Fix Version/s: None
    • Component/s: python
    • Labels:
      None
    1. AVRO-283_generic.patch
      4 kB
      Marcio Silva
    2. AVRO-283.patch
      6 kB
      Douglas Kaminsky

      Issue Links

        Activity

        Hide
        Philip Zeyliger added a comment -

        Just to be clear, I don't believe your intention here is to generate code for python (a la Java's specific implementation.)

        Show
        Philip Zeyliger added a comment - Just to be clear, I don't believe your intention here is to generate code for python (a la Java's specific implementation.)
        Hide
        Doug Cutting added a comment -

        I assume the intent here is to make instance access look like a.foo rather than a['foo']. Is that right?

        Show
        Doug Cutting added a comment - I assume the intent here is to make instance access look like a.foo rather than a ['foo'] . Is that right?
        Hide
        Jeff Hammerbacher added a comment -

        Yep, sorry for the misleading name. I'll change it.

        Show
        Jeff Hammerbacher added a comment - Yep, sorry for the misleading name. I'll change it.
        Hide
        Douglas Kaminsky added a comment -

        From AVRO-973 - This would also resolve long-standing misimplementation of union branch selection in the DatumWriter

        Please see attached patch (originally attached to AVRO-973) that presents a lightweight solution to the branch selection issue with a slight reworking of the DatumWriter.

        This has been open for awhile – do you think we could get some traction on this?

        Show
        Douglas Kaminsky added a comment - From AVRO-973 - This would also resolve long-standing misimplementation of union branch selection in the DatumWriter Please see attached patch (originally attached to AVRO-973 ) that presents a lightweight solution to the branch selection issue with a slight reworking of the DatumWriter. This has been open for awhile – do you think we could get some traction on this?
        Hide
        Douglas Kaminsky added a comment -

        Also please note in the attached file that the _setattr_ definition is wrong and will recursively destroy you, please replace it if you actually gank any code from the patch.

        Show
        Douglas Kaminsky added a comment - Also please note in the attached file that the _ setattr _ definition is wrong and will recursively destroy you, please replace it if you actually gank any code from the patch.
        Hide
        Marcio Silva added a comment -

        Initial attempt at a backwards-compatible generic record class. Created classes for all the named types that extend their original type as well as a GenericContainer.

        Show
        Marcio Silva added a comment - Initial attempt at a backwards-compatible generic record class. Created classes for all the named types that extend their original type as well as a GenericContainer.
        Hide
        Marcio Silva added a comment -

        After following the discussion in AVRO-1022, I decided to hold off on my original plan for creating specific records using meta-classes until a consensus developed about widening names beyond the existing spec.

        I think the remaining issues with named records in AVRO-973 can be resolved with the the addition of a schema accessor, so that's what I've implemented. I tried to preserve backwards compatability with the existing DatumReader implementation (though the addition of field name validation to _setitem_ in the GenericRecord could be an issue for users that added other properties to datums previously read).

        Show
        Marcio Silva added a comment - After following the discussion in AVRO-1022 , I decided to hold off on my original plan for creating specific records using meta-classes until a consensus developed about widening names beyond the existing spec. I think the remaining issues with named records in AVRO-973 can be resolved with the the addition of a schema accessor, so that's what I've implemented. I tried to preserve backwards compatability with the existing DatumReader implementation (though the addition of field name validation to _ setitem _ in the GenericRecord could be an issue for users that added other properties to datums previously read).
        Hide
        Miki Tebeka added a comment -

        AVRO-823.patch is not a patch file. Can you generate a patch with running "svn diff"?
        I like the idea, but I see many problems with the code in AVRO-823.patch

        • There are several missing imports/functions (such as datautil, getSchemaEntity)
        • Never use default mutable arguments (json={})
        • It's probably better to inherit UserDict
        • I don't think you should allow access to the internal _json and friends
        • See PEP8 on how to name variables, methods ...
        • In line 114 you do if index_of_schema < 0, however in line 108 it's a list
        Show
        Miki Tebeka added a comment - AVRO-823 .patch is not a patch file. Can you generate a patch with running "svn diff"? I like the idea, but I see many problems with the code in AVRO-823 .patch There are several missing imports/functions (such as datautil, getSchemaEntity) Never use default mutable arguments (json={}) It's probably better to inherit UserDict I don't think you should allow access to the internal _json and friends See PEP8 on how to name variables, methods ... In line 114 you do if index_of_schema < 0 , however in line 108 it's a list
        Hide
        Marcio Silva added a comment -

        Mike, Can you also take a look at AVRO-283_generic.patch

        Show
        Marcio Silva added a comment - Mike, Can you also take a look at AVRO-283 _generic.patch
        Hide
        Douglas Kaminsky added a comment -

        @Miki - that was a really quick first pass based on some proprietary code we use at my workplace - if you're interested there is a much more fully fleshed out version that addresses a lot of those issues

        Show
        Douglas Kaminsky added a comment - @Miki - that was a really quick first pass based on some proprietary code we use at my workplace - if you're interested there is a much more fully fleshed out version that addresses a lot of those issues
        Hide
        Douglas Kaminsky added a comment -

        And btw...

        • I know what a patch is, this was not intended to be one, so the real mistake was naming it ".patch"
        • Feel free to reformat anything I submit to PEP8 conventions but I need to follow the conventions in place in my organization - I think I'm mostly compliant, minus naming
        Show
        Douglas Kaminsky added a comment - And btw... I know what a patch is, this was not intended to be one, so the real mistake was naming it ".patch" Feel free to reformat anything I submit to PEP8 conventions but I need to follow the conventions in place in my organization - I think I'm mostly compliant, minus naming

          People

          • Assignee:
            Jeff Hammerbacher
            Reporter:
            Jeff Hammerbacher
          • Votes:
            3 Vote for this issue
            Watchers:
            6 Start watching this issue

            Dates

            • Created:
              Updated:

              Development