stevenzwu commented on code in PR #16728:
URL: https://github.com/apache/iceberg/pull/16728#discussion_r3457106951
##########
flink/v2.1/flink/src/main/java/org/apache/iceberg/flink/FlinkCatalog.java:
##########
@@ -112,7 +111,6 @@ public FlinkCatalog(
long cacheExpirationIntervalMs) {
Review Comment:
(re: `Map<String, String> catalogProps,` at line 109 — outside the diff,
anchoring here)
The `catalogProps` constructor parameter is no longer used after the field
and `this.catalogProps = catalogProps` assignment were removed.
`FlinkCatalogFactory` (line 168) still passes the catalog `properties` in, and
the constructor silently discards them. We can drop the parameter and update
the factory caller (this class is `@Internal`).
##########
flink/v2.1/flink/src/main/java/org/apache/iceberg/flink/FlinkCatalog.java:
##########
@@ -345,7 +343,7 @@ public CatalogTable getTable(ObjectPath tablePath)
// props as json string to distinguish between catalog and table
properties in createTable.
Review Comment:
The four-line comment above (lines 339-343) is now stale. It explains why
catalog properties get serialized into the JSON blob ("we need to send across
catalog properties also", "distinguish between catalog and table properties"),
but after this commit the JSON carries only the source name/database/table — no
catalog props. Rephrase to describe what is actually serialized and why.
##########
flink/v2.1/flink/src/main/java/org/apache/iceberg/flink/FlinkDynamicTableFactory.java:
##########
@@ -257,7 +257,6 @@ private static Map<String, String>
mergeSrcCatalogProps(Map<String, String> tabl
FlinkCreateTableOptions.CATALOG_DATABASE.key(),
createTableOptions.catalogDb());
Review Comment:
(re: Javadoc at lines 240-243 — outside the diff, anchoring here)
The Javadoc on `mergeSrcCatalogProps` still claims this method flattens
catalog properties that were serialized as JSON. After this PR the JSON carries
only `<catalogName, db, table>` and the explicit puts above merge only those
three names; no catalog props are flattened. Update the Javadoc to describe the
current behavior.
--
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]
