You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@avro.apache.org by "Doug Cutting (JIRA)" <ji...@apache.org> on 2009/11/24 23:25:39 UTC
[jira] Created: (AVRO-237) improvements to reflection
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
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.
[jira] Commented: (AVRO-237) improvements to reflection
Posted by "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.
[jira] Updated: (AVRO-237) improvements to reflection
Posted by "Doug Cutting (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/AVRO-237?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Doug Cutting updated AVRO-237:
------------------------------
Attachment: AVRO-237.patch
> Map isn't actually a Collection in Java. I think you don't need to special-case it out.
Good catch. Fixed.
> but the javadoc says "only fields of the direct class..
Javadoc fixed.
> getFields() is complicated enough that it deserves one line of javadoc
I added javadoc for this and comments explaining why a cache is used in one case and not the other.
> it would be nice to at least error out in that edge case.
Good point. This now throws an exception.
> I don't think you've added a test case where a non-list Collection (or instance thereof) is used.
Good point. I've now added one.
> I noticed you added Short.class to SpecificData; is there a test case that that should be part of?
I added a test case for this.
> 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, 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.
[jira] Commented: (AVRO-237) improvements to reflection
Posted by "Philip Zeyliger (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/AVRO-237?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12782592#action_12782592 ]
Philip Zeyliger commented on AVRO-237:
--------------------------------------
Looks good! +1
> 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, 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.
[jira] Updated: (AVRO-237) improvements to reflection
Posted by "Doug Cutting (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/AVRO-237?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Doug Cutting updated AVRO-237:
------------------------------
Resolution: Fixed
Hadoop Flags: [Incompatible change, Reviewed] (was: [Reviewed])
Status: Resolved (was: Patch Available)
I just committed this. Thanks for the reviews, Philip!
> 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, 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.
[jira] Updated: (AVRO-237) improvements to reflection
Posted by "Doug Cutting (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/AVRO-237?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Doug Cutting updated AVRO-237:
------------------------------
Attachment: AVRO-237.patch
Here's a patch that addresses these issues.
> 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.
[jira] Updated: (AVRO-237) improvements to reflection
Posted by "Doug Cutting (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/AVRO-237?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Doug Cutting updated AVRO-237:
------------------------------
Hadoop Flags: [Reviewed]
Status: Patch Available (was: Open)
> 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, 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.