nastra commented on code in PR #9487:
URL: https://github.com/apache/iceberg/pull/9487#discussion_r1479468964
##########
core/src/main/java/org/apache/iceberg/jdbc/JdbcCatalog.java:
##########
@@ -80,19 +83,37 @@ public class JdbcCatalog extends BaseMetastoreCatalog
private final Function<Map<String, String>, FileIO> ioBuilder;
private final Function<Map<String, String>, JdbcClientPool>
clientPoolBuilder;
private final boolean initializeCatalogTables;
+ private final boolean updateCatalogTables;
private CloseableGroup closeableGroup;
public JdbcCatalog() {
- this(null, null, true);
+ this(null, null, true, true);
}
+ /**
+ * @deprecated Use {@link JdbcCatalog#JdbcCatalog(Function, Function,
boolean, boolean)} instead;
+ * will be removed in 2.0.0
+ */
+ @Deprecated
public JdbcCatalog(
Function<Map<String, String>, FileIO> ioBuilder,
Function<Map<String, String>, JdbcClientPool> clientPoolBuilder,
boolean initializeCatalogTables) {
this.ioBuilder = ioBuilder;
this.clientPoolBuilder = clientPoolBuilder;
this.initializeCatalogTables = initializeCatalogTables;
+ this.updateCatalogTables = true;
+ }
+
+ public JdbcCatalog(
+ Function<Map<String, String>, FileIO> ioBuilder,
+ Function<Map<String, String>, JdbcClientPool> clientPoolBuilder,
+ boolean initializeCatalogTables,
+ boolean updateCatalogTables) {
+ this.ioBuilder = ioBuilder;
+ this.clientPoolBuilder = clientPoolBuilder;
+ this.initializeCatalogTables = initializeCatalogTables;
+ this.updateCatalogTables = updateCatalogTables;
Review Comment:
My impression from reading comments from @rdblue and @danielcweeks was that
we want the migration to the new schema to be **explicit** (please correct me
if I'm wrong here). However, `updateCatalogTables` is `true` by default,
meaning that old clients will be messed up and see views as tables if a single
new client just happened to connect to that DB as well and trigger a migration.
In order to achieve an explicit migration, I think we should have a separate
property, such as `jdbc.add-view-schema` or `jdbc.add-view-support` (note that
I explicitly didn't want to use the verb `enable`, because it implies that you
can disable it again, which isn't the case here).
* by default, `jdbc.add-view-support` should be `false`.
* having this property, users can configure it via Spark:
`spark.sql.catalog.<my-catalog>.jdbc.add-view-support=true` (this flag should
be required only once)
* we should also make sure to properly document this flag and mention this
in the release notes, so that users are aware of this and its implications.
Once users upgraded all of their clients, they can set this flag to `true`
The alternative to having this property is to tell users run the `ALTER`
stmt themselves to enable view support, but given that we already have code
that runs the `ALTER` I'd probably be in favor of that property.
--
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: issues-unsubscr...@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For additional commands, e-mail: issues-h...@iceberg.apache.org
