You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@parquet.apache.org by "RustedBones (via GitHub)" <gi...@apache.org> on 2024/03/08 11:56:34 UTC
[PR] [PARQUET-2448] fix model conversions for avro <= generated classes [parquet-mr]
RustedBones opened a new pull request, #1296:
URL: https://github.com/apache/parquet-mr/pull/1296
When running with avro <= 1.8, the default data supplier was looking for the root record's class conversions to initialize its model.
In case the record contained a nested class with another logical-type, the conversion would fails.
Adapt the code to inspect the whole schema and initialize the model accordingly.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@parquet.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@parquet.apache.org
For additional commands, e-mail: issues-help@parquet.apache.org
Re: [PR] PARQUET-2448: Fix model conversions for avro <= 1.8 generated classes [parquet-mr]
Posted by "RustedBones (via GitHub)" <gi...@apache.org>.
RustedBones commented on code in PR #1296:
URL: https://github.com/apache/parquet-mr/pull/1296#discussion_r1528544433
##########
parquet-avro/src/test/java/org/apache/parquet/avro/TestAvroRecordConverter.java:
##########
Review Comment:
I can confirm that the original tests are passing with the changes
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@parquet.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@parquet.apache.org
For additional commands, e-mail: issues-help@parquet.apache.org
Re: [PR] PARQUET-2448: Fix model conversions for avro <= 1.8 generated classes [parquet-mr]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac merged PR #1296:
URL: https://github.com/apache/parquet-mr/pull/1296
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@parquet.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@parquet.apache.org
For additional commands, e-mail: issues-help@parquet.apache.org
Re: [PR] PARQUET-2448: Fix model conversions for avro <= 1.8 generated classes [parquet-mr]
Posted by "gszadovszky (via GitHub)" <gi...@apache.org>.
gszadovszky commented on code in PR #1296:
URL: https://github.com/apache/parquet-mr/pull/1296#discussion_r1525839145
##########
doc/dremel_paper/schema.png:
##########
Review Comment:
Same as above
##########
parquet-avro/src/test/java/org/apache/parquet/avro/TestAvroRecordConverter.java:
##########
Review Comment:
It seems you have rewritten the whole unit test. Was it necessary to do so? I would feel more comfortable if the existing tests would pass as they were.
##########
parquet-avro/src/main/java/org/apache/parquet/avro/AvroRecordConverter.java:
##########
@@ -215,31 +263,7 @@ static SpecificData getModelForSchema(Schema schema) {
final String avroVersion = getRuntimeAvroVersion();
// Avro 1.7 and 1.8 don't include conversions in the MODEL$ field by default
if (avroVersion != null && (avroVersion.startsWith("1.8.") || avroVersion.startsWith("1.7."))) {
- final Field conversionsField;
- try {
- conversionsField = clazz.getDeclaredField("conversions");
- } catch (NoSuchFieldException e) {
- // Avro classes without logical types (denoted by the "conversions" field) can be returned as-is
- return model;
- }
-
- final Conversion<?>[] conversions;
- try {
- conversionsField.setAccessible(true);
- conversions = (Conversion<?>[]) conversionsField.get(null);
- } catch (IllegalAccessException e) {
- LOG.warn(String.format(
- "Field `conversions` in class %s was inaccessible. Parquet will use default "
- + "SpecificData model for reading and writing.",
- clazz));
- return null;
Review Comment:
At this point we were returning a `null` as the model. The new code does not seem to do so. Is it intentional and correct?
##########
doc/dremel_paper/dremel_example.png:
##########
Review Comment:
Something went odd with the formatting. It shouldn't touch binary files.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@parquet.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@parquet.apache.org
For additional commands, e-mail: issues-help@parquet.apache.org
Re: [PR] PARQUET-2448: Fix model conversions for avro <= 1.8 generated classes [parquet-mr]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on code in PR #1296:
URL: https://github.com/apache/parquet-mr/pull/1296#discussion_r1525687972
##########
doc/dremel_paper/schema.png:
##########
Review Comment:
ditto
##########
doc/dremel_paper/dremel_example.png:
##########
Review Comment:
Could you please revert this accidental change?
##########
parquet-avro/src/test/java/org/apache/parquet/avro/TestAvroRecordConverter.java:
##########
@@ -18,18 +18,18 @@
*/
package org.apache.parquet.avro;
-import static org.junit.Assert.assertEquals;
-import static org.junit.Assert.assertNull;
-import static org.junit.Assert.assertTrue;
+import static org.junit.Assert.*;
Review Comment:
Could you please avoid wildcard import?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@parquet.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@parquet.apache.org
For additional commands, e-mail: issues-help@parquet.apache.org
Re: [PR] PARQUET-2448: Fix model conversions for avro <= 1.8 generated classes [parquet-mr]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on PR #1296:
URL: https://github.com/apache/parquet-mr/pull/1296#issuecomment-1998838287
I need to spend some time to get familiar with the code path.
cc avro experts @gszadovszky @Fokko .
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@parquet.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@parquet.apache.org
For additional commands, e-mail: issues-help@parquet.apache.org
Re: [PR] PARQUET-2448: Fix model conversions for avro <= 1.8 generated classes [parquet-mr]
Posted by "RustedBones (via GitHub)" <gi...@apache.org>.
RustedBones commented on code in PR #1296:
URL: https://github.com/apache/parquet-mr/pull/1296#discussion_r1528395526
##########
parquet-avro/src/test/java/org/apache/parquet/avro/TestAvroRecordConverter.java:
##########
Review Comment:
Previous tests did not covered nested classes. I started to adapt the original code but that lead creating many classes with code duplication.
This refactoring went with the following simplification:
- test valid avro with generated class
- test deprecated avro (1.7 and 1.8) with same pseudo generated code
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@parquet.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@parquet.apache.org
For additional commands, e-mail: issues-help@parquet.apache.org
Re: [PR] PARQUET-2448: Fix model conversions for avro <= 1.8 generated classes [parquet-mr]
Posted by "gszadovszky (via GitHub)" <gi...@apache.org>.
gszadovszky commented on code in PR #1296:
URL: https://github.com/apache/parquet-mr/pull/1296#discussion_r1528404720
##########
parquet-avro/src/test/java/org/apache/parquet/avro/TestAvroRecordConverter.java:
##########
Review Comment:
OK, I accept this.
To be on the safe side, could you revert the unit test changes on your env and execute it on the new code?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@parquet.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@parquet.apache.org
For additional commands, e-mail: issues-help@parquet.apache.org
Re: [PR] PARQUET-2448: Fix model conversions for avro <= 1.8 generated classes [parquet-mr]
Posted by "gszadovszky (via GitHub)" <gi...@apache.org>.
gszadovszky commented on code in PR #1296:
URL: https://github.com/apache/parquet-mr/pull/1296#discussion_r1528568307
##########
parquet-avro/src/test/java/org/apache/parquet/avro/TestAvroRecordConverter.java:
##########
Review Comment:
Thank you @RustedBones!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@parquet.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@parquet.apache.org
For additional commands, e-mail: issues-help@parquet.apache.org
Re: [PR] PARQUET-2448: Fix model conversions for avro <= 1.8 generated classes [parquet-mr]
Posted by "RustedBones (via GitHub)" <gi...@apache.org>.
RustedBones commented on code in PR #1296:
URL: https://github.com/apache/parquet-mr/pull/1296#discussion_r1528385474
##########
parquet-avro/src/main/java/org/apache/parquet/avro/AvroRecordConverter.java:
##########
@@ -215,31 +263,7 @@ static SpecificData getModelForSchema(Schema schema) {
final String avroVersion = getRuntimeAvroVersion();
// Avro 1.7 and 1.8 don't include conversions in the MODEL$ field by default
if (avroVersion != null && (avroVersion.startsWith("1.8.") || avroVersion.startsWith("1.7."))) {
- final Field conversionsField;
- try {
- conversionsField = clazz.getDeclaredField("conversions");
- } catch (NoSuchFieldException e) {
- // Avro classes without logical types (denoted by the "conversions" field) can be returned as-is
- return model;
- }
-
- final Conversion<?>[] conversions;
- try {
- conversionsField.setAccessible(true);
- conversions = (Conversion<?>[]) conversionsField.get(null);
- } catch (IllegalAccessException e) {
- LOG.warn(String.format(
- "Field `conversions` in class %s was inaccessible. Parquet will use default "
- + "SpecificData model for reading and writing.",
- clazz));
- return null;
Review Comment:
Good catch. Update the PR so the previous behavior is kept
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@parquet.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@parquet.apache.org
For additional commands, e-mail: issues-help@parquet.apache.org
Re: [PR] PARQUET-2448: Fix model conversions for avro <= 1.8 generated classes [parquet-mr]
Posted by "RustedBones (via GitHub)" <gi...@apache.org>.
RustedBones commented on code in PR #1296:
URL: https://github.com/apache/parquet-mr/pull/1296#discussion_r1528395526
##########
parquet-avro/src/test/java/org/apache/parquet/avro/TestAvroRecordConverter.java:
##########
Review Comment:
Previous tests did not covered nested classes. I started to adapt the original code but that lead creating many classes with code duplication.
This refactoring went with the following simplification:
- test valid avro with generated class
- test deprecated avro (1.7 and 1.8 with same pseudo generated code)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@parquet.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@parquet.apache.org
For additional commands, e-mail: issues-help@parquet.apache.org
Re: [PR] [PARQUET-2448] fix model conversions for avro <= 1.8 generated classes [parquet-mr]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on PR #1296:
URL: https://github.com/apache/parquet-mr/pull/1296#issuecomment-1988022524
Thanks for the fix! Could you please make the CI happy?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@parquet.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@parquet.apache.org
For additional commands, e-mail: issues-help@parquet.apache.org