[jira] Commented: (AVRO-237) improvements to reflection

["Philip Zeyliger (JIRA)" <ji...@apache.org>]

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

Philip Zeyliger commented on AVRO-237:
--------------------------------------

Makes sense.  Standard nit-picking below, but the patch itself was clear.

ReflectData.java:

> Both Java arrays and non-{@link Map}

Map isn't actually a Collection in Java.  I think you don't need to special-case it out.  (It's confusing, because Map.entrySet() is, and Map is certainly talked about as part of the Collections API.)

You modified findField to look up at super classes,
but the javadoc says "only fields of the direct class, not it's
super classes" are used.  Is that still true?  I think
you may have modified the javadoc in one sentence, but not
the other.

getFields() is complicated enough that it deserves 
one line of javadoc explaining what it's doing.

As I read this, one thing that might be nice is to
comment which of these methods are called frequently
at runtime, and which only at schema management time.
So, for example, you do nothing to cache previous
results in getFields(), whereas you do a lot 
for the similarly named getField().  I think the 
distinction is that the latter is intended to 
be called more often; it may be helpful to future
users that this distinction is called out explicitly.

BTW, how do you handle?
{quote}
  class A {
    int foo;
  }
  
  class B extends A {
    double foo;
  }
{quote}
It's poor programming on the part of A and B, but it would be nice to at least error
out in that edge case.

As far as testing, I don't think you've added a test case
where a non-list Collection (or instance thereof) is used.
I think you could meaningfully test getField() and getFields()
on their own, though the implicit check is probably good enough.
I noticed you added Short.class to SpecificData; is there 
a test case that that should be part of?


> improvements to reflection
> --------------------------
>
>                 Key: AVRO-237
>                 URL: https://issues.apache.org/jira/browse/AVRO-237
>             Project: Avro
>          Issue Type: Improvement
>          Components: java
>            Reporter: Doug Cutting
>            Assignee: Doug Cutting
>             Fix For: 1.3.0
>
>         Attachments: AVRO-237.patch
>
>
> A few improvements to reflection to better match actual Java usage:
>  - instead of just List implementations, any Collection that's not a Map should be mapped to an Avro array
>  - inherited fields should be included in schemas induced from a class
>  - inherited methods should be included in protocols induced from an interface

-- 
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.