avro-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "James Baldassari (JIRA)" <j...@apache.org>
Subject [jira] [Commented] (AVRO-839) Implement builder pattern in generated record classes that sets default values when omitted
Date Mon, 22 Aug 2011 20:10:37 GMT

    [ https://issues.apache.org/jira/browse/AVRO-839?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13088974#comment-13088974
] 

James Baldassari commented on AVRO-839:
---------------------------------------

Sorry for the delay again.  I'll try to find time to work on this more regularly.  Thanks
for the comments.

bq. Why does SpecificExceptionBase include serialVersionUID?
Probably just to get Eclipse to stop complaining.  SpecificExceptinoBase isA Throwable, which
is serializable, so apparently the "right thing" to do is to declare the magic serialVersionUID
field.  However, it's not strictly required, so I'll remove it if you'd prefer.

bq. The first line of new SpecificFixed constructor can just be 'this();', no?
Yes.  I'll change it.

bq. GenericData#deepCopy() should probably not be static.
OK, I didn't realize that GenericData was a singleton, so I'll make deepCopy() non-static
and then invoke it via the singleton instance.

bq. We might instead refactor GenericDatumReader's newRecord() and createFixed() to GenericData
so they can be used by deepCopy(). Currently I believe deepCopy is broken for SpecificFixed.
It also adds a dependency by generic on specific, when otherwise specific currently layers
on generic.
Comparing GenericData#deepCopy() and GenericDatumReader#createFixed(), it looks to me like
they're doing similar things for Fixed values.  Could you elaborate on why you think it's
broken in GenericData#deepCopy()?  Even if it does work in deepCopy(), it would probably be
a good idea to move createFixed() over to GenericData just to prevent code duplication.

As for the new specific dependency in GenericData, I agree that it's not an ideal situation.
 This dependency is coming from the code that handles deep copies of record instances.  This
code could be changed to simply return new GenericData.Record(schema) for all records.  It
just seemed to me that performing a deep copy on a specific record should return the same
specific record type rather than a GenericData.Record.  What about adding a newInstance()
method to IndexedRecord (or GenericContainer)?  That way generic records could return GenericData.Record
instances, and specific records could return instances of the generated type.

bq. RecordBuilderBase#isValidValue's logic for checking for a NULL in a union is incorrect.
Schema#getTypes() returns List<Schema>, not List<Schema.Type>. We should perhaps
add a test of a union with null for this. Also, I don't think this method needs to be static.

Good catch!  I think I've fixed this problem, but I will add a unit test for this method as
you suggested.

> Implement builder pattern in generated record classes that sets default values when omitted
> -------------------------------------------------------------------------------------------
>
>                 Key: AVRO-839
>                 URL: https://issues.apache.org/jira/browse/AVRO-839
>             Project: Avro
>          Issue Type: Improvement
>          Components: java
>            Reporter: James Baldassari
>            Assignee: James Baldassari
>         Attachments: AVRO-839-v2.patch, AVRO-839-v3.patch, AVRO-839.patch
>
>
> This is an idea for an improvement to the SpecificCompiler-generated record classes.
 There are two main issues to address:
> # Default values specified in schemas are only used at read time, not when writing/serializing
records.  For example, a NullPointerException is thrown when attempting to write a record
that has an uninitialized array or string type.  I'm sure this was done for good reasons,
like giving users maximum control and preventing unnecessary garbage collection, but I think
it's also somewhat confusing and unintuitive for new users (myself included).
> # Users have to create their own factory classes/methods for every record type, both
to ensure that all non-primitive members are initialized and to facilitate the construction
and initialization of record instances (i.e. constructing and setting values in a single statement).
> These issues have been discussed previously here:
> * [http://search-hadoop.com/m/iDVTn1JVeSR1]
> * AVRO-726
> * AVRO-770
> * [http://search-hadoop.com/m/JuY1V16pwxh1]
> I'd like to propose a solution that is used by at least one other messaging framework.
 For each generated record class there will be a public static inner class called Builder.
 The Builder inner class has the same fields as the record class, as well as accessors and
mutators for each of these fields.  Whenever a mutator method is called, the Builder sets
a boolean flag indicating that the field has been set.  All mutators return a reference to
'this', so it's possible to chain a series of setter invocations, which makes it really easy
to construct records in a single statement.  The Builder also has a build() method which constructs
a record instance using the values that were set in the Builder.  When the build() method
is invoked, if there are any fields that have not been set but have default values as defined
in the schema, the Builder will set the values of these fields using their defaults.
> One nice thing about implementing the builder pattern in a static inner Builder class
rather than in the record itself is that this enhancement will be completely backwards-compatible
with existing code.  The record class itself would not change, and the public fields would
still be there, so existing code would still work.  Users would have the option to use the
Builder or continue constructing records manually.  Eventually the public fields could be
phased out, and the record would be made immutable.  All changes would have to be done through
the Builder.
> Here is an example of what this might look like:
> {code}
> // Person.newBuilder() returns a new Person.Builder instance
> // All Person.Builder setters return 'this' allowing us to chain set calls together for
convenience
> // Person.Builder.build() returns a Person instance after setting any uninitialized values
that have defaults
> Person me = Person.newBuilder().setName("James").setCountry("US").setState("MA").build();
> // We still have direct access to Person's members, so the records are backwards-compatible
> me.state = "CA";
> // Person has accessor methods now so that the public fields can be phased out later
> System.out.println(me.getState());
> // No NPE here because the array<Person> field that stores this person's friends
has been automatically 
> // initialized by the Builder to a new java.util.ArrayList<Person> due to a @java_class
annotation in the IDL
> System.out.println(me.getFriends().size());
> {code}
> What do people think about this approach?  Any other ideas?

--
This message is automatically generated by JIRA.
For more information on JIRA, see: http://www.atlassian.com/software/jira

        

Mime
View raw message