dramaticlly commented on code in PR #16449:
URL: https://github.com/apache/iceberg/pull/16449#discussion_r3648621284
##########
parquet/src/main/java/org/apache/iceberg/data/parquet/BaseParquetReaders.java:
##########
@@ -78,26 +78,8 @@ protected ParquetValueReader<T> createReader(
}
}
- /**
- * @deprecated will be removed in 1.12.0. Subclasses should override {@link
- * #createStructReader(List, Types.StructType, Integer)} instead
- */
- @Deprecated
- protected ParquetValueReader<T> createStructReader(
- List<ParquetValueReader<?>> fieldReaders, Types.StructType structType) {
- throw new UnsupportedOperationException(
- "Deprecated method is not used in this implementation, only
createStructReader(list, Types.Struct, Integer) should be used");
- }
-
- /**
- * This method can be overridden to provide a custom implementation which
also uses the fieldId of
- * the Schema when creating the struct reader
- */
- protected ParquetValueReader<T> createStructReader(
- List<ParquetValueReader<?>> fieldReaders, Types.StructType structType,
Integer fieldId) {
- // Fallback to the signature without fieldId if not overridden
- return createStructReader(fieldReaders, structType);
- }
+ protected abstract ParquetValueReader<T> createStructReader(
Review Comment:
@RussellSpitzer I think with `createStructReader(fieldReaders, structType)`
removed and all subclass override the method, this can now become abstract
method, do you think this is safe to apply?
##########
core/src/main/java/org/apache/iceberg/SystemConfigs.java:
##########
@@ -80,21 +80,6 @@ private SystemConfigs() {}
8,
Integer::parseUnsignedInt);
- /**
- * @deprecated will be removed in 1.12.0; use name mapping instead
- */
- @Deprecated
- public static final ConfigEntry<Boolean>
NETFLIX_UNSAFE_PARQUET_ID_FALLBACK_ENABLED =
Review Comment:
@rdblue I want to double check with you on this. More in
https://github.com/apache/iceberg/pull/16449#issuecomment-4806641933, I end up
changing few spark and Flink test for using NameMapping.empty() instead of null
(unsafe positional ID assignment).
##########
core/src/main/java/org/apache/iceberg/hadoop/HadoopFileIO.java:
##########
@@ -138,10 +129,6 @@ public Configuration getConf() {
return hadoopConf.get();
}
- /**
- * @deprecated since 1.11.0, will be removed in 1.12.0.
- */
- @Deprecated
Review Comment:
@nastra want to check with you that on not remove this for now, as the
HadoopFileIO today still implement the required method defined in
HadoopConfigurable interface. Can follow up with better removal cycle in 1.13
if needed.
##########
aws/src/test/java/org/apache/iceberg/aws/s3/signer/TestS3V4RestSignerClient.java:
##########
@@ -198,21 +200,9 @@ void legacySignerProperties(
}
}
- @SuppressWarnings("deprecation")
- public static Stream<Arguments> legacySignerProperties() {
+ public static Stream<Arguments> signerUriResolution() {
Review Comment:
@nastra I renamed method source and UT above as after this PR we no longer
have legacySignerProperties
##########
core/src/main/java/org/apache/iceberg/rest/responses/BaseScanTaskResponse.java:
##########
@@ -93,20 +89,7 @@ public B withFileScanTasks(List<FileScanTask> tasks) {
return self();
}
- /**
- * @deprecated since 1.11.0, will be removed in 1.12.0.
- */
- @Deprecated
- public B withDeleteFiles(List<DeleteFile> deleteFilesList) {
- this.deleteFiles = DeleteFileSet.of(deleteFilesList);
- return self();
- }
-
- /**
- * @deprecated since 1.11.0, visibility will be reduced in 1.12.0.
- */
- @Deprecated
- public B withSpecsById(Map<Integer, PartitionSpec> specs) {
+ protected B withSpecsById(Map<Integer, PartitionSpec> specs) {
Review Comment:
@singhpk234 can you help check the visibility change from public to
protected align with your original goal, also end up providing sanctioned entry
point of witSpecById in builder for
BaseScanTaskResponse,FetchPlanningResultResponse,FetchScanTasksResponse,PlanTableScanResponse,
##########
core/src/main/java/org/apache/iceberg/ManifestFiles.java:
##########
@@ -118,39 +118,6 @@ public static CloseableIterable<String> readPaths(
entry -> entry.file().location());
}
- /**
- * Returns a {@link CloseableIterable} of file paths in the {@link
ManifestFile}.
- *
- * @param manifest a ManifestFile
- * @param io a FileIO
- * @return a manifest reader
- * @deprecated since 1.11.0, will be removed in 1.12.0; use {@link
#readPaths(ManifestFile,
- * FileIO, Map)} instead.
- */
- @Deprecated
- public static CloseableIterable<String> readPaths(ManifestFile manifest,
FileIO io) {
- return readPaths(manifest, io, null);
- }
-
- /**
- * Returns a new {@link ManifestReader} for a {@link ManifestFile}.
- *
- * <p><em>Note:</em> Callers should use {@link
ManifestFiles#read(ManifestFile, FileIO, Map)} to
- * ensure the schema used by filters is the latest table schema. This should
be used only when
- * reading a manifest without filters.
- *
- * @param manifest a ManifestFile
- * @param io a FileIO
- * @return a manifest reader
- * @deprecated since 1.11.0, will be removed in 1.12.0; use {@link
#read(ManifestFile, FileIO,
- * Map)} instead. Reading partition specs from manifest file metadata
will not be supported
- * for non-Avro manifest formats.
- */
- @Deprecated
- public static ManifestReader<DataFile> read(ManifestFile manifest, FileIO
io) {
- return read(manifest, io, null);
Review Comment:
@RussellSpitzer I think the public static `read` method might be used a lot
outside iceberg package, want to double check this is ok to remove with 1
release between deprecation and removal
##########
core/src/main/java/org/apache/iceberg/BaseScan.java:
##########
@@ -103,14 +103,6 @@ public Table table() {
return table;
}
- /**
- * @deprecated since 1.11.0, will be removed in 1.12.0; use {@link
BaseScan#fileIO()} instead.
Review Comment:
For existing scan which use io to read manifests, I end up using
`table().io() instead of `fileIO().get()` as it reads better
##########
core/src/main/java/org/apache/iceberg/rest/CatalogHandlers.java:
##########
@@ -502,15 +502,6 @@ public static void tableExists(Catalog catalog,
TableIdentifier ident) {
}
}
- /**
- * @deprecated since 1.11.0, will be removed in 1.12.0. Use {@link
#loadTable(Catalog,
- * TableIdentifier, SnapshotMode)} instead.
- */
- @Deprecated
- public static LoadTableResponse loadTable(Catalog catalog, TableIdentifier
ident) {
Review Comment:
@gaborkaszab I think this 2 arguments public static method might be used a
lot outside iceberg package, want to double check this is ok to remove with 1
release between deprecation and removal.
##########
aws/src/main/java/org/apache/iceberg/aws/s3/signer/S3V4RestSignerClient.java:
##########
@@ -232,36 +203,9 @@ private boolean credentialProvided() {
@Value.Check
protected void check() {
Preconditions.checkArgument(
- properties().containsKey(S3_SIGNER_URI)
- || properties().containsKey(RESTCatalogProperties.SIGNER_URI)
+ properties().containsKey(RESTCatalogProperties.SIGNER_URI)
|| properties().containsKey(CatalogProperties.URI),
"S3 signer service URI is required");
-
- if (properties().containsKey(S3_SIGNER_URI)
- && !properties().containsKey(RESTCatalogProperties.SIGNER_URI)) {
- LOG.warn(
- "S3 signer URI is configured via deprecated property {}, this won't
be supported in future releases. "
- + "Please use {} instead.",
- S3_SIGNER_URI,
- RESTCatalogProperties.SIGNER_URI);
- }
-
- if (properties().containsKey(S3_SIGNER_ENDPOINT)
- && !properties().containsKey(RESTCatalogProperties.SIGNER_ENDPOINT)) {
- LOG.warn(
- "Signer endpoint is configured via deprecated property {}, this
won't be supported in future releases. "
- + "Please use {} instead.",
- S3_SIGNER_ENDPOINT,
- RESTCatalogProperties.SIGNER_ENDPOINT);
- }
-
- // TODO change to required in 1.12.0
- if (!properties().containsKey(S3_SIGNER_ENDPOINT)
- && !properties().containsKey(RESTCatalogProperties.SIGNER_ENDPOINT)) {
- LOG.warn(
Review Comment:
@adutra can you help check the deprecation align with your original goal in
#15451.
- Removes S3_SIGNER_URI / S3_SIGNER_ENDPOINT / S3_SIGNER_DEFAULT_ENDPOINT
- RESTCatalogProperties.SIGNER_ENDPOINT is now required
--
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]
