RussellSpitzer commented on code in PR #17196:
URL: https://github.com/apache/iceberg/pull/17196#discussion_r3604451979
##########
core/src/main/java/org/apache/iceberg/data/avro/DataReader.java:
##########
@@ -49,21 +49,37 @@ public static <D> DataReader<D> create(
public static <D> DataReader<D> create(
org.apache.iceberg.Schema expectedSchema, Schema readSchema,
Map<Integer, ?> idToConstant) {
- return new DataReader<>(expectedSchema, readSchema, idToConstant);
+ return create(expectedSchema, readSchema, idToConstant, true);
+ }
+
+ public static <D> DataReader<D> create(
Review Comment:
So for this and the other public methods, I think we had a thread starting
to discuss this elsewhere but I want to start a new one so we can hash this out
a fresh. So I don't get too confused.
To start with where i'm coming from. I don't want to expose two behaviors
anywhere I can expose just 1 so i'm strongly biased towards not parameterizing
public methods if at all possible. My gut feeling here is that this should be
a package private constructor, we don't actually want to let external readers
decide to use the old behavior. I was imagining
AvroSchemaUtil publicly just always be non-legacy, no overloaded constructors
WriteBuilder has an optional "legacyTimestampMapping"
DataReader has a package private fallback constructor
So what is the strongest case for allowing external consumers of these
functions to fallback to the legacy behavior instead of just always using
"non-legacy"?
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
