Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

AVRO-2415 - Union resolver regression when dealing with arrays #537

Merged
merged 5 commits into from Jun 17, 2019
Merged

AVRO-2415 - Union resolver regression when dealing with arrays #537

merged 5 commits into from Jun 17, 2019

Conversation

unchuckable
Copy link
Contributor

@unchuckable unchuckable commented Jun 6, 2019

Fix error with resolution with

writer = array
reader = union { null, array }

Resolution works fine in 1.8.2 and got broken with the new Resolver code.

https://issues.apache.org/jira/browse/AVRO-2415

@probot-autolabeler probot-autolabeler bot added the Java Pull Requests for Java binding label Jun 6, 2019
@jacobtolar
Copy link
Contributor

This looks like a duplicate of AVRO-2400 which I have a pull request for here: #526

I actually suggested the same fix you made here. See the discussion in the JIRA. https://issues.apache.org/jira/browse/AVRO-2400

@unchuckable
Copy link
Contributor Author

@jacobtolar - thanks for the feedback. At first, I thought so, too, (hence, I at first added a comment to your ticket, but then moved it to another, since it's really another issue). Please do take the time and look through the fixes, they're in fact different (you effectively change name lookup in Resolver, I change evaluation order in ResolvingGrammarGenerator). Also, do look up the test snippet in

https://issues.apache.org/jira/projects/AVRO/issues/AVRO-2415

and you will find that your PR does nothing to fix it, because it really addresses a different issue.

Sad truth to be told, there seems to be more than one thing amiss with the current resolver implementation, and I'm very sorry we didn't catch that sooner.

@unchuckable unchuckable changed the title AVRO-2415 - Union resolver regression AVRO-2415 - Union resolver regression when dealing with arrays Jun 8, 2019
@nandorKollar nandorKollar mentioned this pull request Jun 13, 2019
@@ -77,6 +77,8 @@
new ReaderWriter(BYTES_UNION_SCHEMA, STRING_UNION_SCHEMA),
new ReaderWriter(DOUBLE_UNION_SCHEMA, INT_FLOAT_UNION_SCHEMA),

new ReaderWriter(NULL_INT_ARRAY_UNION_SCHEMA, INT_ARRAY_SCHEMA),

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should add a test for union {null, map} since this fixes dealing with that case as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@jacobtolar
Copy link
Contributor

and you will find that your PR does nothing to fix it, because it really addresses a different issue.

Yeah, you're right, it's broken in multiple ways. My original suggestion was this patch (which also fixes the problem in AVRO-2400). Now that PR has morphed a little into "make spec and implementation(s) consistent".

Conflicts:
	lang/java/avro/src/main/java/org/apache/avro/io/parsing/ResolvingGrammarGenerator.java
@unchuckable
Copy link
Contributor Author

Problem persists after merge #526 (AVRO-2400). Added test case to cover map unions (AVRO-2018, #543)

@nandorKollar nandorKollar merged commit f6417e8 into apache:master Jun 17, 2019
nandorKollar pushed a commit that referenced this pull request Jun 17, 2019
@unchuckable unchuckable deleted the union-resolver branch June 17, 2019 14:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Java Pull Requests for Java binding
Projects
None yet
5 participants