GH-2992: Gate LocalTimestamp references in AvroSchemaConverter #2993
Conversation
|
cc @RustedBones , can you take a look at this fix as well? |
clairemcginty
force-pushed
the
parquet-2992
branch
from
7c2513d
to
bd3c805
Compare
| @@ -605,4 +611,14 @@ private static String namespace(String name, Map<String, Integer> names) { | |||
| Integer nameCount = names.merge(name, 1, (oldValue, value) -> oldValue + 1); | |||
| return nameCount > 1 ? name + nameCount : null; | |||
| } | |||
|
|
|||
| /* Avro <= 1.9 does not support conversions to LocalTimestamp{Micros, Millis} classes */ | |||
| private static boolean avroVersionSupportsLocalTimestampTypes() { | |||
There was a problem hiding this comment.
What do you think of mocking this method to capture the behavior of supporting and not-supporting local timestamps.
There was a problem hiding this comment.
I'll give it a try with Powermock!
I might start a thread on the mailing list this week discussing Avro version support more broadly, to formalize which Avro versions are currently supported/any planned EOL dates? I've been thinking it would also be good to have some automated testing of parquet-avro on different Avro versions... it would just be a little complicated to set up since we'd need either a separate Maven module per supported Avro version, or some very clever classloading :)
There was a problem hiding this comment.
Hey @clairemcginty, that's a good idea!
What do we consider the lower-bound? :D Avro 1.8.2 is May 2017, so that's pretty old, but I'm aware of some breaking API changes because we exposed some Jackson classes in the public API.
To do cross-testing, we can do something similar to we do with Hadoop 2, you can get some inspiration there:
Lines 638 to 643 in d4384d3
There was a problem hiding this comment.
I think 1.8 is a reasonable lower bound! for context on our use case, we use the Beam library, which until very recently was tightly coupled with 1.8. Things are now more flexible, but there's a lot of inertia surrounding 1.8 -- the breaking changes surrounding logical types make it a non-trivial upgrade 😅
I'll take a look at the hadoop setup! Avro is a bit tricky because we need to both set the Avro-compiler version, and load the Avro core library, for whatever version of Avro we want to test...
There was a problem hiding this comment.
Thanks for the context, that helps. I see that 1.8.2 is still in there, so that sounds reasonable to me 👍
parquet-avro/src/main/java/org/apache/parquet/avro/AvroSchemaConverter.java
Show resolved
Hide resolved
…pache#2993) * PARQUET-2992: Gate LocalTimestamp references in AvroSchemaConverter * apacheGH-2992: Test logical type conversion for different Avro versions
Rationale for this change
See linked issue; AvroSchemaConverter references Avro classes and methods that don't exist before Avro 1.10
What changes are included in this PR?
Performs an Avro version check to silo references to local timestamp classes.
Are these changes tested?
I've tested them locally on one of my projects using Avro 1.8... unfortunately there isn't a great way to unit-test this :/
Are there any user-facing changes?
This should preserve the existing behavior for Avro 1.8 users while unlocking the new LocalTimestamp types for 1.10+ users.
Closes #2992