Core: Deprecate Avro.ReadBuilder.createReaderFunc, ProjectionDatumReader - #17853
Core: Deprecate Avro.ReadBuilder.createReaderFunc, ProjectionDatumReader#17853kinolaev wants to merge 1 commit into
Conversation
| import org.apache.iceberg.mapping.NameMapping; | ||
| import org.apache.iceberg.types.TypeUtil; | ||
|
|
||
| public class ProjectionDatumReader<D> implements DatumReader<D>, SupportsRowPosition { |
There was a problem hiding this comment.
This is a public class. Wouldn't it be better to deprecate it first?
https://iceberg.apache.org/contribute/#minor-version-deprecations-required
Modules iceberg-common iceberg-core iceberg-data iceberg-orc iceberg-parquet
Changes to public interfaces and classes in the subprojects listed above require a deprecation cycle of one minor release.
These projects contain common and internal code used by other projects and can evolve within a major release. Minor release deprecation will provide other subprojects and external projects notice and opportunity to transition to new implementations.
There was a problem hiding this comment.
Sounds right to me, but I don't have a strong opinion about it. Let's wait to hear what other community members prefer.
manuzhang
left a comment
manuzhang
left a comment
There was a problem hiding this comment.
Please mark it @deprecated first.
kinolaev
changed the title
61fdec7 to
3daf2b3
Compare
|
Thanks @ebyhr and @manuzhang. I've restored the class and methods, and added deprecation notices. |
| } | ||
|
|
||
| /** | ||
| * @deprecated will be removed in 1.13.0; use {@link #createResolvingReader(Function)} instead. |
There was a problem hiding this comment.
thanks @kinolaev for the change, but I think it's better to follow the convention in https://iceberg.apache.org/contribute/#deprecation-notices
to state both deprecated since, target removal version and if there's any replacement to use. There's some inconsistency across 3 deprecation notices.
There was a problem hiding this comment.
thanks @dramaticlly, I've updated the notices.
Signed-off-by: Sergei Nikolaev <kinolaev@gmail.com>
dramaticlly
left a comment
dramaticlly
left a comment
There was a problem hiding this comment.
LGTM but with some questions for myself
| * @deprecated since 1.12.0, will be removed in 1.13.0; use {@link | ||
| * #createResolvingReader(Function)} instead. |
There was a problem hiding this comment.
This reads like a drop-in replacement but I think new function takes different parameters, as iceberg schema instead of Avro schema, and it rely on caller to call setSchema(avroSchema) first.
There was a problem hiding this comment.
Yes, a user that isn't satisfied with GenericAvroReader and PlannedDataReader should convert the passed read schema first:
Avro.read().createResolvingReader(icebergReadSchema -> new MyAvroReader(AvroSchemaUtil.convert(icebergReadSchema)));As far as I understand, calling setSchema(avroFileSchema) is not required because Avro's DataFileReader will call it automatically.
| return this; | ||
| } | ||
|
|
||
| public ReadBuilder rename(String fullName, String newName) { |
There was a problem hiding this comment.
this also looks not used, especially after ProjectionDatumReader removal, might worth considering as well.
There was a problem hiding this comment.
renames can also be passed to GenericAvroReader via the SupportsCustomRecords interface.
4 participants
Possibly part of #16445.
It seems like
ProjectionDatumReaderwas only used withDataReaderviaAvro.ReadBuilder.createReaderFunc. AfterDataReaderremoval (#17699) theTestReadDefaultValues.testDefaultDoesNotOverrideExplicitValuetest is the only caller. And the test succeed without.createReaderFunc(GenericAvroReader::create)call, becauseGenericAvroReaderis the default reader inAvro.ReadBuilder.