You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "jbonofre (via GitHub)" <gi...@apache.org> on 2024/04/09 19:26:05 UTC
[PR] [WIP] Core: Fix JDBC Catalog table commit when migrating from schema V0 to V1 [iceberg]
jbonofre opened a new pull request, #10111:
URL: https://github.com/apache/iceberg/pull/10111
This PR fixes table commit on the JDBC Catalog.
When upgrading JDBC Catalog SQL schema from `V0` to `V1`, the `iceberg_type` column is added with `NULL` value by default.
However, the SQL statement to update table (updating metadata location for instance) expects `iceberg_type` contains `TABLE`. It doesn't test if `iceberg_type` is `NULL`.
This PR updates the SQL statement to deal with `NULL` or `TABLE` for `iceberg_type`.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] [WIP] Core: Fix JDBC Catalog table commit when migrating from schema V0 to V1 [iceberg]
Posted by "jbonofre (via GitHub)" <gi...@apache.org>.
jbonofre commented on PR #10111:
URL: https://github.com/apache/iceberg/pull/10111#issuecomment-2045915251
I'm preparing tests to illustrate the original problem and demonstrate this fix works.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Fix JDBC Catalog table commit when migrating from schema V0 to V1 [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #10111:
URL: https://github.com/apache/iceberg/pull/10111#discussion_r1559581343
##########
core/src/test/java/org/apache/iceberg/jdbc/TestJdbcUtil.java:
##########
@@ -45,4 +51,114 @@ public void testFilterAndRemovePrefix() {
assertThat(expected).isEqualTo(actual);
}
+
+ @Test
+ public void testV0toV1SqlStatements() throws Exception {
Review Comment:
I think it would make sense to have such a test where the Catalog API is used to create a table/view rathen than the underlying SQL statements
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] [WIP] Core: Fix JDBC Catalog table commit when migrating from schema V0 to V1 [iceberg]
Posted by "jbonofre (via GitHub)" <gi...@apache.org>.
jbonofre commented on code in PR #10111:
URL: https://github.com/apache/iceberg/pull/10111#discussion_r1558816450
##########
core/src/main/java/org/apache/iceberg/jdbc/JdbcUtil.java:
##########
@@ -54,7 +54,29 @@ enum SchemaVersion {
static final String TABLE_RECORD_TYPE = "TABLE";
static final String VIEW_RECORD_TYPE = "VIEW";
- private static final String V1_DO_COMMIT_SQL =
+ private static final String V1_DO_COMMIT_TABLE_SQL =
+ "UPDATE "
+ + CATALOG_TABLE_VIEW_NAME
+ + " SET "
+ + JdbcTableOperations.METADATA_LOCATION_PROP
+ + " = ? , "
+ + JdbcTableOperations.PREVIOUS_METADATA_LOCATION_PROP
+ + " = ?"
+ + " WHERE "
+ + CATALOG_NAME
+ + " = ? AND "
+ + TABLE_NAMESPACE
+ + " = ? AND "
+ + TABLE_NAME
+ + " = ? AND "
+ + JdbcTableOperations.METADATA_LOCATION_PROP
+ + " = ? AND ("
+ + RECORD_TYPE
+ + " = ?"
Review Comment:
Yes I can hardcode. Just adding the or clause won't work as we need the parenthesis.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Fix JDBC Catalog table commit when migrating from schema V0 to V1 [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #10111:
URL: https://github.com/apache/iceberg/pull/10111#discussion_r1565994045
##########
core/src/test/java/org/apache/iceberg/jdbc/TestJdbcCatalog.java:
##########
@@ -1024,7 +1028,49 @@ public void report(MetricsReport report) {
}
}
+ private String createMetadataLocationViaJdbcCatalog(TableIdentifier identifier)
+ throws SQLException {
+ // temporary connection just to actually create a concrete metadata location
+ String jdbcUrl = null;
+ try {
+ java.nio.file.Path dbFile = Files.createTempFile("temp", "metadata");
+ jdbcUrl = "jdbc:sqlite:" + dbFile.toAbsolutePath();
+ } catch (IOException e) {
+ throw new SQLException("Error while creating temp data", e);
+ }
+
+ Map<String, String> properties = Maps.newHashMap();
+
+ properties.put(CatalogProperties.URI, jdbcUrl);
+
+ warehouseLocation = this.tableDir.toAbsolutePath().toString();
+ properties.put(CatalogProperties.WAREHOUSE_LOCATION, warehouseLocation);
+ properties.put("type", "jdbc");
+
+ JdbcCatalog jdbcCatalog =
+ (JdbcCatalog) CatalogUtil.buildIcebergCatalog("TEMP", properties, conf);
+ jdbcCatalog.buildTable(identifier, SCHEMA).create();
+
+ SQLiteDataSource dataSource = new SQLiteDataSource();
+ dataSource.setUrl(jdbcUrl);
+
+ try (Connection connection = dataSource.getConnection()) {
+ ResultSet result =
+ connection
+ .prepareStatement("SELECT * FROM " + JdbcUtil.CATALOG_TABLE_VIEW_NAME)
+ .executeQuery();
+ result.next();
+ return result.getString(JdbcTableOperations.METADATA_LOCATION_PROP);
+ }
+ }
+
private void initLegacySchema(String jdbcUrl) throws SQLException {
+ TableIdentifier table1 = TableIdentifier.of(Namespace.of("namespace1"), "table1");
+ TableIdentifier table2 = TableIdentifier.of(Namespace.of("namespace2"), "table2");
+
+ String table1MetadataLocation = createMetadataLocationViaJdbcCatalog(table1);
Review Comment:
I'm also not super opinionated since I see that the current test does ultimately test the behavior we want to validate.
I think @nastra suggestion of
>get the legacy schema we might want to consider creating the table via the Catalog API and then dropping the iceberg_type field.
is probably cleaner, because I think it removes the need for the insert into statement with all the metadata file details etc. The only additional code would be the one to drop the `iceberg_type` field but I think that would be quite small in comparison (again all theoretical in my head :) ) ? So overall seems like that approach would be a net reduction in the test code and still be readable.
Either way not super opinionated, for the purpose of this PR I think we're good; we can cleanup in a follow on.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Fix JDBC Catalog table commit when migrating from schema V0 to V1 [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on PR #10111:
URL: https://github.com/apache/iceberg/pull/10111#issuecomment-2057754827
Looks like Flink CI timed out when running actions/cache before even running the actual tests. I'm going to retrigger and merge.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Fix JDBC Catalog table commit when migrating from schema V0 to V1 [iceberg]
Posted by "jbonofre (via GitHub)" <gi...@apache.org>.
jbonofre commented on code in PR #10111:
URL: https://github.com/apache/iceberg/pull/10111#discussion_r1559367008
##########
core/src/test/java/org/apache/iceberg/jdbc/TestJdbcUtil.java:
##########
@@ -45,4 +51,97 @@ public void testFilterAndRemovePrefix() {
assertThat(expected).isEqualTo(actual);
}
+
+ @Test
+ public void testV0toV1SqlStatements() throws Exception {
+ java.nio.file.Path dbFile = Files.createTempFile("icebergSchemaUpdate", "db");
+ String jdbcUrl = "jdbc:sqlite:" + dbFile.toAbsolutePath();
+
+ SQLiteDataSource dataSource = new SQLiteDataSource();
+ dataSource.setUrl(jdbcUrl);
+
+ try (JdbcClientPool connections = new JdbcClientPool(jdbcUrl, Maps.newHashMap())) {
+ // create "old style" SQL schema
+ connections.newClient().prepareStatement(JdbcUtil.V0_CREATE_CATALOG_SQL).executeUpdate();
+
+ // inserting tables
+ JdbcUtil.doCommitCreateTable(
+ JdbcUtil.SchemaVersion.V0,
+ connections,
+ "TEST",
+ Namespace.of("namespace1"),
+ TableIdentifier.of(Namespace.of("namespace1"), "table1"),
+ "testLocation");
+ JdbcUtil.doCommitCreateTable(
Review Comment:
You mean `table1` is enough for the test ?
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Fix JDBC Catalog table commit when migrating from schema V0 to V1 [iceberg]
Posted by "jbonofre (via GitHub)" <gi...@apache.org>.
jbonofre commented on code in PR #10111:
URL: https://github.com/apache/iceberg/pull/10111#discussion_r1559923936
##########
core/src/test/java/org/apache/iceberg/jdbc/TestJdbcUtil.java:
##########
@@ -45,4 +51,97 @@ public void testFilterAndRemovePrefix() {
assertThat(expected).isEqualTo(actual);
}
+
+ @Test
+ public void testV0toV1SqlStatements() throws Exception {
+ java.nio.file.Path dbFile = Files.createTempFile("icebergSchemaUpdate", "db");
+ String jdbcUrl = "jdbc:sqlite:" + dbFile.toAbsolutePath();
+
+ SQLiteDataSource dataSource = new SQLiteDataSource();
+ dataSource.setUrl(jdbcUrl);
+
+ try (JdbcClientPool connections = new JdbcClientPool(jdbcUrl, Maps.newHashMap())) {
+ // create "old style" SQL schema
+ connections.newClient().prepareStatement(JdbcUtil.V0_CREATE_CATALOG_SQL).executeUpdate();
+
+ // inserting tables
+ JdbcUtil.doCommitCreateTable(
+ JdbcUtil.SchemaVersion.V0,
+ connections,
+ "TEST",
+ Namespace.of("namespace1"),
+ TableIdentifier.of(Namespace.of("namespace1"), "table1"),
+ "testLocation");
+ JdbcUtil.doCommitCreateTable(
+ JdbcUtil.SchemaVersion.V0,
+ connections,
+ "TEST",
+ Namespace.of("namespace1"),
+ TableIdentifier.of(Namespace.of("namespace1"), "table2"),
+ "testLocation");
+
+ try (PreparedStatement statement =
+ connections.newClient().prepareStatement(JdbcUtil.V0_LIST_TABLE_SQL)) {
+ statement.setString(1, "TEST");
+ statement.setString(2, "namespace1");
+ ResultSet tables = statement.executeQuery();
+ tables.next();
+ assertThat(tables.getString(JdbcUtil.TABLE_NAME)).isEqualTo("table1");
+ tables.next();
+ assertThat(tables.getString(JdbcUtil.TABLE_NAME)).isEqualTo("table2");
+ }
+
+ // updating the schema from V0 to V1
+ connections.newClient().prepareStatement(JdbcUtil.V1_UPDATE_CATALOG_SQL).execute();
+
+ // trying to add a table on the updated schema
+ JdbcUtil.doCommitCreateTable(
+ JdbcUtil.SchemaVersion.V1,
+ connections,
+ "TEST",
+ Namespace.of("namespace1"),
+ TableIdentifier.of(Namespace.of("namespace1"), "table3"),
+ "testLocation");
+
+ // testing the tables after migration and new table added
+ try (PreparedStatement statement =
+ connections.newClient().prepareStatement(JdbcUtil.V0_LIST_TABLE_SQL)) {
+ statement.setString(1, "TEST");
+ statement.setString(2, "namespace1");
+ ResultSet tables = statement.executeQuery();
+ tables.next();
+ assertThat(tables.getString(JdbcUtil.TABLE_NAME)).isEqualTo("table1");
+ assertThat(tables.getString(JdbcUtil.RECORD_TYPE)).isNull();
+ tables.next();
+ assertThat(tables.getString(JdbcUtil.TABLE_NAME)).isEqualTo("table2");
+ assertThat(tables.getString(JdbcUtil.RECORD_TYPE)).isNull();
+ tables.next();
+ assertThat(tables.getString(JdbcUtil.TABLE_NAME)).isEqualTo("table3");
+ assertThat(tables.getString(JdbcUtil.RECORD_TYPE)).isEqualTo(JdbcUtil.TABLE_RECORD_TYPE);
+ }
+
+ // update a table (commit) created on V1 schema
+ int updated =
+ JdbcUtil.updateTable(
+ JdbcUtil.SchemaVersion.V1,
+ connections,
+ "TEST",
+ TableIdentifier.of(Namespace.of("namespace1"), "table3"),
+ "newLocation",
+ "testLocation");
+ assertThat(updated).isEqualTo(1);
+
+ // update a table (commit) migrated from V0 schema
+ // this was failing before the fix from PR #10111
Review Comment:
OK, no problem. Let me remove this comment.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] [WIP] Core: Fix JDBC Catalog table commit when migrating from schema V0 to V1 [iceberg]
Posted by "ajantha-bhat (via GitHub)" <gi...@apache.org>.
ajantha-bhat commented on code in PR #10111:
URL: https://github.com/apache/iceberg/pull/10111#discussion_r1558665711
##########
core/src/main/java/org/apache/iceberg/jdbc/JdbcUtil.java:
##########
@@ -54,7 +54,29 @@ enum SchemaVersion {
static final String TABLE_RECORD_TYPE = "TABLE";
static final String VIEW_RECORD_TYPE = "VIEW";
- private static final String V1_DO_COMMIT_SQL =
+ private static final String V1_DO_COMMIT_TABLE_SQL =
+ "UPDATE "
+ + CATALOG_TABLE_VIEW_NAME
+ + " SET "
+ + JdbcTableOperations.METADATA_LOCATION_PROP
+ + " = ? , "
+ + JdbcTableOperations.PREVIOUS_METADATA_LOCATION_PROP
+ + " = ?"
+ + " WHERE "
+ + CATALOG_NAME
+ + " = ? AND "
+ + TABLE_NAMESPACE
+ + " = ? AND "
+ + TABLE_NAME
+ + " = ? AND "
+ + JdbcTableOperations.METADATA_LOCATION_PROP
+ + " = ? AND ("
+ + RECORD_TYPE
+ + " = ?"
Review Comment:
Should we hardcode the record type to TABLE since it is only called for table?
Similar comment for the `V1_DO_COMMIT_VIEW_SQL`.
or may be we just have to add "OR RECORD_TYPE IS NULL" to previous `V1_DO_COMMIT_SQL` without new changes.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Fix JDBC Catalog table commit when migrating from schema V0 to V1 [iceberg]
Posted by "ajantha-bhat (via GitHub)" <gi...@apache.org>.
ajantha-bhat commented on code in PR #10111:
URL: https://github.com/apache/iceberg/pull/10111#discussion_r1559369352
##########
core/src/test/java/org/apache/iceberg/jdbc/TestJdbcUtil.java:
##########
@@ -45,4 +51,97 @@ public void testFilterAndRemovePrefix() {
assertThat(expected).isEqualTo(actual);
}
+
+ @Test
+ public void testV0toV1SqlStatements() throws Exception {
+ java.nio.file.Path dbFile = Files.createTempFile("icebergSchemaUpdate", "db");
+ String jdbcUrl = "jdbc:sqlite:" + dbFile.toAbsolutePath();
+
+ SQLiteDataSource dataSource = new SQLiteDataSource();
+ dataSource.setUrl(jdbcUrl);
+
+ try (JdbcClientPool connections = new JdbcClientPool(jdbcUrl, Maps.newHashMap())) {
+ // create "old style" SQL schema
+ connections.newClient().prepareStatement(JdbcUtil.V0_CREATE_CATALOG_SQL).executeUpdate();
+
+ // inserting tables
+ JdbcUtil.doCommitCreateTable(
+ JdbcUtil.SchemaVersion.V0,
+ connections,
+ "TEST",
+ Namespace.of("namespace1"),
+ TableIdentifier.of(Namespace.of("namespace1"), "table1"),
+ "testLocation");
+ JdbcUtil.doCommitCreateTable(
Review Comment:
yeah
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Fix JDBC Catalog table commit when migrating from schema V0 to V1 [iceberg]
Posted by "jbonofre (via GitHub)" <gi...@apache.org>.
jbonofre commented on code in PR #10111:
URL: https://github.com/apache/iceberg/pull/10111#discussion_r1559367373
##########
core/src/test/java/org/apache/iceberg/jdbc/TestJdbcUtil.java:
##########
@@ -45,4 +51,97 @@ public void testFilterAndRemovePrefix() {
assertThat(expected).isEqualTo(actual);
}
+
+ @Test
+ public void testV0toV1SqlStatements() throws Exception {
+ java.nio.file.Path dbFile = Files.createTempFile("icebergSchemaUpdate", "db");
+ String jdbcUrl = "jdbc:sqlite:" + dbFile.toAbsolutePath();
+
+ SQLiteDataSource dataSource = new SQLiteDataSource();
+ dataSource.setUrl(jdbcUrl);
+
+ try (JdbcClientPool connections = new JdbcClientPool(jdbcUrl, Maps.newHashMap())) {
+ // create "old style" SQL schema
+ connections.newClient().prepareStatement(JdbcUtil.V0_CREATE_CATALOG_SQL).executeUpdate();
+
+ // inserting tables
+ JdbcUtil.doCommitCreateTable(
+ JdbcUtil.SchemaVersion.V0,
+ connections,
+ "TEST",
+ Namespace.of("namespace1"),
+ TableIdentifier.of(Namespace.of("namespace1"), "table1"),
+ "testLocation");
+ JdbcUtil.doCommitCreateTable(
+ JdbcUtil.SchemaVersion.V0,
+ connections,
+ "TEST",
+ Namespace.of("namespace1"),
+ TableIdentifier.of(Namespace.of("namespace1"), "table2"),
+ "testLocation");
+
+ try (PreparedStatement statement =
+ connections.newClient().prepareStatement(JdbcUtil.V0_LIST_TABLE_SQL)) {
+ statement.setString(1, "TEST");
+ statement.setString(2, "namespace1");
+ ResultSet tables = statement.executeQuery();
+ tables.next();
+ assertThat(tables.getString(JdbcUtil.TABLE_NAME)).isEqualTo("table1");
+ tables.next();
+ assertThat(tables.getString(JdbcUtil.TABLE_NAME)).isEqualTo("table2");
+ }
+
+ // updating the schema from V0 to V1
+ connections.newClient().prepareStatement(JdbcUtil.V1_UPDATE_CATALOG_SQL).execute();
+
+ // trying to add a table on the updated schema
Review Comment:
Good point, I will add this.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Fix JDBC Catalog table commit when migrating from schema V0 to V1 [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #10111:
URL: https://github.com/apache/iceberg/pull/10111#discussion_r1559627285
##########
core/src/test/java/org/apache/iceberg/jdbc/TestJdbcUtil.java:
##########
@@ -45,4 +51,114 @@ public void testFilterAndRemovePrefix() {
assertThat(expected).isEqualTo(actual);
}
+
+ @Test
+ public void testV0toV1SqlStatements() throws Exception {
Review Comment:
it's probably ok to keep these tests here, but users will always interact via the Catalog API, so we need to make sure that stuff works when such an upgrade is done and the Catalog API is used
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Fix JDBC Catalog table commit when migrating from schema V0 to V1 [iceberg]
Posted by "jbonofre (via GitHub)" <gi...@apache.org>.
jbonofre commented on code in PR #10111:
URL: https://github.com/apache/iceberg/pull/10111#discussion_r1565744149
##########
core/src/test/java/org/apache/iceberg/jdbc/TestJdbcCatalog.java:
##########
@@ -1024,7 +1028,49 @@ public void report(MetricsReport report) {
}
}
+ private String createMetadataLocationViaJdbcCatalog(TableIdentifier identifier)
+ throws SQLException {
+ // temporary connection just to actually create a concrete metadata location
+ String jdbcUrl = null;
+ try {
+ java.nio.file.Path dbFile = Files.createTempFile("temp", "metadata");
+ jdbcUrl = "jdbc:sqlite:" + dbFile.toAbsolutePath();
+ } catch (IOException e) {
+ throw new SQLException("Error while creating temp data", e);
+ }
+
+ Map<String, String> properties = Maps.newHashMap();
+
+ properties.put(CatalogProperties.URI, jdbcUrl);
+
+ warehouseLocation = this.tableDir.toAbsolutePath().toString();
+ properties.put(CatalogProperties.WAREHOUSE_LOCATION, warehouseLocation);
+ properties.put("type", "jdbc");
+
+ JdbcCatalog jdbcCatalog =
+ (JdbcCatalog) CatalogUtil.buildIcebergCatalog("TEMP", properties, conf);
+ jdbcCatalog.buildTable(identifier, SCHEMA).create();
+
+ SQLiteDataSource dataSource = new SQLiteDataSource();
+ dataSource.setUrl(jdbcUrl);
+
+ try (Connection connection = dataSource.getConnection()) {
+ ResultSet result =
+ connection
+ .prepareStatement("SELECT * FROM " + JdbcUtil.CATALOG_TABLE_VIEW_NAME)
+ .executeQuery();
+ result.next();
+ return result.getString(JdbcTableOperations.METADATA_LOCATION_PROP);
+ }
+ }
+
private void initLegacySchema(String jdbcUrl) throws SQLException {
+ TableIdentifier table1 = TableIdentifier.of(Namespace.of("namespace1"), "table1");
+ TableIdentifier table2 = TableIdentifier.of(Namespace.of("namespace2"), "table2");
+
+ String table1MetadataLocation = createMetadataLocationViaJdbcCatalog(table1);
Review Comment:
I used a temporary kind of "fake" catalog to just get the metadata location as we don't have an API (public) to create the metadata in the test.
I was thinking about changing a bit the API to be able to easily create metadata location in tests, but I didn't want to do that in this PR (as it's larger).
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Fix JDBC Catalog table commit when migrating from schema V0 to V1 [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #10111:
URL: https://github.com/apache/iceberg/pull/10111#discussion_r1565738304
##########
core/src/test/java/org/apache/iceberg/jdbc/TestJdbcCatalog.java:
##########
@@ -1024,7 +1028,49 @@ public void report(MetricsReport report) {
}
}
+ private String createMetadataLocationViaJdbcCatalog(TableIdentifier identifier)
+ throws SQLException {
+ // temporary connection just to actually create a concrete metadata location
+ String jdbcUrl = null;
+ try {
+ java.nio.file.Path dbFile = Files.createTempFile("temp", "metadata");
+ jdbcUrl = "jdbc:sqlite:" + dbFile.toAbsolutePath();
+ } catch (IOException e) {
+ throw new SQLException("Error while creating temp data", e);
+ }
+
+ Map<String, String> properties = Maps.newHashMap();
+
+ properties.put(CatalogProperties.URI, jdbcUrl);
+
+ warehouseLocation = this.tableDir.toAbsolutePath().toString();
+ properties.put(CatalogProperties.WAREHOUSE_LOCATION, warehouseLocation);
+ properties.put("type", "jdbc");
+
+ JdbcCatalog jdbcCatalog =
+ (JdbcCatalog) CatalogUtil.buildIcebergCatalog("TEMP", properties, conf);
+ jdbcCatalog.buildTable(identifier, SCHEMA).create();
+
+ SQLiteDataSource dataSource = new SQLiteDataSource();
+ dataSource.setUrl(jdbcUrl);
+
+ try (Connection connection = dataSource.getConnection()) {
+ ResultSet result =
+ connection
+ .prepareStatement("SELECT * FROM " + JdbcUtil.CATALOG_TABLE_VIEW_NAME)
+ .executeQuery();
+ result.next();
+ return result.getString(JdbcTableOperations.METADATA_LOCATION_PROP);
+ }
+ }
+
private void initLegacySchema(String jdbcUrl) throws SQLException {
+ TableIdentifier table1 = TableIdentifier.of(Namespace.of("namespace1"), "table1");
+ TableIdentifier table2 = TableIdentifier.of(Namespace.of("namespace2"), "table2");
+
+ String table1MetadataLocation = createMetadataLocationViaJdbcCatalog(table1);
Review Comment:
I don't have a strong opinion here but I think to get the `legacy` schema we might want to consider creating the table via the Catalog API and then dropping the `iceberg_type` field. It seems a bit odd to actually create the table just to fetch its metadata location so that we can insert it here.
I'm curious what others think here /cc @amogh-jahagirdar @danielcweeks
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Fix JDBC Catalog table commit when migrating from schema V0 to V1 [iceberg]
Posted by "jbonofre (via GitHub)" <gi...@apache.org>.
jbonofre commented on code in PR #10111:
URL: https://github.com/apache/iceberg/pull/10111#discussion_r1559603081
##########
core/src/test/java/org/apache/iceberg/jdbc/TestJdbcUtil.java:
##########
@@ -45,4 +51,114 @@ public void testFilterAndRemovePrefix() {
assertThat(expected).isEqualTo(actual);
}
+
+ @Test
+ public void testV0toV1SqlStatements() throws Exception {
Review Comment:
I can do that in the `TestJdbcCatalog` using the API. The reason of this test was to focus on SQL statements.
Let me move this test in `TestJdbcCatalog`.
NB: I will check if the `TestTableCatalog` covers the commit workflow/use case.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Fix JDBC Catalog table commit when migrating from schema V0 to V1 [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #10111:
URL: https://github.com/apache/iceberg/pull/10111#discussion_r1559919696
##########
core/src/test/java/org/apache/iceberg/jdbc/TestJdbcUtil.java:
##########
@@ -45,4 +51,97 @@ public void testFilterAndRemovePrefix() {
assertThat(expected).isEqualTo(actual);
}
+
+ @Test
+ public void testV0toV1SqlStatements() throws Exception {
+ java.nio.file.Path dbFile = Files.createTempFile("icebergSchemaUpdate", "db");
+ String jdbcUrl = "jdbc:sqlite:" + dbFile.toAbsolutePath();
+
+ SQLiteDataSource dataSource = new SQLiteDataSource();
+ dataSource.setUrl(jdbcUrl);
+
+ try (JdbcClientPool connections = new JdbcClientPool(jdbcUrl, Maps.newHashMap())) {
+ // create "old style" SQL schema
+ connections.newClient().prepareStatement(JdbcUtil.V0_CREATE_CATALOG_SQL).executeUpdate();
+
+ // inserting tables
+ JdbcUtil.doCommitCreateTable(
+ JdbcUtil.SchemaVersion.V0,
+ connections,
+ "TEST",
+ Namespace.of("namespace1"),
+ TableIdentifier.of(Namespace.of("namespace1"), "table1"),
+ "testLocation");
+ JdbcUtil.doCommitCreateTable(
+ JdbcUtil.SchemaVersion.V0,
+ connections,
+ "TEST",
+ Namespace.of("namespace1"),
+ TableIdentifier.of(Namespace.of("namespace1"), "table2"),
+ "testLocation");
+
+ try (PreparedStatement statement =
+ connections.newClient().prepareStatement(JdbcUtil.V0_LIST_TABLE_SQL)) {
+ statement.setString(1, "TEST");
+ statement.setString(2, "namespace1");
+ ResultSet tables = statement.executeQuery();
+ tables.next();
+ assertThat(tables.getString(JdbcUtil.TABLE_NAME)).isEqualTo("table1");
+ tables.next();
+ assertThat(tables.getString(JdbcUtil.TABLE_NAME)).isEqualTo("table2");
+ }
+
+ // updating the schema from V0 to V1
+ connections.newClient().prepareStatement(JdbcUtil.V1_UPDATE_CATALOG_SQL).execute();
+
+ // trying to add a table on the updated schema
+ JdbcUtil.doCommitCreateTable(
+ JdbcUtil.SchemaVersion.V1,
+ connections,
+ "TEST",
+ Namespace.of("namespace1"),
+ TableIdentifier.of(Namespace.of("namespace1"), "table3"),
+ "testLocation");
+
+ // testing the tables after migration and new table added
+ try (PreparedStatement statement =
+ connections.newClient().prepareStatement(JdbcUtil.V0_LIST_TABLE_SQL)) {
+ statement.setString(1, "TEST");
+ statement.setString(2, "namespace1");
+ ResultSet tables = statement.executeQuery();
+ tables.next();
+ assertThat(tables.getString(JdbcUtil.TABLE_NAME)).isEqualTo("table1");
+ assertThat(tables.getString(JdbcUtil.RECORD_TYPE)).isNull();
+ tables.next();
+ assertThat(tables.getString(JdbcUtil.TABLE_NAME)).isEqualTo("table2");
+ assertThat(tables.getString(JdbcUtil.RECORD_TYPE)).isNull();
+ tables.next();
+ assertThat(tables.getString(JdbcUtil.TABLE_NAME)).isEqualTo("table3");
+ assertThat(tables.getString(JdbcUtil.RECORD_TYPE)).isEqualTo(JdbcUtil.TABLE_RECORD_TYPE);
+ }
+
+ // update a table (commit) created on V1 schema
+ int updated =
+ JdbcUtil.updateTable(
+ JdbcUtil.SchemaVersion.V1,
+ connections,
+ "TEST",
+ TableIdentifier.of(Namespace.of("namespace1"), "table3"),
+ "newLocation",
+ "testLocation");
+ assertThat(updated).isEqualTo(1);
+
+ // update a table (commit) migrated from V0 schema
+ // this was failing before the fix from PR #10111
Review Comment:
Agree with @ajantha-bhat , I'd just remove the comment on 142. IMO It doesn't add useful info to someone who's reading the test.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Fix JDBC Catalog table commit when migrating from schema V0 to V1 [iceberg]
Posted by "ajantha-bhat (via GitHub)" <gi...@apache.org>.
ajantha-bhat commented on code in PR #10111:
URL: https://github.com/apache/iceberg/pull/10111#discussion_r1559338058
##########
core/src/test/java/org/apache/iceberg/jdbc/TestJdbcUtil.java:
##########
@@ -45,4 +51,97 @@ public void testFilterAndRemovePrefix() {
assertThat(expected).isEqualTo(actual);
}
+
+ @Test
+ public void testV0toV1SqlStatements() throws Exception {
+ java.nio.file.Path dbFile = Files.createTempFile("icebergSchemaUpdate", "db");
+ String jdbcUrl = "jdbc:sqlite:" + dbFile.toAbsolutePath();
+
+ SQLiteDataSource dataSource = new SQLiteDataSource();
+ dataSource.setUrl(jdbcUrl);
+
+ try (JdbcClientPool connections = new JdbcClientPool(jdbcUrl, Maps.newHashMap())) {
+ // create "old style" SQL schema
+ connections.newClient().prepareStatement(JdbcUtil.V0_CREATE_CATALOG_SQL).executeUpdate();
+
+ // inserting tables
+ JdbcUtil.doCommitCreateTable(
+ JdbcUtil.SchemaVersion.V0,
+ connections,
+ "TEST",
+ Namespace.of("namespace1"),
+ TableIdentifier.of(Namespace.of("namespace1"), "table1"),
+ "testLocation");
+ JdbcUtil.doCommitCreateTable(
+ JdbcUtil.SchemaVersion.V0,
+ connections,
+ "TEST",
+ Namespace.of("namespace1"),
+ TableIdentifier.of(Namespace.of("namespace1"), "table2"),
+ "testLocation");
+
+ try (PreparedStatement statement =
+ connections.newClient().prepareStatement(JdbcUtil.V0_LIST_TABLE_SQL)) {
+ statement.setString(1, "TEST");
+ statement.setString(2, "namespace1");
+ ResultSet tables = statement.executeQuery();
+ tables.next();
+ assertThat(tables.getString(JdbcUtil.TABLE_NAME)).isEqualTo("table1");
+ tables.next();
+ assertThat(tables.getString(JdbcUtil.TABLE_NAME)).isEqualTo("table2");
+ }
+
+ // updating the schema from V0 to V1
+ connections.newClient().prepareStatement(JdbcUtil.V1_UPDATE_CATALOG_SQL).execute();
+
+ // trying to add a table on the updated schema
+ JdbcUtil.doCommitCreateTable(
+ JdbcUtil.SchemaVersion.V1,
+ connections,
+ "TEST",
+ Namespace.of("namespace1"),
+ TableIdentifier.of(Namespace.of("namespace1"), "table3"),
+ "testLocation");
+
+ // testing the tables after migration and new table added
+ try (PreparedStatement statement =
+ connections.newClient().prepareStatement(JdbcUtil.V0_LIST_TABLE_SQL)) {
+ statement.setString(1, "TEST");
+ statement.setString(2, "namespace1");
+ ResultSet tables = statement.executeQuery();
+ tables.next();
+ assertThat(tables.getString(JdbcUtil.TABLE_NAME)).isEqualTo("table1");
+ assertThat(tables.getString(JdbcUtil.RECORD_TYPE)).isNull();
+ tables.next();
+ assertThat(tables.getString(JdbcUtil.TABLE_NAME)).isEqualTo("table2");
+ assertThat(tables.getString(JdbcUtil.RECORD_TYPE)).isNull();
+ tables.next();
+ assertThat(tables.getString(JdbcUtil.TABLE_NAME)).isEqualTo("table3");
+ assertThat(tables.getString(JdbcUtil.RECORD_TYPE)).isEqualTo(JdbcUtil.TABLE_RECORD_TYPE);
+ }
+
+ // update a table (commit) created on V1 schema
+ int updated =
+ JdbcUtil.updateTable(
+ JdbcUtil.SchemaVersion.V1,
+ connections,
+ "TEST",
+ TableIdentifier.of(Namespace.of("namespace1"), "table3"),
+ "newLocation",
+ "testLocation");
+ assertThat(updated).isEqualTo(1);
+
+ // update a table (commit) migrated from V0 schema
+ // this was failing before the fix from PR #10111
Review Comment:
nit: PR comment is enough for reviewer. Maybe no need of code comment.
##########
core/src/test/java/org/apache/iceberg/jdbc/TestJdbcUtil.java:
##########
@@ -45,4 +51,97 @@ public void testFilterAndRemovePrefix() {
assertThat(expected).isEqualTo(actual);
}
+
+ @Test
+ public void testV0toV1SqlStatements() throws Exception {
+ java.nio.file.Path dbFile = Files.createTempFile("icebergSchemaUpdate", "db");
+ String jdbcUrl = "jdbc:sqlite:" + dbFile.toAbsolutePath();
+
+ SQLiteDataSource dataSource = new SQLiteDataSource();
+ dataSource.setUrl(jdbcUrl);
+
+ try (JdbcClientPool connections = new JdbcClientPool(jdbcUrl, Maps.newHashMap())) {
+ // create "old style" SQL schema
+ connections.newClient().prepareStatement(JdbcUtil.V0_CREATE_CATALOG_SQL).executeUpdate();
+
+ // inserting tables
+ JdbcUtil.doCommitCreateTable(
+ JdbcUtil.SchemaVersion.V0,
+ connections,
+ "TEST",
+ Namespace.of("namespace1"),
+ TableIdentifier.of(Namespace.of("namespace1"), "table1"),
+ "testLocation");
+ JdbcUtil.doCommitCreateTable(
Review Comment:
nit: table2 may be redundant
##########
core/src/test/java/org/apache/iceberg/jdbc/TestJdbcUtil.java:
##########
@@ -45,4 +51,97 @@ public void testFilterAndRemovePrefix() {
assertThat(expected).isEqualTo(actual);
}
+
+ @Test
+ public void testV0toV1SqlStatements() throws Exception {
+ java.nio.file.Path dbFile = Files.createTempFile("icebergSchemaUpdate", "db");
+ String jdbcUrl = "jdbc:sqlite:" + dbFile.toAbsolutePath();
+
+ SQLiteDataSource dataSource = new SQLiteDataSource();
+ dataSource.setUrl(jdbcUrl);
+
+ try (JdbcClientPool connections = new JdbcClientPool(jdbcUrl, Maps.newHashMap())) {
+ // create "old style" SQL schema
+ connections.newClient().prepareStatement(JdbcUtil.V0_CREATE_CATALOG_SQL).executeUpdate();
+
+ // inserting tables
+ JdbcUtil.doCommitCreateTable(
+ JdbcUtil.SchemaVersion.V0,
+ connections,
+ "TEST",
+ Namespace.of("namespace1"),
+ TableIdentifier.of(Namespace.of("namespace1"), "table1"),
+ "testLocation");
+ JdbcUtil.doCommitCreateTable(
+ JdbcUtil.SchemaVersion.V0,
+ connections,
+ "TEST",
+ Namespace.of("namespace1"),
+ TableIdentifier.of(Namespace.of("namespace1"), "table2"),
+ "testLocation");
+
+ try (PreparedStatement statement =
+ connections.newClient().prepareStatement(JdbcUtil.V0_LIST_TABLE_SQL)) {
+ statement.setString(1, "TEST");
+ statement.setString(2, "namespace1");
+ ResultSet tables = statement.executeQuery();
+ tables.next();
+ assertThat(tables.getString(JdbcUtil.TABLE_NAME)).isEqualTo("table1");
+ tables.next();
+ assertThat(tables.getString(JdbcUtil.TABLE_NAME)).isEqualTo("table2");
+ }
+
+ // updating the schema from V0 to V1
+ connections.newClient().prepareStatement(JdbcUtil.V1_UPDATE_CATALOG_SQL).execute();
+
+ // trying to add a table on the updated schema
Review Comment:
can we add create one view after table3? and update it Because code is changed for view commit SQL also and tests can cover it.
##########
core/src/main/java/org/apache/iceberg/jdbc/JdbcUtil.java:
##########
@@ -54,7 +54,31 @@ enum SchemaVersion {
static final String TABLE_RECORD_TYPE = "TABLE";
static final String VIEW_RECORD_TYPE = "VIEW";
- private static final String V1_DO_COMMIT_SQL =
Review Comment:
Do we also need to handle the same for `V1_DO_COMMIT_CREATE_SQL` ?
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Fix JDBC Catalog table commit when migrating from schema V0 to V1 [iceberg]
Posted by "ajantha-bhat (via GitHub)" <gi...@apache.org>.
ajantha-bhat commented on code in PR #10111:
URL: https://github.com/apache/iceberg/pull/10111#discussion_r1559372303
##########
core/src/main/java/org/apache/iceberg/jdbc/JdbcUtil.java:
##########
@@ -54,7 +54,31 @@ enum SchemaVersion {
static final String TABLE_RECORD_TYPE = "TABLE";
static final String VIEW_RECORD_TYPE = "VIEW";
- private static final String V1_DO_COMMIT_SQL =
Review Comment:
Yeah, True. I just saw the pattern and commented (could have checked more). It is not needed here as it is an insert statement and not the query.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Fix JDBC Catalog table commit when migrating from schema V0 to V1 [iceberg]
Posted by "jbonofre (via GitHub)" <gi...@apache.org>.
jbonofre commented on code in PR #10111:
URL: https://github.com/apache/iceberg/pull/10111#discussion_r1559638330
##########
core/src/test/java/org/apache/iceberg/jdbc/TestJdbcUtil.java:
##########
@@ -45,4 +51,114 @@ public void testFilterAndRemovePrefix() {
assertThat(expected).isEqualTo(actual);
}
+
+ @Test
+ public void testV0toV1SqlStatements() throws Exception {
Review Comment:
Agree, let me add a new test in `TestJdbcCatalog` using the facing API. Thanks @nastra !
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Fix JDBC Catalog table commit when migrating from schema V0 to V1 [iceberg]
Posted by "jbonofre (via GitHub)" <gi...@apache.org>.
jbonofre commented on code in PR #10111:
URL: https://github.com/apache/iceberg/pull/10111#discussion_r1559369000
##########
core/src/main/java/org/apache/iceberg/jdbc/JdbcUtil.java:
##########
@@ -54,7 +54,31 @@ enum SchemaVersion {
static final String TABLE_RECORD_TYPE = "TABLE";
static final String VIEW_RECORD_TYPE = "VIEW";
- private static final String V1_DO_COMMIT_SQL =
Review Comment:
It's not necessary as it creates the record, so populating cleanly the `iceberg_type`. The problem is just with the update where `iceberg_type` is used in the `WHERE` clause.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Fix JDBC Catalog table commit when migrating from schema V0 to V1 [iceberg]
Posted by "jbonofre (via GitHub)" <gi...@apache.org>.
jbonofre commented on PR #10111:
URL: https://github.com/apache/iceberg/pull/10111#issuecomment-2047310640
@nastra @ajantha-bhat @danielcweeks The PR is ready for review. It fixes the commit issue on migrated tables (from `V0` to `V1` schema, where `iceberg_type` is `NULL`). I added a test to illustrate the issue (where it was failing before the fix on the SQL statement).
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Fix JDBC Catalog table commit when migrating from schema V0 to V1 [iceberg]
Posted by "jbonofre (via GitHub)" <gi...@apache.org>.
jbonofre commented on code in PR #10111:
URL: https://github.com/apache/iceberg/pull/10111#discussion_r1565605307
##########
core/src/test/java/org/apache/iceberg/jdbc/TestJdbcUtil.java:
##########
@@ -45,4 +51,114 @@ public void testFilterAndRemovePrefix() {
assertThat(expected).isEqualTo(actual);
}
+
+ @Test
+ public void testV0toV1SqlStatements() throws Exception {
Review Comment:
I updated the PR to extend the migration test with `append` of data files and `commit`. I tested this test on `main` and I confirm it fails. It passes with the fix on this PR.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Fix JDBC Catalog table commit when migrating from schema V0 to V1 [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #10111:
URL: https://github.com/apache/iceberg/pull/10111#discussion_r1565731774
##########
core/src/test/java/org/apache/iceberg/jdbc/TestJdbcCatalog.java:
##########
@@ -239,6 +240,9 @@ public void testSchemaIsMigratedToAddViewSupport() throws Exception {
.create();
assertThat(jdbcCatalog.listViews(Namespace.of("namespace1"))).hasSize(1).containsExactly(view);
+
+ // testing append datafile to check commit, it should not throw an exception
+ jdbcCatalog.loadTable(tableOne).newAppend().appendFile(FILE_A).commit();
Review Comment:
I think we should also make sure that we can create a new table after migration and that the tables can be loaded. Something like this:
```
TableIdentifier tableThree = TableIdentifier.of("namespace2", "table3");
jdbcCatalog.createTable(tableThree, SCHEMA);
assertThat(jdbcCatalog.tableExists(tableThree)).isTrue();
// testing append datafile to check commit, it should not throw an exception
jdbcCatalog.loadTable(tableOne).newAppend().appendFile(FILE_A).commit();
jdbcCatalog.loadTable(tableTwo).newAppend().appendFile(FILE_B).commit();
assertThat(jdbcCatalog.tableExists(tableOne)).isTrue();
assertThat(jdbcCatalog.tableExists(tableTwo)).isTrue();
```
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Fix JDBC Catalog table commit when migrating from schema V0 to V1 [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra merged PR #10111:
URL: https://github.com/apache/iceberg/pull/10111
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org