You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "edgarRd (via GitHub)" <gi...@apache.org> on 2023/03/16 00:51:54 UTC
[GitHub] [iceberg] edgarRd opened a new pull request, #7121: Spark: Parameterize backup suffix in migrate procedure
edgarRd opened a new pull request, #7121:
URL: https://github.com/apache/iceberg/pull/7121
This PR adds the ability to use a custom backup suffix during the `migrate` process. By default, the suffix `__BACKUP__` is used, which sometimes gets represented as `__backup__` in engines that lower case table names leading to confusion.
This change enables uses to optionally set a suffix for those backup tables. A follow up PR in docs would update the parameter list once merged.
PTAL Thanks.
--
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
[GitHub] [iceberg] RussellSpitzer commented on pull request #7121: Spark: Parameterize backup suffix in migrate procedure
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on PR #7121:
URL: https://github.com/apache/iceberg/pull/7121#issuecomment-1478028525
I'd rather not add another option, could we just change the suffix to __backup__? I just can't imagine the situation where folks need to change this dynamically
--
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
[GitHub] [iceberg] sririshindra commented on a diff in pull request #7121: Spark: Parameterize backup suffix in migrate procedure
Posted by "sririshindra (via GitHub)" <gi...@apache.org>.
sririshindra commented on code in PR #7121:
URL: https://github.com/apache/iceberg/pull/7121#discussion_r1142771731
##########
spark/v3.3/spark-extensions/src/test/java/org/apache/iceberg/spark/extensions/TestMigrateTableProcedure.java:
##########
@@ -106,20 +108,41 @@ public void testMigrateWithOptions() throws IOException {
sql("DROP TABLE IF EXISTS %s", tableName + "_BACKUP_");
}
- @Test
- public void testMigrateWithDropBackup() throws IOException {
+ private void testMigrateWithDropBackup(@Nullable String backupSuffix) throws IOException {
Review Comment:
nit: I am not really familiar with the usage of the `@Nullable` annotation. But how about directly passing the optional to the method as an argument. i.e couldn't you just pass `testMigrateWithDropBackup(Optional.ofNullable("_tmp"));` & `testMigrateWithDropBackup(Optional.ofNullable(null));` rather than constructing the optional inside the method.
I am not familiar with this pattern you are using, but if it is some sort of standard coding practice please let me know.
--
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
[GitHub] [iceberg] edgarRd commented on pull request #7121: Spark: Parameterize backup suffix in migrate procedure
Posted by "edgarRd (via GitHub)" <gi...@apache.org>.
edgarRd commented on PR #7121:
URL: https://github.com/apache/iceberg/pull/7121#issuecomment-1477027981
@sririshindra Thanks for the review. I believe I've made the changes requested, PTAL when you have a chance.
--
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
[GitHub] [iceberg] sririshindra commented on a diff in pull request #7121: Spark: Parameterize backup suffix in migrate procedure
Posted by "sririshindra (via GitHub)" <gi...@apache.org>.
sririshindra commented on code in PR #7121:
URL: https://github.com/apache/iceberg/pull/7121#discussion_r1141200013
##########
spark/v3.3/spark/src/main/java/org/apache/iceberg/spark/procedures/MigrateTableProcedure.java:
##########
@@ -91,9 +92,13 @@ public InternalRow[] call(InternalRow args) {
}
boolean dropBackup = args.isNullAt(2) ? false : args.getBoolean(2);
+ String backupSuffix = args.isNullAt(3) ? null : args.getString(3);
MigrateTableSparkAction migrateTableSparkAction =
SparkActions.get().migrateTable(tableName).tableProperties(properties);
+ if (backupSuffix != null) {
+ migrateTableSparkAction = migrateTableSparkAction.withBackupSuffix(backupSuffix);
+ }
MigrateTable.Result result;
if (dropBackup) {
Review Comment:
Can you also change this to the following to match the style above.
```
if (dropBackup) {
migrateTableSparkAction = migrateTableSparkAction.dropBackup()
}
MigrateTable.Result result = migrateTableSparkAction.execute();
```
--
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
[GitHub] [iceberg] edgarRd commented on a diff in pull request #7121: Spark: Parameterize backup suffix in migrate procedure
Posted by "edgarRd (via GitHub)" <gi...@apache.org>.
edgarRd commented on code in PR #7121:
URL: https://github.com/apache/iceberg/pull/7121#discussion_r1142695836
##########
spark/v3.3/spark-extensions/src/test/java/org/apache/iceberg/spark/extensions/TestMigrateTableProcedure.java:
##########
@@ -122,6 +122,38 @@ public void testMigrateWithDropBackup() throws IOException {
Assert.assertFalse(spark.catalog().tableExists(tableName + "_BACKUP_"));
}
+ @Test
+ public void testMigrateWithBackupSuffix() throws IOException {
+ Assume.assumeTrue(catalogName.equals("spark_catalog"));
+ String backupSuffix = "_tmp";
+ String location = temp.newFolder().toString();
+ sql(
+ "CREATE TABLE %s (id bigint NOT NULL, data string) USING parquet LOCATION '%s'",
+ tableName, location);
+ sql("INSERT INTO TABLE %s VALUES (1, 'a')", tableName);
+
+ Object result =
+ scalarSql(
+ "CALL %s.system.migrate(table => '%s', backup_suffix => '%s')",
Review Comment:
Agreed. I thought about that case but didn't add a unit test. Thanks for catching it.
--
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
[GitHub] [iceberg] sririshindra commented on a diff in pull request #7121: Spark: Parameterize backup suffix in migrate procedure
Posted by "sririshindra (via GitHub)" <gi...@apache.org>.
sririshindra commented on code in PR #7121:
URL: https://github.com/apache/iceberg/pull/7121#discussion_r1141193614
##########
spark/v3.3/spark-extensions/src/test/java/org/apache/iceberg/spark/extensions/TestMigrateTableProcedure.java:
##########
@@ -122,6 +122,38 @@ public void testMigrateWithDropBackup() throws IOException {
Assert.assertFalse(spark.catalog().tableExists(tableName + "_BACKUP_"));
}
+ @Test
+ public void testMigrateWithBackupSuffix() throws IOException {
+ Assume.assumeTrue(catalogName.equals("spark_catalog"));
+ String backupSuffix = "_tmp";
+ String location = temp.newFolder().toString();
+ sql(
+ "CREATE TABLE %s (id bigint NOT NULL, data string) USING parquet LOCATION '%s'",
+ tableName, location);
+ sql("INSERT INTO TABLE %s VALUES (1, 'a')", tableName);
+
+ Object result =
+ scalarSql(
+ "CALL %s.system.migrate(table => '%s', backup_suffix => '%s')",
+ catalogName, tableName, backupSuffix);
+
+ Assert.assertEquals("Should have added one file", 1L, result);
+
+ Table createdTable = validationCatalog.loadTable(tableIdent);
+
+ String tableLocation = createdTable.location().replace("file:", "");
+ Assert.assertEquals("Table should have original location", location, tableLocation);
+
+ sql("INSERT INTO TABLE %s VALUES (1, 'a')", tableName);
+
+ assertEquals(
+ "Should have expected rows",
+ ImmutableList.of(row(1L, "a"), row(1L, "a")),
+ sql("SELECT * FROM %s ORDER BY id", tableName));
+
+ sql("DROP TABLE %s", tableName + backupSuffix);
Review Comment:
Before you drop the backup table here, can you also assert that the data in the backup table remains intact after the migration i.e can you do something like
```
assertEquals(
"Should have expected rows",
ImmutableList.of(row(1L, "a")),
sql("SELECT * FROM %s ORDER BY id", tableName + backupSuffix));
```
--
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
[GitHub] [iceberg] edgarRd commented on pull request #7121: Spark: Parameterize backup suffix in migrate procedure
Posted by "edgarRd (via GitHub)" <gi...@apache.org>.
edgarRd commented on PR #7121:
URL: https://github.com/apache/iceberg/pull/7121#issuecomment-1478055925
@RussellSpitzer I think `backup` is just too generic, specially when there are hundreds of tables in a catalog. For instance, users may already use `backup` for other cases, not just for tables migrated to iceberg - this is quite a common case. Therefore, users may have different suffixes they would want to use, for instance `_hive` or any other. In order to not be prescriptive of the suffix - which right now is very generic - allowing users to optionally changing it should fill that gap.
--
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
[GitHub] [iceberg] edgarRd commented on pull request #7121: Spark: Parameterize backup suffix in migrate procedure
Posted by "edgarRd (via GitHub)" <gi...@apache.org>.
edgarRd commented on PR #7121:
URL: https://github.com/apache/iceberg/pull/7121#issuecomment-1478115826
Each user has a different migration strategy and they could use the suffix as a tagging mechanism in the old table name for different scenarios, having a fixed suffix - while it could be more descriptive - still does not fill the gap.
--
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] Spark: Parameterize backup suffix in migrate procedure [iceberg]
Posted by "edgarRd (via GitHub)" <gi...@apache.org>.
edgarRd closed pull request #7121: Spark: Parameterize backup suffix in migrate procedure
URL: https://github.com/apache/iceberg/pull/7121
--
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
[GitHub] [iceberg] sririshindra commented on a diff in pull request #7121: Spark: Parameterize backup suffix in migrate procedure
Posted by "sririshindra (via GitHub)" <gi...@apache.org>.
sririshindra commented on code in PR #7121:
URL: https://github.com/apache/iceberg/pull/7121#discussion_r1141194943
##########
spark/v3.3/spark-extensions/src/test/java/org/apache/iceberg/spark/extensions/TestMigrateTableProcedure.java:
##########
@@ -122,6 +122,38 @@ public void testMigrateWithDropBackup() throws IOException {
Assert.assertFalse(spark.catalog().tableExists(tableName + "_BACKUP_"));
}
+ @Test
+ public void testMigrateWithBackupSuffix() throws IOException {
+ Assume.assumeTrue(catalogName.equals("spark_catalog"));
+ String backupSuffix = "_tmp";
+ String location = temp.newFolder().toString();
+ sql(
+ "CREATE TABLE %s (id bigint NOT NULL, data string) USING parquet LOCATION '%s'",
+ tableName, location);
+ sql("INSERT INTO TABLE %s VALUES (1, 'a')", tableName);
+
+ Object result =
+ scalarSql(
+ "CALL %s.system.migrate(table => '%s', backup_suffix => '%s')",
Review Comment:
Maybe you could add a separate test for the following case as well.
```
"CALL %s.system.migrate(table => '%s', drop_backup => true, backup_suffix => '%s')"
```
Wouldn't hurt to test this combination as the code may change in the future and method chaining doesn't work as expected. it might be better to add a unit test now to ensure this doesn't cause any issues in the future. What do you think?
--
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
[GitHub] [iceberg] RussellSpitzer commented on pull request #7121: Spark: Parameterize backup suffix in migrate procedure
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on PR #7121:
URL: https://github.com/apache/iceberg/pull/7121#issuecomment-1478078090
I think you are just making the case for changing it to something like _iceberg_migration_backup_ or something like that. Still don't see why a user would want it to be different that a more descriptive suffix?
--
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
[GitHub] [iceberg] edgarRd commented on a diff in pull request #7121: Spark: Parameterize backup suffix in migrate procedure
Posted by "edgarRd (via GitHub)" <gi...@apache.org>.
edgarRd commented on code in PR #7121:
URL: https://github.com/apache/iceberg/pull/7121#discussion_r1142694741
##########
spark/v3.3/spark-extensions/src/test/java/org/apache/iceberg/spark/extensions/TestMigrateTableProcedure.java:
##########
@@ -122,6 +122,38 @@ public void testMigrateWithDropBackup() throws IOException {
Assert.assertFalse(spark.catalog().tableExists(tableName + "_BACKUP_"));
}
+ @Test
+ public void testMigrateWithBackupSuffix() throws IOException {
+ Assume.assumeTrue(catalogName.equals("spark_catalog"));
+ String backupSuffix = "_tmp";
+ String location = temp.newFolder().toString();
+ sql(
+ "CREATE TABLE %s (id bigint NOT NULL, data string) USING parquet LOCATION '%s'",
+ tableName, location);
+ sql("INSERT INTO TABLE %s VALUES (1, 'a')", tableName);
+
+ Object result =
+ scalarSql(
+ "CALL %s.system.migrate(table => '%s', backup_suffix => '%s')",
+ catalogName, tableName, backupSuffix);
+
+ Assert.assertEquals("Should have added one file", 1L, result);
+
+ Table createdTable = validationCatalog.loadTable(tableIdent);
+
+ String tableLocation = createdTable.location().replace("file:", "");
+ Assert.assertEquals("Table should have original location", location, tableLocation);
+
+ sql("INSERT INTO TABLE %s VALUES (1, 'a')", tableName);
+
+ assertEquals(
+ "Should have expected rows",
+ ImmutableList.of(row(1L, "a"), row(1L, "a")),
+ sql("SELECT * FROM %s ORDER BY id", tableName));
+
+ sql("DROP TABLE %s", tableName + backupSuffix);
Review Comment:
Sure, makes sense.
--
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
[GitHub] [iceberg] edgarRd commented on pull request #7121: Spark: Parameterize backup suffix in migrate procedure
Posted by "edgarRd (via GitHub)" <gi...@apache.org>.
edgarRd commented on PR #7121:
URL: https://github.com/apache/iceberg/pull/7121#issuecomment-1477998380
PTAL @rdblue @RussellSpitzer - Thanks!
--
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] Spark: Parameterize backup suffix in migrate procedure [iceberg]
Posted by "edgarRd (via GitHub)" <gi...@apache.org>.
edgarRd commented on PR #7121:
URL: https://github.com/apache/iceberg/pull/7121#issuecomment-1753919401
I suppose we were okay with changing the backup name on `migrate` after all: https://github.com/apache/iceberg/pull/8227/files - closing 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
[GitHub] [iceberg] edgarRd commented on a diff in pull request #7121: Spark: Parameterize backup suffix in migrate procedure
Posted by "edgarRd (via GitHub)" <gi...@apache.org>.
edgarRd commented on code in PR #7121:
URL: https://github.com/apache/iceberg/pull/7121#discussion_r1143529385
##########
spark/v3.3/spark-extensions/src/test/java/org/apache/iceberg/spark/extensions/TestMigrateTableProcedure.java:
##########
@@ -106,20 +108,41 @@ public void testMigrateWithOptions() throws IOException {
sql("DROP TABLE IF EXISTS %s", tableName + "_BACKUP_");
}
- @Test
- public void testMigrateWithDropBackup() throws IOException {
+ private void testMigrateWithDropBackup(@Nullable String backupSuffix) throws IOException {
Review Comment:
I looked in the code base and the annotation patter seems preferred: [core/src/main/java/org/apache/iceberg/puffin/PuffinCompressionCodec.java](https://github.com/apache/iceberg/blob/master/core/src/main/java/org/apache/iceberg/puffin/PuffinCompressionCodec.java#L62)
--
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
[GitHub] [iceberg] sririshindra commented on a diff in pull request #7121: Spark: Parameterize backup suffix in migrate procedure
Posted by "sririshindra (via GitHub)" <gi...@apache.org>.
sririshindra commented on code in PR #7121:
URL: https://github.com/apache/iceberg/pull/7121#discussion_r1141193297
##########
spark/v3.3/spark-extensions/src/test/java/org/apache/iceberg/spark/extensions/TestMigrateTableProcedure.java:
##########
@@ -122,6 +122,38 @@ public void testMigrateWithDropBackup() throws IOException {
Assert.assertFalse(spark.catalog().tableExists(tableName + "_BACKUP_"));
}
+ @Test
+ public void testMigrateWithBackupSuffix() throws IOException {
+ Assume.assumeTrue(catalogName.equals("spark_catalog"));
+ String backupSuffix = "_tmp";
+ String location = temp.newFolder().toString();
+ sql(
+ "CREATE TABLE %s (id bigint NOT NULL, data string) USING parquet LOCATION '%s'",
+ tableName, location);
+ sql("INSERT INTO TABLE %s VALUES (1, 'a')", tableName);
+
+ Object result =
+ scalarSql(
+ "CALL %s.system.migrate(table => '%s', backup_suffix => '%s')",
+ catalogName, tableName, backupSuffix);
+
+ Assert.assertEquals("Should have added one file", 1L, result);
+
+ Table createdTable = validationCatalog.loadTable(tableIdent);
+
+ String tableLocation = createdTable.location().replace("file:", "");
+ Assert.assertEquals("Table should have original location", location, tableLocation);
+
+ sql("INSERT INTO TABLE %s VALUES (1, 'a')", tableName);
+
+ assertEquals(
+ "Should have expected rows",
+ ImmutableList.of(row(1L, "a"), row(1L, "a")),
Review Comment:
Can you make this (ImmutableList.of(row(1L, "a"), row(2L, "b"))). Otherwise there is no need to do "ORDER BY id" in the next line since both ids are same.
##########
spark/v3.3/spark-extensions/src/test/java/org/apache/iceberg/spark/extensions/TestMigrateTableProcedure.java:
##########
@@ -122,6 +122,38 @@ public void testMigrateWithDropBackup() throws IOException {
Assert.assertFalse(spark.catalog().tableExists(tableName + "_BACKUP_"));
}
+ @Test
+ public void testMigrateWithBackupSuffix() throws IOException {
+ Assume.assumeTrue(catalogName.equals("spark_catalog"));
+ String backupSuffix = "_tmp";
+ String location = temp.newFolder().toString();
+ sql(
+ "CREATE TABLE %s (id bigint NOT NULL, data string) USING parquet LOCATION '%s'",
+ tableName, location);
+ sql("INSERT INTO TABLE %s VALUES (1, 'a')", tableName);
+
+ Object result =
+ scalarSql(
+ "CALL %s.system.migrate(table => '%s', backup_suffix => '%s')",
+ catalogName, tableName, backupSuffix);
+
+ Assert.assertEquals("Should have added one file", 1L, result);
+
+ Table createdTable = validationCatalog.loadTable(tableIdent);
+
+ String tableLocation = createdTable.location().replace("file:", "");
+ Assert.assertEquals("Table should have original location", location, tableLocation);
+
+ sql("INSERT INTO TABLE %s VALUES (1, 'a')", tableName);
Review Comment:
nit: Can you make the value (2, 'b')
##########
spark/v3.3/spark-extensions/src/test/java/org/apache/iceberg/spark/extensions/TestMigrateTableProcedure.java:
##########
@@ -122,6 +122,38 @@ public void testMigrateWithDropBackup() throws IOException {
Assert.assertFalse(spark.catalog().tableExists(tableName + "_BACKUP_"));
}
+ @Test
+ public void testMigrateWithBackupSuffix() throws IOException {
+ Assume.assumeTrue(catalogName.equals("spark_catalog"));
+ String backupSuffix = "_tmp";
+ String location = temp.newFolder().toString();
+ sql(
+ "CREATE TABLE %s (id bigint NOT NULL, data string) USING parquet LOCATION '%s'",
+ tableName, location);
+ sql("INSERT INTO TABLE %s VALUES (1, 'a')", tableName);
+
+ Object result =
+ scalarSql(
+ "CALL %s.system.migrate(table => '%s', backup_suffix => '%s')",
Review Comment:
Maybe you could add a separate test that tests the following
"CALL %s.system.migrate(table => '%s', drop_backup => true, backup_suffix => '%s')"
Wouldn't hurt to test this combination as the code may change in the future and method chaining doesn't work as expected. it might be better to add a unit test to ensure this doesn't cause any issues. What do you think?
##########
spark/v3.3/spark-extensions/src/test/java/org/apache/iceberg/spark/extensions/TestMigrateTableProcedure.java:
##########
@@ -122,6 +122,38 @@ public void testMigrateWithDropBackup() throws IOException {
Assert.assertFalse(spark.catalog().tableExists(tableName + "_BACKUP_"));
}
+ @Test
+ public void testMigrateWithBackupSuffix() throws IOException {
+ Assume.assumeTrue(catalogName.equals("spark_catalog"));
+ String backupSuffix = "_tmp";
+ String location = temp.newFolder().toString();
+ sql(
+ "CREATE TABLE %s (id bigint NOT NULL, data string) USING parquet LOCATION '%s'",
+ tableName, location);
+ sql("INSERT INTO TABLE %s VALUES (1, 'a')", tableName);
+
+ Object result =
+ scalarSql(
+ "CALL %s.system.migrate(table => '%s', backup_suffix => '%s')",
+ catalogName, tableName, backupSuffix);
+
+ Assert.assertEquals("Should have added one file", 1L, result);
+
+ Table createdTable = validationCatalog.loadTable(tableIdent);
+
+ String tableLocation = createdTable.location().replace("file:", "");
+ Assert.assertEquals("Table should have original location", location, tableLocation);
+
+ sql("INSERT INTO TABLE %s VALUES (1, 'a')", tableName);
+
+ assertEquals(
+ "Should have expected rows",
+ ImmutableList.of(row(1L, "a"), row(1L, "a")),
+ sql("SELECT * FROM %s ORDER BY id", tableName));
+
+ sql("DROP TABLE %s", tableName + backupSuffix);
Review Comment:
Before you drop the backup table here, can you also assert that the data in the backup table remains intact after the migration i.e can you do something like
assertEquals(
"Should have expected rows",
ImmutableList.of(row(1L, "a")),
sql("SELECT * FROM %s ORDER BY id", tableName + backupSuffix));
##########
spark/v3.3/spark/src/main/java/org/apache/iceberg/spark/procedures/MigrateTableProcedure.java:
##########
@@ -91,9 +92,13 @@ public InternalRow[] call(InternalRow args) {
}
boolean dropBackup = args.isNullAt(2) ? false : args.getBoolean(2);
+ String backupSuffix = args.isNullAt(3) ? null : args.getString(3);
MigrateTableSparkAction migrateTableSparkAction =
SparkActions.get().migrateTable(tableName).tableProperties(properties);
+ if (backupSuffix != null) {
Review Comment:
nit: newline before this.
##########
spark/v3.3/spark/src/main/java/org/apache/iceberg/spark/procedures/MigrateTableProcedure.java:
##########
@@ -91,9 +92,13 @@ public InternalRow[] call(InternalRow args) {
}
boolean dropBackup = args.isNullAt(2) ? false : args.getBoolean(2);
+ String backupSuffix = args.isNullAt(3) ? null : args.getString(3);
MigrateTableSparkAction migrateTableSparkAction =
SparkActions.get().migrateTable(tableName).tableProperties(properties);
+ if (backupSuffix != null) {
+ migrateTableSparkAction = migrateTableSparkAction.withBackupSuffix(backupSuffix);
+ }
MigrateTable.Result result;
Review Comment:
Can you also change this to the following to match the style above
if (dropBackup) {
migrateTableSparkAction = migrateTableSparkAction.dropBackup()
}
MigrateTable.Result result = migrateTableSparkAction.execute();
--
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
[GitHub] [iceberg] sririshindra commented on a diff in pull request #7121: Spark: Parameterize backup suffix in migrate procedure
Posted by "sririshindra (via GitHub)" <gi...@apache.org>.
sririshindra commented on code in PR #7121:
URL: https://github.com/apache/iceberg/pull/7121#discussion_r1141193297
##########
spark/v3.3/spark-extensions/src/test/java/org/apache/iceberg/spark/extensions/TestMigrateTableProcedure.java:
##########
@@ -122,6 +122,38 @@ public void testMigrateWithDropBackup() throws IOException {
Assert.assertFalse(spark.catalog().tableExists(tableName + "_BACKUP_"));
}
+ @Test
+ public void testMigrateWithBackupSuffix() throws IOException {
+ Assume.assumeTrue(catalogName.equals("spark_catalog"));
+ String backupSuffix = "_tmp";
+ String location = temp.newFolder().toString();
+ sql(
+ "CREATE TABLE %s (id bigint NOT NULL, data string) USING parquet LOCATION '%s'",
+ tableName, location);
+ sql("INSERT INTO TABLE %s VALUES (1, 'a')", tableName);
+
+ Object result =
+ scalarSql(
+ "CALL %s.system.migrate(table => '%s', backup_suffix => '%s')",
+ catalogName, tableName, backupSuffix);
+
+ Assert.assertEquals("Should have added one file", 1L, result);
+
+ Table createdTable = validationCatalog.loadTable(tableIdent);
+
+ String tableLocation = createdTable.location().replace("file:", "");
+ Assert.assertEquals("Table should have original location", location, tableLocation);
+
+ sql("INSERT INTO TABLE %s VALUES (1, 'a')", tableName);
+
+ assertEquals(
+ "Should have expected rows",
+ ImmutableList.of(row(1L, "a"), row(1L, "a")),
Review Comment:
Can you make this `(ImmutableList.of(row(1L, "a"), row(2L, "b")))`. Otherwise there is no need to do "ORDER BY id" in the next line since both ids are same.
--
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
[GitHub] [iceberg] sririshindra commented on a diff in pull request #7121: Spark: Parameterize backup suffix in migrate procedure
Posted by "sririshindra (via GitHub)" <gi...@apache.org>.
sririshindra commented on code in PR #7121:
URL: https://github.com/apache/iceberg/pull/7121#discussion_r1141196730
##########
spark/v3.3/spark/src/main/java/org/apache/iceberg/spark/procedures/MigrateTableProcedure.java:
##########
@@ -91,9 +92,13 @@ public InternalRow[] call(InternalRow args) {
}
boolean dropBackup = args.isNullAt(2) ? false : args.getBoolean(2);
+ String backupSuffix = args.isNullAt(3) ? null : args.getString(3);
MigrateTableSparkAction migrateTableSparkAction =
SparkActions.get().migrateTable(tableName).tableProperties(properties);
+ if (backupSuffix != null) {
+ migrateTableSparkAction = migrateTableSparkAction.withBackupSuffix(backupSuffix);
+ }
MigrateTable.Result result;
Review Comment:
Can you also change this to the following to match the style above
if (dropBackup) {
migrateTableSparkAction = migrateTableSparkAction.dropBackup()
}
MigrateTable.Result result = migrateTableSparkAction.execute();
--
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