You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "slfan1989 (via GitHub)" <gi...@apache.org> on 2025/08/17 03:04:03 UTC
[PR] Add 'skip_file_list' option to RewriteTablePathProcedure for optional file-list generation. [iceberg]
slfan1989 opened a new pull request, #13837:
URL: https://github.com/apache/iceberg/pull/13837
(no 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] API, Spark 4.0: Add 'skip_file_list' option to RewriteTablePathProcedure for optional file-list generation. [iceberg]
Posted by "dramaticlly (via GitHub)" <gi...@apache.org>.
dramaticlly commented on code in PR #13837:
URL: https://github.com/apache/iceberg/pull/13837#discussion_r2302720310
##########
open-api/src/testFixtures/java/org/apache/iceberg/rest/RESTCatalogServer.java:
##########
@@ -86,7 +86,7 @@ private CatalogContext initializeBackendCatalog() throws IOException {
if (warehouseLocation == null) {
File tmp = java.nio.file.Files.createTempDirectory("iceberg_warehouse").toFile();
tmp.deleteOnExit();
- warehouseLocation = new File(tmp, "iceberg_data").getAbsolutePath();
+ warehouseLocation = new File(tmp, "iceberg_data").toURI().toString();
Review Comment:
can we remove the change from this patch and let's use #13882 to fix testFixture?
--
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] API, Spark 4.0: Add 'skip_file_list' option to RewriteTablePathProcedure for optional file-list generation. [iceberg]
Posted by "slfan1989 (via GitHub)" <gi...@apache.org>.
slfan1989 commented on PR #13837:
URL: https://github.com/apache/iceberg/pull/13837#issuecomment-3226460844
@dramaticlly @szehon-ho Could you please review this PR again? Thanks a lot! The issues you pointed out have been improved.
cc: @huaxingao
--
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] API, Spark 4.0: Add 'skip_file_list' option to RewriteTablePathProcedure for optional file-list generation. [iceberg]
Posted by "slfan1989 (via GitHub)" <gi...@apache.org>.
slfan1989 commented on code in PR #13837:
URL: https://github.com/apache/iceberg/pull/13837#discussion_r2312157068
##########
open-api/src/testFixtures/java/org/apache/iceberg/rest/RESTCatalogServer.java:
##########
@@ -86,7 +86,7 @@ private CatalogContext initializeBackendCatalog() throws IOException {
if (warehouseLocation == null) {
File tmp = java.nio.file.Files.createTempDirectory("iceberg_warehouse").toFile();
tmp.deleteOnExit();
- warehouseLocation = new File(tmp, "iceberg_data").getAbsolutePath();
+ warehouseLocation = new File(tmp, "iceberg_data").toURI().toString();
Review Comment:
@dramaticlly Thank you for your suggestion, which gave me some insights. I have added a logic in the unit tests for the REST Catalog. When encountering REST Catalog, it will use getAbsolutePath to obtain the path for deleting the file. After local testing, the solution works well, and I have added comments to explain the issue.
```
if (SparkCatalogConfig.REST.catalogName().equals(catalogName)) {
// We applied this special handling because the base path for
// matching the RESTCATALOG's Hive BaseLocation is represented
// in the form of an AbsolutePath.
filePath = file.getAbsolutePath().toString();
}
```
--
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] API, Spark 4.0: Add 'skip_file_list' option to RewriteTablePathProcedure for optional file-list generation. [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #13837:
URL: https://github.com/apache/iceberg/pull/13837#discussion_r2290281341
##########
spark/v4.0/spark/src/main/java/org/apache/iceberg/spark/procedures/RewriteTablePathProcedure.java:
##########
@@ -58,14 +60,19 @@ public class RewriteTablePathProcedure extends BaseProcedure {
TARGET_PREFIX_PARAM,
START_VERSION_PARAM,
END_VERSION_PARM,
- STAGING_LOCATION_PARAM
+ STAGING_LOCATION_PARAM,
+ SKIP_FILE_LIST_PARAM
};
private static final StructType OUTPUT_TYPE =
new StructType(
new StructField[] {
new StructField("latest_version", DataTypes.StringType, true, Metadata.empty()),
- new StructField("file_list_location", DataTypes.StringType, true, Metadata.empty())
+ new StructField("file_list_location", DataTypes.StringType, true, Metadata.empty()),
+ new StructField(
+ "rewrite_metadata_files_count", DataTypes.IntegerType, true, Metadata.empty()),
Review Comment:
```suggestion
"rewritten_metadata_files_count", DataTypes.IntegerType, true, Metadata.empty()),
```
##########
spark/v4.0/spark/src/main/java/org/apache/iceberg/spark/procedures/RewriteTablePathProcedure.java:
##########
@@ -58,14 +60,19 @@ public class RewriteTablePathProcedure extends BaseProcedure {
TARGET_PREFIX_PARAM,
START_VERSION_PARAM,
END_VERSION_PARM,
- STAGING_LOCATION_PARAM
+ STAGING_LOCATION_PARAM,
+ SKIP_FILE_LIST_PARAM
};
private static final StructType OUTPUT_TYPE =
new StructType(
new StructField[] {
new StructField("latest_version", DataTypes.StringType, true, Metadata.empty()),
- new StructField("file_list_location", DataTypes.StringType, true, Metadata.empty())
+ new StructField("file_list_location", DataTypes.StringType, true, Metadata.empty()),
+ new StructField(
+ "rewrite_metadata_files_count", DataTypes.IntegerType, true, Metadata.empty()),
+ new StructField(
+ "rewrite_delete_files_count", DataTypes.IntegerType, true, Metadata.empty())
Review Comment:
```suggestion
"rewritten_delete_files_count", DataTypes.IntegerType, true, Metadata.empty())
```
--
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] API, Spark 4.0: Add create_file_list option to RewriteTablePathProcedure. [iceberg]
Posted by "slfan1989 (via GitHub)" <gi...@apache.org>.
slfan1989 commented on code in PR #13837:
URL: https://github.com/apache/iceberg/pull/13837#discussion_r2362784984
##########
spark/v4.0/spark/src/main/java/org/apache/iceberg/spark/actions/RewriteTablePathSparkAction.java:
##########
@@ -300,12 +304,28 @@ private String rebuildMetadata() {
.collect(Collectors.toSet());
rewritePositionDeletes(deleteFiles);
+ ImmutableRewriteTablePath.Result.Builder builder =
+ ImmutableRewriteTablePath.Result.builder()
+ .stagingLocation(stagingDir)
+ .rewrittenDeleteFilePathsCount(deleteFiles.size())
+ .rewrittenManifestFilePathsCount(metaFiles.size())
+ .latestVersion(RewriteTablePathUtil.fileName(endVersionName));
+
+ if (!createFileList) {
+ return builder.fileListLocation(NOT_APPLICABLE).build();
+ }
+
Set<Pair<String, String>> copyPlan = Sets.newHashSet();
copyPlan.addAll(rewriteVersionResult.copyPlan());
copyPlan.addAll(rewriteManifestListResult.copyPlan());
copyPlan.addAll(rewriteManifestResult.copyPlan());
+ String fileListLocation = saveFileList(copyPlan);
- return saveFileList(copyPlan);
+ return builder
+ .stagingLocation(stagingDir)
+ .fileListLocation(fileListLocation)
+ .latestVersion(RewriteTablePathUtil.fileName(endVersionName))
Review Comment:
Apologies for the misunderstanding, and thank you for your feedback. You are correct that this code is redundant, as the relevant variables are already set in line 309, and we don't need to set them again.
--
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] API, Spark 4.0: Add 'skip_file_list' option to RewriteTablePathProcedure for optional file-list generation. [iceberg]
Posted by "slfan1989 (via GitHub)" <gi...@apache.org>.
slfan1989 commented on PR #13837:
URL: https://github.com/apache/iceberg/pull/13837#issuecomment-3212901600
@nastra @dramaticlly Thank you for reviewing the code! Your suggestions have been very helpful to me. I have optimized the code based on both of your suggestions. Could you please take another look? Thank you so much!
--
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] API, Spark 4.0: Add 'skip_file_list' option to RewriteTablePathProcedure for optional file-list generation. [iceberg]
Posted by "slfan1989 (via GitHub)" <gi...@apache.org>.
slfan1989 commented on code in PR #13837:
URL: https://github.com/apache/iceberg/pull/13837#discussion_r2292598195
##########
spark/v4.0/spark/src/main/java/org/apache/iceberg/spark/actions/RewriteTablePathSparkAction.java:
##########
@@ -304,12 +307,29 @@ private String rebuildMetadata() {
.collect(Collectors.toSet());
rewritePositionDeletes(deleteFiles);
+ ImmutableRewriteTablePath.Result.Builder builder =
+ ImmutableRewriteTablePath.Result.builder()
+ .stagingLocation(stagingDir)
+ .rewrittenDeleteFilesCount(deleteFiles.size())
+ .rewrittenManifestFilesCount(metaFiles.size())
+ .latestVersion(RewriteTablePathUtil.fileName(endVersionName));
+
+ // skip file list
+ if (skipFileList) {
+ return builder.build();
+ }
+
Set<Pair<String, String>> copyPlan = Sets.newHashSet();
copyPlan.addAll(rewriteVersionResult.copyPlan());
copyPlan.addAll(rewriteManifestListResult.copyPlan());
copyPlan.addAll(rewriteManifestResult.copyPlan());
+ String fileListLocation = saveFileList(copyPlan);
- return saveFileList(copyPlan);
+ return builder
+ .stagingLocation(stagingDir)
+ .fileListLocation(fileListLocation)
Review Comment:
@dramaticlly
I checked the code based on nastra’s suggestion and confirmed that his analysis is correct. I also verified it locally: if fileListLocation is not set, an exception will indeed be thrown.
```
Cannot build Result, some of required attributes are not set [fileListLocation]
java.lang.IllegalStateException: Cannot build Result, some of required attributes are not set [fileListLocation]
at org.apache.iceberg.actions.ImmutableRewriteTablePath$Result$Builder.build(ImmutableRewriteTablePath.java:514)
```
I’ve been thinking—if fileListLocation is empty, can we simply treat it as `N/A`? My idea is to assign `N/A` directly during the check, without changing the original definition. Do you think this approach would work?
--
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] API, Spark 4.0: Add 'skip_file_list' option to RewriteTablePathProcedure for optional file-list generation. [iceberg]
Posted by "slfan1989 (via GitHub)" <gi...@apache.org>.
slfan1989 commented on code in PR #13837:
URL: https://github.com/apache/iceberg/pull/13837#discussion_r2308866081
##########
open-api/src/testFixtures/java/org/apache/iceberg/rest/RESTCatalogServer.java:
##########
@@ -86,7 +86,7 @@ private CatalogContext initializeBackendCatalog() throws IOException {
if (warehouseLocation == null) {
File tmp = java.nio.file.Files.createTempDirectory("iceberg_warehouse").toFile();
tmp.deleteOnExit();
- warehouseLocation = new File(tmp, "iceberg_data").getAbsolutePath();
+ warehouseLocation = new File(tmp, "iceberg_data").toURI().toString();
Review Comment:
@dramaticlly Thank you for your message! I have carefully readed the relevant code and confirmed that this change is necessary. The reason is that the base path for the Hive library in RESTCatalog is inconsistent with other types of Catalogs. Currently, HIVE, HADOOP, and SPARK_SESSION all use URI format, so RESTCatalog needs to be adjusted to the same format. Additionally, since the TestRewriteTablePathProcedure test involves parameter type testing and simultaneously tests four different Catalogs, we must standardize the base path of the Hive library across all four Catalogs to URI format in order to conduct the test successfully. Otherwise, there could be issues with prefix validation failures.
```
Path file:/var/folders/2k/21gv5vmx6z7dlr1g0_jtm8r80000ks/T/iceberg_warehouse10689209309257803697/iceberg_data/default/table/data/deletes.parquet does not start with /var/folders/2k/21gv5vmx6z7dlr1g0_jtm8r80000ks/T/iceberg_warehouse10689209309257803697/iceberg_data/default/table/
java.lang.IllegalArgumentException: Path file:/var/folders/2k/21gv5vmx6z7dlr1g0_jtm8r80000ks/T/iceberg_warehouse10689209309257803697/iceberg_data/default/table/data/deletes.parquet does not start with /var/folders/2k/21gv5vmx6z7dlr1g0_jtm8r80000ks/T/iceberg_warehouse10689209309257803697/iceberg_data/default/table/
at org.apache.iceberg.RewriteTablePathUtil.relativize(RewriteTablePathUtil.java:686)
.....
```
I have provided a detailed description in #13882 and hope someone from the team can assist in reviewing this issue.
cc: @nastra @manuzhang @huaxingao
--
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] API, Spark 4.0: Add 'skip_file_list' option to RewriteTablePathProcedure for optional file-list generation. [iceberg]
Posted by "slfan1989 (via GitHub)" <gi...@apache.org>.
slfan1989 commented on code in PR #13837:
URL: https://github.com/apache/iceberg/pull/13837#discussion_r2354309389
##########
api/src/main/java/org/apache/iceberg/actions/RewriteTablePath.java:
##########
@@ -112,5 +125,15 @@ interface Result {
/** Name of latest metadata file version */
String latestVersion();
+
+ /** Number of delete files with rewritten paths. */
+ default int deleteFilesPathRewrittenCount() {
Review Comment:
Thank you for reviewing the code. I’ve made improvements based on your suggestions.
--
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] API, Spark 4.0: Add 'skip_file_list' option to RewriteTablePathProcedure for optional file-list generation. [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #13837:
URL: https://github.com/apache/iceberg/pull/13837#discussion_r2290272750
##########
open-api/src/testFixtures/java/org/apache/iceberg/rest/RESTCatalogServer.java:
##########
@@ -86,7 +86,7 @@ private CatalogContext initializeBackendCatalog() throws IOException {
if (warehouseLocation == null) {
File tmp = java.nio.file.Files.createTempDirectory("iceberg_warehouse").toFile();
tmp.deleteOnExit();
- warehouseLocation = new File(tmp, "iceberg_data").getAbsolutePath();
+ warehouseLocation = new File(tmp, "iceberg_data").toURI().toString();
Review Comment:
what error is this causing? Can you please paste a stack trace?
--
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] API, Spark 4.0: Add 'skip_file_list' option to RewriteTablePathProcedure for optional file-list generation. [iceberg]
Posted by "slfan1989 (via GitHub)" <gi...@apache.org>.
slfan1989 commented on code in PR #13837:
URL: https://github.com/apache/iceberg/pull/13837#discussion_r2314255027
##########
open-api/src/testFixtures/java/org/apache/iceberg/rest/RESTCatalogServer.java:
##########
@@ -86,7 +86,7 @@ private CatalogContext initializeBackendCatalog() throws IOException {
if (warehouseLocation == null) {
File tmp = java.nio.file.Files.createTempDirectory("iceberg_warehouse").toFile();
tmp.deleteOnExit();
- warehouseLocation = new File(tmp, "iceberg_data").getAbsolutePath();
+ warehouseLocation = new File(tmp, "iceberg_data").toURI().toString();
Review Comment:
@dramaticlly Can I make this modification? I would like to hear your opinion. Thank you!
--
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] API, Spark 4.0: Add 'skip_file_list' option to RewriteTablePathProcedure for optional file-list generation. [iceberg]
Posted by "slfan1989 (via GitHub)" <gi...@apache.org>.
slfan1989 commented on code in PR #13837:
URL: https://github.com/apache/iceberg/pull/13837#discussion_r2317540772
##########
spark/v4.0/spark/src/main/java/org/apache/iceberg/spark/actions/RewriteTablePathSparkAction.java:
##########
@@ -90,12 +90,14 @@ public class RewriteTablePathSparkAction extends BaseSparkAction<RewriteTablePat
private static final Logger LOG = LoggerFactory.getLogger(RewriteTablePathSparkAction.class);
private static final String RESULT_LOCATION = "file-list";
+ public static final String NOT_APPLICABLE = "N/A";
Review Comment:
Thank you for your suggestion! I have made improvements based on your feedback.
--
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] API, Spark 4.0: Add 'skip_file_list' option to RewriteTablePathProcedure for optional file-list generation. [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #13837:
URL: https://github.com/apache/iceberg/pull/13837#discussion_r2290274514
##########
spark/v4.0/spark-extensions/src/test/java/org/apache/iceberg/spark/extensions/TestRewriteTablePathProcedure.java:
##########
@@ -171,8 +177,79 @@ public void testProcedureWithInvalidInput() {
"Cannot find provided version file %s in metadata log.", "v11.metadata.json");
}
+ @TestTemplate
+ public void testRewriteTablePathWithSkipFileList() {
+ String location = targetTableDir.toFile().toURI().toString();
+ Table table = validationCatalog.loadTable(tableIdent);
+ String metadataJson = TableUtil.metadataFileLocation(table);
+
+ List<Object[]> result =
+ sql(
+ "CALL %s.system.rewrite_table_path(table => '%s', source_prefix => '%s', target_prefix => '%s', skip_file_list => true)",
+ catalogName, tableIdent, table.location(), location);
+ assertThat(result).hasSize(1);
+ assertThat(result.get(0)[0])
+ .as("Should return correct latest version")
+ .isEqualTo(RewriteTablePathUtil.fileName(metadataJson));
+ assertThat(result.get(0)[1]).as("Should return empty").asString().isEqualTo("");
+ }
+
private void checkFileListLocationCount(String fileListLocation, long expectedFileCount) {
long fileCount = spark.read().format("text").load(fileListLocation).count();
assertThat(fileCount).isEqualTo(expectedFileCount);
}
+
+ @TestTemplate
+ public void testRewriteTablePathWithManifestAndDeleteCounts() throws IOException {
+
Review Comment:
unnecessary newline
--
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] API, Spark 4.0: Add 'skip_file_list' option to RewriteTablePathProcedure for optional file-list generation. [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #13837:
URL: https://github.com/apache/iceberg/pull/13837#discussion_r2292638286
##########
api/src/main/java/org/apache/iceberg/actions/RewriteTablePath.java:
##########
@@ -112,5 +122,15 @@ interface Result {
/** Name of latest metadata file version */
String latestVersion();
+
+ /** count of rewrite delete files, default value is 0 */
+ default int rewrittenDeleteFilesCount() {
Review Comment:
I think that makes sense with a slight update to the naming, so maybe `rewrittenDeleteFilePathsCount` and `rewrittenManifestFilePathsCount`. @szehon-ho do you think that makes sense here?
--
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] API, Spark 4.0: Add 'skip_file_list' option to RewriteTablePathProcedure for optional file-list generation. [iceberg]
Posted by "slfan1989 (via GitHub)" <gi...@apache.org>.
slfan1989 commented on code in PR #13837:
URL: https://github.com/apache/iceberg/pull/13837#discussion_r2292601507
##########
api/src/main/java/org/apache/iceberg/actions/RewriteTablePath.java:
##########
@@ -112,5 +122,15 @@ interface Result {
/** Name of latest metadata file version */
String latestVersion();
+
+ /** count of rewrite delete files, default value is 0 */
+ default int rewrittenDeleteFilesCount() {
Review Comment:
I think your suggestion is reasonable. Would it be fine to name them `deleteFilesPathRewrittenCount` and `manifestFilesPathRewrittenCount`? This naming would accurately reflect that only the paths are being rewritten, rather than the files themselves being merged or compacted.
--
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] API, Spark 4.0: Add 'skip_file_list' option to RewriteTablePathProcedure for optional file-list generation. [iceberg]
Posted by "slfan1989 (via GitHub)" <gi...@apache.org>.
slfan1989 commented on code in PR #13837:
URL: https://github.com/apache/iceberg/pull/13837#discussion_r2354314625
##########
spark/v4.0/spark-extensions/src/test/java/org/apache/iceberg/spark/extensions/TestRewriteTablePathProcedure.java:
##########
@@ -171,8 +178,92 @@ public void testProcedureWithInvalidInput() {
"Cannot find provided version file %s in metadata log.", "v11.metadata.json");
}
+ @TestTemplate
+ public void testRewriteTablePathWithoutFileList() {
+ String location = targetTableDir.toFile().toURI().toString();
+ Table table = validationCatalog.loadTable(tableIdent);
+ String metadataJson = TableUtil.metadataFileLocation(table);
+
+ List<Object[]> result =
+ sql(
+ "CALL %s.system.rewrite_table_path(table => '%s', source_prefix => '%s', target_prefix => '%s', create_file_list => false)",
+ catalogName, tableIdent, table.location(), location);
+ assertThat(result).hasSize(1);
+ assertThat(result.get(0)[0])
+ .as("Should return correct latest version")
+ .isEqualTo(RewriteTablePathUtil.fileName(metadataJson));
+ assertThat(result.get(0)[1])
+ .as("Check if file list location is correctly marked as N/A when not generated")
+ .asString()
+ .isEqualTo("N/A");
+ }
+
private void checkFileListLocationCount(String fileListLocation, long expectedFileCount) {
long fileCount = spark.read().format("text").load(fileListLocation).count();
assertThat(fileCount).isEqualTo(expectedFileCount);
}
+
+ @TestTemplate
+ public void testRewriteTablePathWithManifestAndDeleteCounts() throws IOException {
+ sql("INSERT INTO %s VALUES (1, 'a')", tableName);
+ sql("INSERT INTO %s VALUES (2, 'b')", tableName);
+ sql("INSERT INTO %s VALUES (3, 'c')", tableName);
+
+ Table table = validationCatalog.loadTable(tableIdent);
+ List<Pair<CharSequence, Long>> rowsToDelete =
+ Lists.newArrayList(
+ Pair.of(
+ table.currentSnapshot().addedDataFiles(table.io()).iterator().next().location(),
+ 0L));
+
+ File file = new File(removePrefix(table.location()) + "/data/deletes.parquet");
+ String filePath = file.toURI().toString();
+ if (SparkCatalogConfig.REST.catalogName().equals(catalogName)) {
+ // We applied this special handling because the base path for
+ // matching the RESTCATALOG's Hive BaseLocation is represented
+ // in the form of an AbsolutePath.
+ filePath = file.getAbsolutePath().toString();
+ }
+ DeleteFile positionDeletes =
Review Comment:
I have improved this part of the code.
--
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] API, Spark 4.0: Add 'skip_file_list' option to RewriteTablePathProcedure for optional file-list generation. [iceberg]
Posted by "slfan1989 (via GitHub)" <gi...@apache.org>.
slfan1989 commented on code in PR #13837:
URL: https://github.com/apache/iceberg/pull/13837#discussion_r2356085664
##########
spark/v4.0/spark/src/main/java/org/apache/iceberg/spark/actions/RewriteTablePathSparkAction.java:
##########
@@ -300,12 +304,28 @@ private String rebuildMetadata() {
.collect(Collectors.toSet());
rewritePositionDeletes(deleteFiles);
+ ImmutableRewriteTablePath.Result.Builder builder =
+ ImmutableRewriteTablePath.Result.builder()
+ .stagingLocation(stagingDir)
+ .rewrittenDeleteFilePathsCount(deleteFiles.size())
+ .rewrittenManifestFilePathsCount(metaFiles.size())
+ .latestVersion(RewriteTablePathUtil.fileName(endVersionName));
+
+ if (!createFileList) {
+ return builder.fileListLocation(NOT_APPLICABLE).build();
+ }
+
Set<Pair<String, String>> copyPlan = Sets.newHashSet();
copyPlan.addAll(rewriteVersionResult.copyPlan());
copyPlan.addAll(rewriteManifestListResult.copyPlan());
copyPlan.addAll(rewriteManifestResult.copyPlan());
+ String fileListLocation = saveFileList(copyPlan);
- return saveFileList(copyPlan);
+ return builder
+ .stagingLocation(stagingDir)
+ .fileListLocation(fileListLocation)
+ .latestVersion(RewriteTablePathUtil.fileName(endVersionName))
Review Comment:
Thanks for the question! The code isn’t duplicated — we moved the doExecute logic into rebuildMetadata so that fileListLocation and other Builder variables can be set in one place.
--
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] API, Spark 4.0: Add 'skip_file_list' option to RewriteTablePathProcedure for optional file-list generation. [iceberg]
Posted by "dramaticlly (via GitHub)" <gi...@apache.org>.
dramaticlly commented on code in PR #13837:
URL: https://github.com/apache/iceberg/pull/13837#discussion_r2316661941
##########
spark/v4.0/spark/src/main/java/org/apache/iceberg/spark/actions/RewriteTablePathSparkAction.java:
##########
@@ -90,12 +90,14 @@ public class RewriteTablePathSparkAction extends BaseSparkAction<RewriteTablePat
private static final Logger LOG = LoggerFactory.getLogger(RewriteTablePathSparkAction.class);
private static final String RESULT_LOCATION = "file-list";
+ public static final String NOT_APPLICABLE = "N/A";
Review Comment:
I think we can make it package private since this is only used in unit tests
--
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] API, Spark 4.0: Add 'skip_file_list' option to RewriteTablePathProcedure for optional file-list generation. [iceberg]
Posted by "slfan1989 (via GitHub)" <gi...@apache.org>.
slfan1989 commented on PR #13837:
URL: https://github.com/apache/iceberg/pull/13837#issuecomment-3264782026
@nastra Could you please take another look at this PR? Thank you very much!
--
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] API, Spark 4.0: Add 'skip_file_list' option to RewriteTablePathProcedure for optional file-list generation. [iceberg]
Posted by "slfan1989 (via GitHub)" <gi...@apache.org>.
slfan1989 commented on code in PR #13837:
URL: https://github.com/apache/iceberg/pull/13837#discussion_r2295066175
##########
spark/v4.0/spark/src/main/java/org/apache/iceberg/spark/actions/RewriteTablePathSparkAction.java:
##########
@@ -304,12 +307,29 @@ private String rebuildMetadata() {
.collect(Collectors.toSet());
rewritePositionDeletes(deleteFiles);
+ ImmutableRewriteTablePath.Result.Builder builder =
+ ImmutableRewriteTablePath.Result.builder()
+ .stagingLocation(stagingDir)
+ .deleteFilesPathRewrittenCount(deleteFiles.size())
+ .manifestFilesPathRewrittenCount(metaFiles.size())
+ .latestVersion(RewriteTablePathUtil.fileName(endVersionName));
+
+ // file list generation disabled
+ if (!fileListEnabled) {
+ return builder.fileListLocation("N/A").build();
Review Comment:
Thank you for the suggestion! I have extracted a variable to represent 'N/A'.
--
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] API, Spark 4.0: Add 'skip_file_list' option to RewriteTablePathProcedure for optional file-list generation. [iceberg]
Posted by "slfan1989 (via GitHub)" <gi...@apache.org>.
slfan1989 commented on code in PR #13837:
URL: https://github.com/apache/iceberg/pull/13837#discussion_r2287498931
##########
api/src/main/java/org/apache/iceberg/actions/RewriteTablePath.java:
##########
@@ -112,5 +122,15 @@ interface Result {
/** Name of latest metadata file version */
String latestVersion();
+
+ /** count of rewrite delete files, default value is 0 */
+ default int deleteFilesCount() {
+ return 0;
+ }
+
+ /** count of rewrite metadata files involved, default value is 0 */
+ default int metadataFilesCount() {
+ return 0;
Review Comment:
> I think rewrite metadata files can be confused with metadata.json files, if this is count of rewritten manifest files. let's call it rewrittenManifestFilesCount? Same wise for rewrittenDeleteFilesCount above.
Your suggestions are very valuable, and I am working on making improvements based on your feedback.
--
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] API, Spark 4.0: Add 'skip_file_list' option to RewriteTablePathProcedure for optional file-list generation. [iceberg]
Posted by "dramaticlly (via GitHub)" <gi...@apache.org>.
dramaticlly commented on code in PR #13837:
URL: https://github.com/apache/iceberg/pull/13837#discussion_r2312046325
##########
spark/v4.0/spark-extensions/src/test/java/org/apache/iceberg/spark/extensions/TestRewriteTablePathProcedure.java:
##########
@@ -171,8 +177,86 @@ public void testProcedureWithInvalidInput() {
"Cannot find provided version file %s in metadata log.", "v11.metadata.json");
}
+ @TestTemplate
+ public void testRewriteTablePathWithoutFileList() {
+ String location = targetTableDir.toFile().toURI().toString();
+ Table table = validationCatalog.loadTable(tableIdent);
+ String metadataJson = TableUtil.metadataFileLocation(table);
+
+ List<Object[]> result =
+ sql(
+ "CALL %s.system.rewrite_table_path(table => '%s', source_prefix => '%s', target_prefix => '%s', create_file_list => false)",
+ catalogName, tableIdent, table.location(), location);
+ assertThat(result).hasSize(1);
+ assertThat(result.get(0)[0])
+ .as("Should return correct latest version")
+ .isEqualTo(RewriteTablePathUtil.fileName(metadataJson));
+ assertThat(result.get(0)[1])
+ .as("Check if file list location is correctly marked as N/A when not generated")
+ .asString()
+ .isEqualTo("N/A");
+ }
+
private void checkFileListLocationCount(String fileListLocation, long expectedFileCount) {
long fileCount = spark.read().format("text").load(fileListLocation).count();
assertThat(fileCount).isEqualTo(expectedFileCount);
}
+
+ @TestTemplate
+ public void testRewriteTablePathWithManifestAndDeleteCounts() throws IOException {
+ sql("INSERT INTO %s VALUES (1, 'a')", tableName);
+ sql("INSERT INTO %s VALUES (2, 'b')", tableName);
+ sql("INSERT INTO %s VALUES (3, 'c')", tableName);
+
+ Table table = validationCatalog.loadTable(tableIdent);
+ List<Pair<CharSequence, Long>> rowsToDelete =
+ Lists.newArrayList(
+ Pair.of(
+ table.currentSnapshot().addedDataFiles(table.io()).iterator().next().location(),
+ 0L));
+
+ File file = new File(removePrefix(table.location()) + "/data/deletes.parquet");
+ DeleteFile positionDeletes =
+ FileHelpers.writeDeleteFile(
+ table, table.io().newOutputFile(file.toURI().toString()), rowsToDelete)
+ .first();
+
+ table.newRowDelta().addDeletes(positionDeletes).commit();
+
+ sql("INSERT INTO %s VALUES (4, 'd')", tableName);
+
+ String targetLocation = targetTableDir.toFile().toURI().toString();
+ String stagingLocation = staging.toFile().toURI().toString();
+
+ List<Object[]> result =
+ sql(
+ "CALL %s.system.rewrite_table_path("
+ + "table => '%s', "
+ + "source_prefix => '%s', "
+ + "target_prefix => '%s', "
+ + "staging_location => '%s', create_file_list => false)",
+ catalogName, tableIdent, table.location(), targetLocation, stagingLocation);
+
+ assertThat(result).hasSize(1);
+ Object[] row = result.get(0);
+
+ int rewrittenManifestFilesCount = ((Number) row[2]).intValue();
+ int rewrittenDeleteFilesCount = ((Number) row[3]).intValue();
+
+ assertThat(rewrittenDeleteFilesCount)
+ .as(
+ "Expected exactly 1 delete file to be rewritten, but found "
+ + rewrittenDeleteFilesCount)
+ .isEqualTo(1);
+
+ assertThat(rewrittenManifestFilesCount)
+ .as(
+ "Expected exactly 5 manifest files to be rewritten, but found "
+ + rewrittenManifestFilesCount)
+ .isEqualTo(5);
+ }
+
+ private String removePrefix(String path) {
+ return path.substring(path.lastIndexOf(":") + 1);
+ }
Review Comment:
```suggestion
@TestTemplate
public void testRewriteTablePathWithManifestAndDeleteCounts() throws IOException {
@TestTemplate
public void testRewriteTablePathWithManifestAndDeleteCounts() throws NoSuchTableException {
sql(
"CREATE TABLE %s (id bigint, data string) USING iceberg TBLPROPERTIES"
+ "('format-version'='2', 'write.delete.mode'='merge-on-read')",
tableName);
List<SimpleRecord> records =
Lists.newArrayList(
new SimpleRecord(1, "a"), new SimpleRecord(2, "b"), new SimpleRecord(3, "c"));
spark
.createDataset(records, Encoders.bean(SimpleRecord.class))
.coalesce(1)
.writeTo(tableName)
.append();
sql("DELETE FROM %s WHERE id=1", tableName);
sql("INSERT INTO %s VALUES (4, 'd')", tableName);
Table table = validationCatalog.loadTable(tableIdent);
String targetLocation = targetTableDir.toFile().toURI().toString();
String stagingLocation = staging.toFile().toURI().toString();
List<Object[]> result =
sql(
"CALL %s.system.rewrite_table_path("
+ "table => '%s', "
+ "source_prefix => '%s', "
+ "target_prefix => '%s', "
+ "staging_location => '%s', create_file_list => false)",
catalogName, tableIdent, table.location(), targetLocation, stagingLocation);
assertThat(result).hasSize(1);
Object[] row = result.get(0);
int rewrittenManifestFilesCount = ((Number) row[2]).intValue();
int rewrittenDeleteFilesCount = ((Number) row[3]).intValue();
assertThat(rewrittenDeleteFilesCount)
.as(
"Expected exactly 1 delete file to be rewritten, but found "
+ rewrittenDeleteFilesCount)
.isEqualTo(1);
assertThat(rewrittenManifestFilesCount)
.as(
"Expected exactly 3 manifest files to be rewritten, but found "
+ rewrittenManifestFilesCount)
.isEqualTo(table.currentSnapshot().allManifests(table.io()).size());
}
```
##########
open-api/src/testFixtures/java/org/apache/iceberg/rest/RESTCatalogServer.java:
##########
@@ -86,7 +86,7 @@ private CatalogContext initializeBackendCatalog() throws IOException {
if (warehouseLocation == null) {
File tmp = java.nio.file.Files.createTempDirectory("iceberg_warehouse").toFile();
tmp.deleteOnExit();
- warehouseLocation = new File(tmp, "iceberg_data").getAbsolutePath();
+ warehouseLocation = new File(tmp, "iceberg_data").toURI().toString();
Review Comment:
@slfan1989 I think this is actually because how position delete was written in unit test with ` File file = new File(removePrefix(table.location()) + "/data/deletes.parquet");`. I suggested alternative way so that we can still get what we want without need to change this (at least for this PR).
If we want to update the behavior in REST test fixture, let's use #13882 instead with proper unit tests
--
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] API, Spark 4.0: Add create_file_list option to RewriteTablePathProcedure. [iceberg]
Posted by "szehon-ho (via GitHub)" <gi...@apache.org>.
szehon-ho commented on PR #13837:
URL: https://github.com/apache/iceberg/pull/13837#issuecomment-3332501434
Sorry for delay in reply, late looks good to me, thanks @slfan1989
--
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] API, Spark 4.0: Add 'skip_file_list' option to RewriteTablePathProcedure for optional file-list generation. [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #13837:
URL: https://github.com/apache/iceberg/pull/13837#discussion_r2290270365
##########
api/src/main/java/org/apache/iceberg/actions/RewriteTablePath.java:
##########
@@ -86,6 +86,16 @@ public interface RewriteTablePath extends Action<RewriteTablePath, RewriteTableP
*/
RewriteTablePath stagingLocation(String stagingLocation);
+ /**
+ * Whether to skip saving the file list location.
+ *
+ * @param skipFileList true to skip saving the file list, false to include it
+ * @return this instance for method chaining
+ */
+ default RewriteTablePath skipFileList(boolean skipFileList) {
Review Comment:
why not call this `createFileList` that always defaults to true. Also without reading the javadoc it's not clear what `skip` actually refers to. Does it skip scanning that file or creating/deleting/x?
##########
api/src/main/java/org/apache/iceberg/actions/RewriteTablePath.java:
##########
@@ -112,5 +122,15 @@ interface Result {
/** Name of latest metadata file version */
String latestVersion();
+
+ /** count of rewrite delete files, default value is 0 */
Review Comment:
```suggestion
/** count of rewritten delete files, default value is 0 */
```
--
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] API, Spark 4.0: Add 'skip_file_list' option to RewriteTablePathProcedure for optional file-list generation. [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #13837:
URL: https://github.com/apache/iceberg/pull/13837#discussion_r2290315628
##########
api/src/main/java/org/apache/iceberg/actions/RewriteTablePath.java:
##########
@@ -112,5 +122,15 @@ interface Result {
/** Name of latest metadata file version */
String latestVersion();
+
+ /** count of rewrite delete files, default value is 0 */
+ default int rewrittenDeleteFilesCount() {
Review Comment:
I think rewrittenDeleteFilesCount might be a bit misleading? Aren't we just rewriting table paths and not actually compacting files together? Other Actions/Procedures use a similar naming but there it actually means that files have been compacted together
--
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] API, Spark 4.0: Add create_file_list option to RewriteTablePathProcedure. [iceberg]
Posted by "slfan1989 (via GitHub)" <gi...@apache.org>.
slfan1989 commented on PR #13837:
URL: https://github.com/apache/iceberg/pull/13837#issuecomment-3332625269
> Sorry for delay in reply, late looks good to me, thanks @slfan1989
@szehon-ho Thank you for helping with the review!
--
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] API, Spark 4.0: Add 'skip_file_list' option to RewriteTablePathProcedure for optional file-list generation. [iceberg]
Posted by "slfan1989 (via GitHub)" <gi...@apache.org>.
slfan1989 commented on code in PR #13837:
URL: https://github.com/apache/iceberg/pull/13837#discussion_r2308866081
##########
open-api/src/testFixtures/java/org/apache/iceberg/rest/RESTCatalogServer.java:
##########
@@ -86,7 +86,7 @@ private CatalogContext initializeBackendCatalog() throws IOException {
if (warehouseLocation == null) {
File tmp = java.nio.file.Files.createTempDirectory("iceberg_warehouse").toFile();
tmp.deleteOnExit();
- warehouseLocation = new File(tmp, "iceberg_data").getAbsolutePath();
+ warehouseLocation = new File(tmp, "iceberg_data").toURI().toString();
Review Comment:
@dramaticlly Thank you for your message! I have carefully readed the relevant code and confirmed that this change is necessary. The reason is that the base path for the Hive library in RESTCatalog is inconsistent with other types of Catalogs. Currently, HIVE, HADOOP, and SPARK_SESSION all use URI format, so RESTCatalog needs to be adjusted to the same format. Additionally, since the TestRewriteTablePathProcedure test involves parameter type testing and simultaneously tests four different Catalogs, we must standardize the base path of the Hive library across all four Catalogs to URI format in order to conduct the test successfully. Otherwise, there could be issues with prefix validation failures.
```
Path file:/var/folders/2k/21gv5vmx6z7dlr1g0_jtm8r80000ks/T/iceberg_warehouse10689209309257803697/iceberg_data/default/table/data/deletes.parquet does not start with /var/folders/2k/21gv5vmx6z7dlr1g0_jtm8r80000ks/T/iceberg_warehouse10689209309257803697/iceberg_data/default/table/
java.lang.IllegalArgumentException: Path file:/var/folders/2k/21gv5vmx6z7dlr1g0_jtm8r80000ks/T/iceberg_warehouse10689209309257803697/iceberg_data/default/table/data/deletes.parquet does not start with /var/folders/2k/21gv5vmx6z7dlr1g0_jtm8r80000ks/T/iceberg_warehouse10689209309257803697/iceberg_data/default/table/
at org.apache.iceberg.RewriteTablePathUtil.relativize(RewriteTablePathUtil.java:686)
.....
```
--
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] API, Spark 4.0: Add 'skip_file_list' option to RewriteTablePathProcedure for optional file-list generation. [iceberg]
Posted by "dramaticlly (via GitHub)" <gi...@apache.org>.
dramaticlly commented on code in PR #13837:
URL: https://github.com/apache/iceberg/pull/13837#discussion_r2316656627
##########
open-api/src/testFixtures/java/org/apache/iceberg/rest/RESTCatalogServer.java:
##########
@@ -86,7 +86,7 @@ private CatalogContext initializeBackendCatalog() throws IOException {
if (warehouseLocation == null) {
File tmp = java.nio.file.Files.createTempDirectory("iceberg_warehouse").toFile();
tmp.deleteOnExit();
- warehouseLocation = new File(tmp, "iceberg_data").getAbsolutePath();
+ warehouseLocation = new File(tmp, "iceberg_data").toURI().toString();
Review Comment:
Given this is just for unit tests, apply special casing on REST catalog is fine to collect path is fine. We can later focus on #13882 to see if we need to resolve the differences between catalogs.
--
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] API, Spark 4.0: Add 'skip_file_list' option to RewriteTablePathProcedure for optional file-list generation. [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on PR #13837:
URL: https://github.com/apache/iceberg/pull/13837#issuecomment-3301486667
@slfan1989 can you please update the PR title to reflect the latest 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] API, Spark 4.0: Add 'skip_file_list' option to RewriteTablePathProcedure for optional file-list generation. [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #13837:
URL: https://github.com/apache/iceberg/pull/13837#discussion_r2290271227
##########
api/src/main/java/org/apache/iceberg/actions/RewriteTablePath.java:
##########
@@ -112,5 +122,15 @@ interface Result {
/** Name of latest metadata file version */
String latestVersion();
+
+ /** count of rewrite delete files, default value is 0 */
+ default int rewrittenDeleteFilesCount() {
+ return 0;
+ }
+
+ /** count of rewrite metadata files involved, default value is 0 */
Review Comment:
```suggestion
/** count of rewritten manifests involved, default value is 0 */
```
--
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] API, Spark 4.0: Add create_file_list option to RewriteTablePathProcedure. [iceberg]
Posted by "szehon-ho (via GitHub)" <gi...@apache.org>.
szehon-ho commented on code in PR #13837:
URL: https://github.com/apache/iceberg/pull/13837#discussion_r2378096699
##########
spark/v4.0/spark/src/main/java/org/apache/iceberg/spark/actions/RewriteTablePathSparkAction.java:
##########
@@ -91,12 +91,14 @@ public class RewriteTablePathSparkAction extends BaseSparkAction<RewriteTablePat
private static final Logger LOG = LoggerFactory.getLogger(RewriteTablePathSparkAction.class);
private static final String RESULT_LOCATION = "file-list";
+ static final String NOT_APPLICABLE = "N/A";
Review Comment:
sorry just noticed one thing, this seems a bit awkward. Was there some discussion on just having an empty string, or does the result object not allow it? In any case, I feel it can be private here.
--
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] API, Spark 4.0: Add create_file_list option to RewriteTablePathProcedure. [iceberg]
Posted by "slfan1989 (via GitHub)" <gi...@apache.org>.
slfan1989 commented on code in PR #13837:
URL: https://github.com/apache/iceberg/pull/13837#discussion_r2378144784
##########
spark/v4.0/spark/src/main/java/org/apache/iceberg/spark/actions/RewriteTablePathSparkAction.java:
##########
@@ -91,12 +91,14 @@ public class RewriteTablePathSparkAction extends BaseSparkAction<RewriteTablePat
private static final Logger LOG = LoggerFactory.getLogger(RewriteTablePathSparkAction.class);
private static final String RESULT_LOCATION = "file-list";
+ static final String NOT_APPLICABLE = "N/A";
Review Comment:
The use of `N/A` has also been discussed. From a personal perspective, I believe it should be acceptable. The specific message link is as follows:
https://github.com/apache/iceberg/pull/13837#discussion_r2292598195
https://github.com/apache/iceberg/pull/13837#discussion_r2292639708
--
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] API, Spark 4.0: Add 'skip_file_list' option to RewriteTablePathProcedure for optional file-list generation. [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #13837:
URL: https://github.com/apache/iceberg/pull/13837#discussion_r2352855241
##########
spark/v4.0/spark/src/test/java/org/apache/iceberg/spark/actions/TestRewriteTablePathsAction.java:
##########
@@ -1234,6 +1235,28 @@ public void testNestedDirectoryStructurePreservation() throws Exception {
assertThat(targetPath2).startsWith(targetTableLocation());
}
+ @Test
+ public void testRewritePathWithoutCreateFileList() throws Exception {
+ // Target table location
+ String targetTableLocation = targetTableLocation();
+
+ // Execute path rewrite operation with file list creation disabled
+ RewriteTablePath.Result result =
+ actions()
+ .rewriteTablePath(table)
+ .rewriteLocationPrefix(tableLocation, targetTableLocation)
+ .createFileList(false) // Disable file list creation
+ .execute();
+
+ // Verify the latest version is correct
+ assertThat(result.latestVersion()).isEqualTo("v3.metadata.json");
+
+ // Check that the file list location is empty since the file list was not created
Review Comment:
can be removed
--
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] API, Spark 4.0: Add 'skip_file_list' option to RewriteTablePathProcedure for optional file-list generation. [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #13837:
URL: https://github.com/apache/iceberg/pull/13837#discussion_r2354431749
##########
spark/v4.0/spark/src/main/java/org/apache/iceberg/spark/actions/RewriteTablePathSparkAction.java:
##########
@@ -300,12 +304,28 @@ private String rebuildMetadata() {
.collect(Collectors.toSet());
rewritePositionDeletes(deleteFiles);
+ ImmutableRewriteTablePath.Result.Builder builder =
+ ImmutableRewriteTablePath.Result.builder()
+ .stagingLocation(stagingDir)
+ .rewrittenDeleteFilePathsCount(deleteFiles.size())
+ .rewrittenManifestFilePathsCount(metaFiles.size())
+ .latestVersion(RewriteTablePathUtil.fileName(endVersionName));
+
+ if (!createFileList) {
+ return builder.fileListLocation(NOT_APPLICABLE).build();
+ }
+
Set<Pair<String, String>> copyPlan = Sets.newHashSet();
copyPlan.addAll(rewriteVersionResult.copyPlan());
copyPlan.addAll(rewriteManifestListResult.copyPlan());
copyPlan.addAll(rewriteManifestResult.copyPlan());
+ String fileListLocation = saveFileList(copyPlan);
- return saveFileList(copyPlan);
+ return builder
+ .stagingLocation(stagingDir)
+ .fileListLocation(fileListLocation)
+ .latestVersion(RewriteTablePathUtil.fileName(endVersionName))
Review Comment:
why are we setting latestVersion and stagingLocation again here?
--
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] API, Spark 4.0: Add create_file_list option to RewriteTablePathProcedure. [iceberg]
Posted by "slfan1989 (via GitHub)" <gi...@apache.org>.
slfan1989 commented on PR #13837:
URL: https://github.com/apache/iceberg/pull/13837#issuecomment-3322329096
> LGTM once the remaining comment has been applied. @szehon-ho can you also take a look please?
@nastra Could we consider moving this PR forward? Many thanks! I’d also like to contribute a new feature on top of it — in-place upgrades from Hive tables to Iceberg tables. Over the past four months, we’ve used this internally to upgrade more than 2,800 tables, delivering significant performance and cost benefits. I hope this feature can help more users perform in-place upgrades from Hive to Iceberg.
cc: @dramaticlly @szehon-ho
--
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] API, Spark 4.0: Add create_file_list option to RewriteTablePathProcedure. [iceberg]
Posted by "slfan1989 (via GitHub)" <gi...@apache.org>.
slfan1989 commented on code in PR #13837:
URL: https://github.com/apache/iceberg/pull/13837#discussion_r2378111676
##########
spark/v4.0/spark/src/main/java/org/apache/iceberg/spark/actions/RewriteTablePathSparkAction.java:
##########
@@ -91,12 +91,14 @@ public class RewriteTablePathSparkAction extends BaseSparkAction<RewriteTablePat
private static final Logger LOG = LoggerFactory.getLogger(RewriteTablePathSparkAction.class);
private static final String RESULT_LOCATION = "file-list";
+ static final String NOT_APPLICABLE = "N/A";
Review Comment:
Thank you for your message. After discussion, it was ultimately decided to be a package-level private member. We also used this variable in the unit tests.
https://github.com/apache/iceberg/pull/13837#discussion_r2316661941
--
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] API, Spark 4.0: Add 'skip_file_list' option to RewriteTablePathProcedure for optional file-list generation. [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #13837:
URL: https://github.com/apache/iceberg/pull/13837#discussion_r2352704615
##########
spark/v4.0/spark-extensions/src/test/java/org/apache/iceberg/spark/extensions/TestRewriteTablePathProcedure.java:
##########
@@ -171,8 +178,92 @@ public void testProcedureWithInvalidInput() {
"Cannot find provided version file %s in metadata log.", "v11.metadata.json");
}
+ @TestTemplate
+ public void testRewriteTablePathWithoutFileList() {
+ String location = targetTableDir.toFile().toURI().toString();
+ Table table = validationCatalog.loadTable(tableIdent);
+ String metadataJson = TableUtil.metadataFileLocation(table);
+
+ List<Object[]> result =
+ sql(
+ "CALL %s.system.rewrite_table_path(table => '%s', source_prefix => '%s', target_prefix => '%s', create_file_list => false)",
+ catalogName, tableIdent, table.location(), location);
+ assertThat(result).hasSize(1);
+ assertThat(result.get(0)[0])
+ .as("Should return correct latest version")
+ .isEqualTo(RewriteTablePathUtil.fileName(metadataJson));
+ assertThat(result.get(0)[1])
+ .as("Check if file list location is correctly marked as N/A when not generated")
+ .asString()
+ .isEqualTo("N/A");
+ }
+
private void checkFileListLocationCount(String fileListLocation, long expectedFileCount) {
long fileCount = spark.read().format("text").load(fileListLocation).count();
assertThat(fileCount).isEqualTo(expectedFileCount);
}
+
+ @TestTemplate
+ public void testRewriteTablePathWithManifestAndDeleteCounts() throws IOException {
+ sql("INSERT INTO %s VALUES (1, 'a')", tableName);
+ sql("INSERT INTO %s VALUES (2, 'b')", tableName);
+ sql("INSERT INTO %s VALUES (3, 'c')", tableName);
+
+ Table table = validationCatalog.loadTable(tableIdent);
+ List<Pair<CharSequence, Long>> rowsToDelete =
+ Lists.newArrayList(
+ Pair.of(
+ table.currentSnapshot().addedDataFiles(table.io()).iterator().next().location(),
+ 0L));
+
+ File file = new File(removePrefix(table.location()) + "/data/deletes.parquet");
+ String filePath = file.toURI().toString();
+ if (SparkCatalogConfig.REST.catalogName().equals(catalogName)) {
+ // We applied this special handling because the base path for
+ // matching the RESTCATALOG's Hive BaseLocation is represented
+ // in the form of an AbsolutePath.
+ filePath = file.getAbsolutePath().toString();
+ }
+ DeleteFile positionDeletes =
Review Comment:
nit: please add a newline after the }
--
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] API, Spark 4.0: Add 'skip_file_list' option to RewriteTablePathProcedure for optional file-list generation. [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #13837:
URL: https://github.com/apache/iceberg/pull/13837#discussion_r2352856378
##########
spark/v4.0/spark/src/main/java/org/apache/iceberg/spark/procedures/RewriteTablePathProcedure.java:
##########
@@ -115,6 +123,7 @@ public Iterator<Scan> call(InternalRow args) {
if (stagingLocation != null) {
action.stagingLocation(stagingLocation);
}
+ action.createFileList(createFileList);
Review Comment:
nit: newline after }
--
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] API, Spark 4.0: Add 'skip_file_list' option to RewriteTablePathProcedure for optional file-list generation. [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #13837:
URL: https://github.com/apache/iceberg/pull/13837#discussion_r2352858504
##########
spark/v4.0/spark/src/main/java/org/apache/iceberg/spark/procedures/RewriteTablePathProcedure.java:
##########
@@ -58,14 +60,19 @@ public class RewriteTablePathProcedure extends BaseProcedure {
TARGET_PREFIX_PARAM,
START_VERSION_PARAM,
END_VERSION_PARM,
- STAGING_LOCATION_PARAM
+ STAGING_LOCATION_PARAM,
+ CREATE_FILE_LIST_PARAM
};
private static final StructType OUTPUT_TYPE =
new StructType(
new StructField[] {
new StructField("latest_version", DataTypes.StringType, true, Metadata.empty()),
- new StructField("file_list_location", DataTypes.StringType, true, Metadata.empty())
+ new StructField("file_list_location", DataTypes.StringType, true, Metadata.empty()),
+ new StructField(
+ "rewritten_metadata_files_count", DataTypes.IntegerType, true, Metadata.empty()),
Review Comment:
we would need to update the naming here as well to indicate that these are referring to paths
--
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] API, Spark 4.0: Add 'skip_file_list' option to RewriteTablePathProcedure for optional file-list generation. [iceberg]
Posted by "manuzhang (via GitHub)" <gi...@apache.org>.
manuzhang commented on code in PR #13837:
URL: https://github.com/apache/iceberg/pull/13837#discussion_r2281194879
##########
api/src/main/java/org/apache/iceberg/actions/RewriteTablePath.java:
##########
@@ -86,6 +86,16 @@ public interface RewriteTablePath extends Action<RewriteTablePath, RewriteTableP
*/
RewriteTablePath stagingLocation(String stagingLocation);
+ /**
+ * Whether to skip saving the file list location.
+ *
+ * @param skipFileList true to skip saving the file list, false to include it
+ * @return this instance for method chaining
+ */
+ default RewriteTablePath saveFileList(boolean skipFileList) {
Review Comment:
The method name and the parameter name have opposite meaning.
`saveFileList(true)` is actually not to save file list.
--
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] API, Spark 4.0: Add 'skip_file_list' option to RewriteTablePathProcedure for optional file-list generation. [iceberg]
Posted by "slfan1989 (via GitHub)" <gi...@apache.org>.
slfan1989 commented on code in PR #13837:
URL: https://github.com/apache/iceberg/pull/13837#discussion_r2290634804
##########
open-api/src/testFixtures/java/org/apache/iceberg/rest/RESTCatalogServer.java:
##########
@@ -86,7 +86,7 @@ private CatalogContext initializeBackendCatalog() throws IOException {
if (warehouseLocation == null) {
File tmp = java.nio.file.Files.createTempDirectory("iceberg_warehouse").toFile();
tmp.deleteOnExit();
- warehouseLocation = new File(tmp, "iceberg_data").getAbsolutePath();
+ warehouseLocation = new File(tmp, "iceberg_data").toURI().toString();
Review Comment:
I provided a more detailed explanation in #13882. The issue arises because the path generated by REST uses getAbsolutePath(), while the current unit tests use targetTableDir.toFile().toURI().toString(). Since the two paths do not match, the unit tests fail.
```
Path file:/var/folders/2k/21gv5vmx6z7dlr1g0_jtm8r80000ks/T/iceberg_warehouse10689209309257803697/iceberg_data/default/table/data/deletes.parquet does not start with /var/folders/2k/21gv5vmx6z7dlr1g0_jtm8r80000ks/T/iceberg_warehouse10689209309257803697/iceberg_data/default/table/
java.lang.IllegalArgumentException: Path file:/var/folders/2k/21gv5vmx6z7dlr1g0_jtm8r80000ks/T/iceberg_warehouse10689209309257803697/iceberg_data/default/table/data/deletes.parquet does not start with /var/folders/2k/21gv5vmx6z7dlr1g0_jtm8r80000ks/T/iceberg_warehouse10689209309257803697/iceberg_data/default/table/
at org.apache.iceberg.RewriteTablePathUtil.relativize(RewriteTablePathUtil.java:686)
at org.apache.iceberg.RewriteTablePathUtil.newPath(RewriteTablePathUtil.java:663)
at org.apache.iceberg.RewriteTablePathUtil.writeDeleteFileEntry(RewriteTablePathUtil.java:492)
at org.apache.iceberg.RewriteTablePathUtil.lambda$rewriteDeleteManifest$5(RewriteTablePathUtil.java:438)
at java.base/java.util.stream.ReferencePipeline$3$1.accept(ReferencePipeline.java:197)
at java.base/java.util.Iterator.forEachRemaining(Iterator.java:133)
at java.base/java.util.Spliterators$IteratorSpliterator.forEachRemaining(Spliterators.java:1845)
at java.base/java.util.stream.AbstractPipeline.copyInto(AbstractPipeline.java:509)
```
--
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] API, Spark 4.0: Add 'skip_file_list' option to RewriteTablePathProcedure for optional file-list generation. [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #13837:
URL: https://github.com/apache/iceberg/pull/13837#discussion_r2292639708
##########
spark/v4.0/spark/src/main/java/org/apache/iceberg/spark/actions/RewriteTablePathSparkAction.java:
##########
@@ -304,12 +307,29 @@ private String rebuildMetadata() {
.collect(Collectors.toSet());
rewritePositionDeletes(deleteFiles);
+ ImmutableRewriteTablePath.Result.Builder builder =
+ ImmutableRewriteTablePath.Result.builder()
+ .stagingLocation(stagingDir)
+ .rewrittenDeleteFilesCount(deleteFiles.size())
+ .rewrittenManifestFilesCount(metaFiles.size())
+ .latestVersion(RewriteTablePathUtil.fileName(endVersionName));
+
+ // skip file list
+ if (skipFileList) {
+ return builder.build();
+ }
+
Set<Pair<String, String>> copyPlan = Sets.newHashSet();
copyPlan.addAll(rewriteVersionResult.copyPlan());
copyPlan.addAll(rewriteManifestListResult.copyPlan());
copyPlan.addAll(rewriteManifestResult.copyPlan());
+ String fileListLocation = saveFileList(copyPlan);
- return saveFileList(copyPlan);
+ return builder
+ .stagingLocation(stagingDir)
+ .fileListLocation(fileListLocation)
Review Comment:
yep I think assigning a `n/a` string when building the result object makes sense to me, cc @szehon-ho
--
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: Add 'skip_file_list' option to RewriteTablePathProcedure for optional file-list generation. [iceberg]
Posted by "slfan1989 (via GitHub)" <gi...@apache.org>.
slfan1989 commented on PR #13837:
URL: https://github.com/apache/iceberg/pull/13837#issuecomment-3194895452
@szehon-ho @dramaticlly @manuzhang @huaxingao @nastra Could you help review this pr? This is a minor improvement. Currently, when using the RewriteTablePath stored procedure, a file named file-list is generated in the target path, even though it is unrelated to the actual data. We would like to have the option to skip creating this file. This PR introduces a new option that allows skipping the generation of the file-list file.
We discussed some aspects of this PR in #12844. Can we continue moving this PR forward?
--
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] API, Spark 4.0: Add 'skip_file_list' option to RewriteTablePathProcedure for optional file-list generation. [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #13837:
URL: https://github.com/apache/iceberg/pull/13837#discussion_r2290281341
##########
spark/v4.0/spark/src/main/java/org/apache/iceberg/spark/procedures/RewriteTablePathProcedure.java:
##########
@@ -58,14 +60,19 @@ public class RewriteTablePathProcedure extends BaseProcedure {
TARGET_PREFIX_PARAM,
START_VERSION_PARAM,
END_VERSION_PARM,
- STAGING_LOCATION_PARAM
+ STAGING_LOCATION_PARAM,
+ SKIP_FILE_LIST_PARAM
};
private static final StructType OUTPUT_TYPE =
new StructType(
new StructField[] {
new StructField("latest_version", DataTypes.StringType, true, Metadata.empty()),
- new StructField("file_list_location", DataTypes.StringType, true, Metadata.empty())
+ new StructField("file_list_location", DataTypes.StringType, true, Metadata.empty()),
+ new StructField(
+ "rewrite_metadata_files_count", DataTypes.IntegerType, true, Metadata.empty()),
Review Comment:
```suggestion
"rewritten_manifests_count", DataTypes.IntegerType, true, Metadata.empty()),
```
--
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] API, Spark 4.0: Add 'skip_file_list' option to RewriteTablePathProcedure for optional file-list generation. [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #13837:
URL: https://github.com/apache/iceberg/pull/13837#discussion_r2290277164
##########
spark/v4.0/spark-extensions/src/test/java/org/apache/iceberg/spark/extensions/TestRewriteTablePathProcedure.java:
##########
@@ -171,8 +177,79 @@ public void testProcedureWithInvalidInput() {
"Cannot find provided version file %s in metadata log.", "v11.metadata.json");
}
+ @TestTemplate
+ public void testRewriteTablePathWithSkipFileList() {
+ String location = targetTableDir.toFile().toURI().toString();
+ Table table = validationCatalog.loadTable(tableIdent);
+ String metadataJson = TableUtil.metadataFileLocation(table);
+
+ List<Object[]> result =
+ sql(
+ "CALL %s.system.rewrite_table_path(table => '%s', source_prefix => '%s', target_prefix => '%s', skip_file_list => true)",
+ catalogName, tableIdent, table.location(), location);
+ assertThat(result).hasSize(1);
+ assertThat(result.get(0)[0])
+ .as("Should return correct latest version")
+ .isEqualTo(RewriteTablePathUtil.fileName(metadataJson));
+ assertThat(result.get(0)[1]).as("Should return empty").asString().isEqualTo("");
+ }
+
private void checkFileListLocationCount(String fileListLocation, long expectedFileCount) {
long fileCount = spark.read().format("text").load(fileListLocation).count();
assertThat(fileCount).isEqualTo(expectedFileCount);
}
+
+ @TestTemplate
+ public void testRewriteTablePathWithManifestAndDeleteCounts() throws IOException {
+
+ sql("INSERT INTO %s VALUES (1, 'a')", tableName);
+ sql("INSERT INTO %s VALUES (2, 'b')", tableName);
+ sql("INSERT INTO %s VALUES (3, 'c')", tableName);
+
+ Table table = validationCatalog.loadTable(tableIdent);
+ List<Pair<CharSequence, Long>> deletes =
Review Comment:
this says `deletes` but it actually refers to data files
--
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] API, Spark 4.0: Add 'skip_file_list' option to RewriteTablePathProcedure for optional file-list generation. [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #13837:
URL: https://github.com/apache/iceberg/pull/13837#discussion_r2290271227
##########
api/src/main/java/org/apache/iceberg/actions/RewriteTablePath.java:
##########
@@ -112,5 +122,15 @@ interface Result {
/** Name of latest metadata file version */
String latestVersion();
+
+ /** count of rewrite delete files, default value is 0 */
+ default int rewrittenDeleteFilesCount() {
+ return 0;
+ }
+
+ /** count of rewrite metadata files involved, default value is 0 */
Review Comment:
```suggestion
/** count of rewritten metadata files involved, default value is 0 */
```
--
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] API, Spark 4.0: Add 'skip_file_list' option to RewriteTablePathProcedure for optional file-list generation. [iceberg]
Posted by "dramaticlly (via GitHub)" <gi...@apache.org>.
dramaticlly commented on code in PR #13837:
URL: https://github.com/apache/iceberg/pull/13837#discussion_r2294839225
##########
spark/v4.0/spark/src/test/java/org/apache/iceberg/spark/actions/TestRewriteTablePathsAction.java:
##########
@@ -1384,4 +1384,26 @@ private void removeBroadcastValuesFromLocalBlockManager(long id) {
blockInfoManager.removeBlock(blockId);
blockManager.memoryStore().remove(blockId);
}
+
+ @Test
+ public void testRewritePathWithCreateFileListFalse() throws Exception {
Review Comment:
nit: how about `testRewritePathWithoutCreateFileList`
##########
api/src/main/java/org/apache/iceberg/actions/RewriteTablePath.java:
##########
@@ -112,5 +125,15 @@ interface Result {
/** Name of latest metadata file version */
String latestVersion();
+
+ /** Number of delete files whose paths were updated. Default value is 0. */
+ default int deleteFilesPathRewrittenCount() {
+ return 0;
+ }
+
+ /** Number of manifest files whose paths were updated. Default value is 0. */
+ default int manifestFilesPathRewrittenCount() {
+ return 0;
+ }
Review Comment:
```suggestion
/** Number of delete files with rewritten paths. */
default int deleteFilesPathRewrittenCount() {
return 0;
}
/** Number of manifest files with rewritten paths. */
default int manifestFilesPathRewrittenCount() {
return 0;
}
```
##########
spark/v4.0/spark/src/main/java/org/apache/iceberg/spark/actions/RewriteTablePathSparkAction.java:
##########
@@ -304,12 +307,29 @@ private String rebuildMetadata() {
.collect(Collectors.toSet());
rewritePositionDeletes(deleteFiles);
+ ImmutableRewriteTablePath.Result.Builder builder =
+ ImmutableRewriteTablePath.Result.builder()
+ .stagingLocation(stagingDir)
+ .deleteFilesPathRewrittenCount(deleteFiles.size())
+ .manifestFilesPathRewrittenCount(metaFiles.size())
+ .latestVersion(RewriteTablePathUtil.fileName(endVersionName));
+
+ // file list generation disabled
+ if (!fileListEnabled) {
+ return builder.fileListLocation("N/A").build();
Review Comment:
nit: can we pull this up as a package local variable to represent not applicable and use it to assert in `spark/v4.0/spark/src/test/java/org/apache/iceberg/spark/actions/TestRewriteTablePathsAction.java`
##########
spark/v4.0/spark/src/main/java/org/apache/iceberg/spark/actions/RewriteTablePathSparkAction.java:
##########
@@ -96,7 +96,7 @@ public class RewriteTablePathSparkAction extends BaseSparkAction<RewriteTablePat
private String startVersionName;
private String endVersionName;
private String stagingDir;
- private boolean skipFileList;
+ private boolean fileListEnabled = true;
Review Comment:
can we call it `createFileList` so we can have consist naming in interface, action and procedure?
--
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] API, Spark 4.0: Add 'skip_file_list' option to RewriteTablePathProcedure for optional file-list generation. [iceberg]
Posted by "slfan1989 (via GitHub)" <gi...@apache.org>.
slfan1989 commented on code in PR #13837:
URL: https://github.com/apache/iceberg/pull/13837#discussion_r2290607582
##########
api/src/main/java/org/apache/iceberg/actions/RewriteTablePath.java:
##########
@@ -86,6 +86,16 @@ public interface RewriteTablePath extends Action<RewriteTablePath, RewriteTableP
*/
RewriteTablePath stagingLocation(String stagingLocation);
+ /**
+ * Whether to skip saving the file list location.
+ *
+ * @param skipFileList true to skip saving the file list, false to include it
+ * @return this instance for method chaining
+ */
+ default RewriteTablePath skipFileList(boolean skipFileList) {
Review Comment:
Thank you for your question! `RewriteTablePathProcedure` generates a CSV file, which is saved by default in the target directory. In #12762, I proposed a new migration approach that allows upgrading Hive tables to Iceberg tables in place. Using this method, we have already upgraded more than 2,000 tables.
Currently, after each migration, a `file-list.csv` file is created in the table’s `metadata` directory. Since this file is not actually part of the metadata, we need to clean it up manually. I suggest adding a parameter to the procedure so that when it is invoked, we can choose not to generate this file, thereby simplifying the migration process.
I think calling it createFileList is reasonable, and I will optimize this part accordingly.
--
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] API, Spark 4.0: Add create_file_list option to RewriteTablePathProcedure. [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra merged PR #13837:
URL: https://github.com/apache/iceberg/pull/13837
--
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] API, Spark 4.0: Add 'skip_file_list' option to RewriteTablePathProcedure for optional file-list generation. [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #13837:
URL: https://github.com/apache/iceberg/pull/13837#discussion_r2352860106
##########
spark/v4.0/spark/src/main/java/org/apache/iceberg/spark/actions/RewriteTablePathSparkAction.java:
##########
@@ -300,12 +304,29 @@ private String rebuildMetadata() {
.collect(Collectors.toSet());
rewritePositionDeletes(deleteFiles);
+ ImmutableRewriteTablePath.Result.Builder builder =
+ ImmutableRewriteTablePath.Result.builder()
+ .stagingLocation(stagingDir)
+ .deleteFilesPathRewrittenCount(deleteFiles.size())
+ .manifestFilesPathRewrittenCount(metaFiles.size())
+ .latestVersion(RewriteTablePathUtil.fileName(endVersionName));
+
+ // file list generation disabled
Review Comment:
comment can be removed as it doesn't add much value imo
--
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] API, Spark 4.0: Add create_file_list option to RewriteTablePathProcedure. [iceberg]
Posted by "slfan1989 (via GitHub)" <gi...@apache.org>.
slfan1989 commented on PR #13837:
URL: https://github.com/apache/iceberg/pull/13837#issuecomment-3303789005
> @slfan1989 can you please update the PR title to reflect the latest changes?
@nastra Thanks a lot for the review! I’ve updated the title to `Add create_file_list option to RewriteTablePathProcedure` — Does this look okay to you?
--
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] API, Spark 4.0: Add 'skip_file_list' option to RewriteTablePathProcedure for optional file-list generation. [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #13837:
URL: https://github.com/apache/iceberg/pull/13837#discussion_r2352692917
##########
api/src/main/java/org/apache/iceberg/actions/RewriteTablePath.java:
##########
@@ -112,5 +125,15 @@ interface Result {
/** Name of latest metadata file version */
String latestVersion();
+
+ /** Number of delete files with rewritten paths. */
+ default int deleteFilesPathRewrittenCount() {
Review Comment:
can we rename these two methods to what I suggested earlier in https://github.com/apache/iceberg/pull/13837#discussion_r2292638286?
--
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] API, Spark 4.0: Add 'skip_file_list' option to RewriteTablePathProcedure for optional file-list generation. [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #13837:
URL: https://github.com/apache/iceberg/pull/13837#discussion_r2352854427
##########
spark/v4.0/spark/src/test/java/org/apache/iceberg/spark/actions/TestRewriteTablePathsAction.java:
##########
@@ -1234,6 +1235,28 @@ public void testNestedDirectoryStructurePreservation() throws Exception {
assertThat(targetPath2).startsWith(targetTableLocation());
}
+ @Test
+ public void testRewritePathWithoutCreateFileList() throws Exception {
+ // Target table location
Review Comment:
I don't think comments like these are helpful, so I would just remove them
--
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] API, Spark 4.0: Add create_file_list option to RewriteTablePathProcedure. [iceberg]
Posted by "slfan1989 (via GitHub)" <gi...@apache.org>.
slfan1989 commented on PR #13837:
URL: https://github.com/apache/iceberg/pull/13837#issuecomment-3330906107
@nastra @dramaticlly @manuzhang Thanks a lot for your help throughout the process of completing 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] API, Spark 4.0: Add create_file_list option to RewriteTablePathProcedure. [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #13837:
URL: https://github.com/apache/iceberg/pull/13837#discussion_r2361958286
##########
spark/v4.0/spark/src/main/java/org/apache/iceberg/spark/actions/RewriteTablePathSparkAction.java:
##########
@@ -300,12 +304,28 @@ private String rebuildMetadata() {
.collect(Collectors.toSet());
rewritePositionDeletes(deleteFiles);
+ ImmutableRewriteTablePath.Result.Builder builder =
+ ImmutableRewriteTablePath.Result.builder()
+ .stagingLocation(stagingDir)
+ .rewrittenDeleteFilePathsCount(deleteFiles.size())
+ .rewrittenManifestFilePathsCount(metaFiles.size())
+ .latestVersion(RewriteTablePathUtil.fileName(endVersionName));
+
+ if (!createFileList) {
+ return builder.fileListLocation(NOT_APPLICABLE).build();
+ }
+
Set<Pair<String, String>> copyPlan = Sets.newHashSet();
copyPlan.addAll(rewriteVersionResult.copyPlan());
copyPlan.addAll(rewriteManifestListResult.copyPlan());
copyPlan.addAll(rewriteManifestResult.copyPlan());
+ String fileListLocation = saveFileList(copyPlan);
- return saveFileList(copyPlan);
+ return builder
+ .stagingLocation(stagingDir)
+ .fileListLocation(fileListLocation)
+ .latestVersion(RewriteTablePathUtil.fileName(endVersionName))
Review Comment:
what I mean is: we are setting latestVersion and stagingLocation already in L309, so why do it here again?
--
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] API, Spark 4.0: Add 'skip_file_list' option to RewriteTablePathProcedure for optional file-list generation. [iceberg]
Posted by "dramaticlly (via GitHub)" <gi...@apache.org>.
dramaticlly commented on code in PR #13837:
URL: https://github.com/apache/iceberg/pull/13837#discussion_r2283518243
##########
spark/v4.0/spark/src/main/java/org/apache/iceberg/spark/actions/RewriteTablePathSparkAction.java:
##########
@@ -304,12 +307,29 @@ private String rebuildMetadata() {
.collect(Collectors.toSet());
rewritePositionDeletes(deleteFiles);
+ ImmutableRewriteTablePath.Result.Builder builder =
+ ImmutableRewriteTablePath.Result.builder()
+ .stagingLocation(stagingDir)
+ .deleteFilesCount(deleteFiles.size())
+ .metadataFilesCount(metaFiles.size())
+ .latestVersion(RewriteTablePathUtil.fileName(endVersionName));
+
+ // skip file list
+ if (skipFileList) {
+ return builder.fileListLocation("").build();
Review Comment:
can you return `builder.build()` instead?
##########
spark/v4.0/spark-extensions/src/test/java/org/apache/iceberg/spark/extensions/TestRewriteTablePathProcedure.java:
##########
@@ -171,6 +171,23 @@ public void testProcedureWithInvalidInput() {
"Cannot find provided version file %s in metadata log.", "v11.metadata.json");
}
+ @TestTemplate
+ public void testRewriteTablePathWithSkipFileList() {
Review Comment:
Let's add another test where we rewritten delete and manifest file count is no zero and assert on the procedure output?
##########
api/src/main/java/org/apache/iceberg/actions/RewriteTablePath.java:
##########
@@ -112,5 +122,15 @@ interface Result {
/** Name of latest metadata file version */
String latestVersion();
+
+ /** count of rewrite delete files, default value is 0 */
+ default int deleteFilesCount() {
+ return 0;
+ }
+
+ /** count of rewrite metadata files involved, default value is 0 */
+ default int metadataFilesCount() {
+ return 0;
Review Comment:
I think rewrite metadata files can be confused with metadata.json files, if this is count of rewritten manifest files. let's call it rewrittenManifestFilesCount? Same wise for rewrittenDeleteFilesCount above.
##########
api/src/main/java/org/apache/iceberg/actions/RewriteTablePath.java:
##########
@@ -112,5 +122,15 @@ interface Result {
/** Name of latest metadata file version */
String latestVersion();
+
+ /** count of rewrite delete files, default value is 0 */
+ default int deleteFilesCount() {
+ return 0;
+ }
+
+ /** count of rewrite metadata files involved, default value is 0 */
+ default int metadataFilesCount() {
+ return 0;
Review Comment:
Also, do we expect other counts might need for path based rewrite? I am wondering how such information can be useful for programatic access in real world scenario
##########
spark/v4.0/spark/src/main/java/org/apache/iceberg/spark/actions/RewriteTablePathSparkAction.java:
##########
@@ -263,7 +265,7 @@ private String jobDesc() {
* <li>Get all files needed to move
* </ul>
*/
- private String rebuildMetadata() {
+ private Result rebuildMetadata() {
Review Comment:
I think we shall add a corresponding test in TestRewriteTablePathsAction
--
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] API, Spark 4.0: Add 'skip_file_list' option to RewriteTablePathProcedure for optional file-list generation. [iceberg]
Posted by "dramaticlly (via GitHub)" <gi...@apache.org>.
dramaticlly commented on code in PR #13837:
URL: https://github.com/apache/iceberg/pull/13837#discussion_r2291811651
##########
spark/v4.0/spark-extensions/src/test/java/org/apache/iceberg/spark/extensions/TestRewriteTablePathProcedure.java:
##########
@@ -171,8 +177,79 @@ public void testProcedureWithInvalidInput() {
"Cannot find provided version file %s in metadata log.", "v11.metadata.json");
}
+ @TestTemplate
+ public void testRewriteTablePathWithSkipFileList() {
+ String location = targetTableDir.toFile().toURI().toString();
+ Table table = validationCatalog.loadTable(tableIdent);
+ String metadataJson = TableUtil.metadataFileLocation(table);
+
+ List<Object[]> result =
+ sql(
+ "CALL %s.system.rewrite_table_path(table => '%s', source_prefix => '%s', target_prefix => '%s', skip_file_list => true)",
+ catalogName, tableIdent, table.location(), location);
+ assertThat(result).hasSize(1);
+ assertThat(result.get(0)[0])
+ .as("Should return correct latest version")
+ .isEqualTo(RewriteTablePathUtil.fileName(metadataJson));
+ assertThat(result.get(0)[1]).as("Should return empty").asString().isEqualTo("");
+ }
+
private void checkFileListLocationCount(String fileListLocation, long expectedFileCount) {
long fileCount = spark.read().format("text").load(fileListLocation).count();
assertThat(fileCount).isEqualTo(expectedFileCount);
}
+
+ @TestTemplate
+ public void testRewriteTablePathWithManifestAndDeleteCounts() throws IOException {
+
+ sql("INSERT INTO %s VALUES (1, 'a')", tableName);
+ sql("INSERT INTO %s VALUES (2, 'b')", tableName);
+ sql("INSERT INTO %s VALUES (3, 'c')", tableName);
+
+ Table table = validationCatalog.loadTable(tableIdent);
+ List<Pair<CharSequence, Long>> deletes =
+ Lists.newArrayList(
+ Pair.of(
+ table.currentSnapshot().addedDataFiles(table.io()).iterator().next().location(),
+ 0L));
+
+ File file = new File(removePrefix(table.location()) + "/data/deletes.parquet");
+ DeleteFile positionDeletes =
+ FileHelpers.writeDeleteFile(
+ table, table.io().newOutputFile(file.toURI().toString()), deletes)
+ .first();
+
+ table.newRowDelta().addDeletes(positionDeletes).commit();
+
+ sql("INSERT INTO %s VALUES (4, 'd')", tableName);
+
+ String targetLocation = targetTableDir.toFile().toURI().toString();
+ String stagingLocation = staging.toFile().toURI().toString();
+
+ List<Object[]> result =
+ sql(
+ "CALL %s.system.rewrite_table_path("
+ + "table => '%s', "
+ + "source_prefix => '%s', "
+ + "target_prefix => '%s', "
+ + "staging_location => '%s')",
+ catalogName, tableIdent, table.location(), targetLocation, stagingLocation);
+
+ assertThat(result).hasSize(1);
+ Object[] row = result.get(0);
+
+ int rewrittenManifestFilesCount = ((Number) row[2]).intValue();
+ int rewrittenDeleteFilesCount = ((Number) row[3]).intValue();
+
+ assertThat(rewrittenDeleteFilesCount)
+ .as("Should rewrite at least one delete file")
+ .isGreaterThan(0);
+ assertThat(rewrittenManifestFilesCount)
+ .as("Should rewrite at least one manifest file")
+ .isGreaterThan(0);
Review Comment:
can we assert on equality of those rewritten file count? This is to ensure we have number correctly in procedure output
##########
spark/v4.0/spark/src/main/java/org/apache/iceberg/spark/actions/RewriteTablePathSparkAction.java:
##########
@@ -304,12 +307,29 @@ private String rebuildMetadata() {
.collect(Collectors.toSet());
rewritePositionDeletes(deleteFiles);
+ ImmutableRewriteTablePath.Result.Builder builder =
+ ImmutableRewriteTablePath.Result.builder()
+ .stagingLocation(stagingDir)
+ .rewrittenDeleteFilesCount(deleteFiles.size())
+ .rewrittenManifestFilesCount(metaFiles.size())
+ .latestVersion(RewriteTablePathUtil.fileName(endVersionName));
+
+ // skip file list
+ if (skipFileList) {
+ return builder.build();
+ }
+
Set<Pair<String, String>> copyPlan = Sets.newHashSet();
copyPlan.addAll(rewriteVersionResult.copyPlan());
copyPlan.addAll(rewriteManifestListResult.copyPlan());
copyPlan.addAll(rewriteManifestResult.copyPlan());
+ String fileListLocation = saveFileList(copyPlan);
- return saveFileList(copyPlan);
+ return builder
+ .stagingLocation(stagingDir)
+ .fileListLocation(fileListLocation)
Review Comment:
@nastra I am curious, does API contract allow String typed fileListLocation to have value null? I assume line 319 once built without fileListLocation will have its value = null
--
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] API, Spark 4.0: Add 'skip_file_list' option to RewriteTablePathProcedure for optional file-list generation. [iceberg]
Posted by "slfan1989 (via GitHub)" <gi...@apache.org>.
slfan1989 commented on code in PR #13837:
URL: https://github.com/apache/iceberg/pull/13837#discussion_r2311512019
##########
open-api/src/testFixtures/java/org/apache/iceberg/rest/RESTCatalogServer.java:
##########
@@ -86,7 +86,7 @@ private CatalogContext initializeBackendCatalog() throws IOException {
if (warehouseLocation == null) {
File tmp = java.nio.file.Files.createTempDirectory("iceberg_warehouse").toFile();
tmp.deleteOnExit();
- warehouseLocation = new File(tmp, "iceberg_data").getAbsolutePath();
+ warehouseLocation = new File(tmp, "iceberg_data").toURI().toString();
Review Comment:
@RussellSpitzer I initially intended to make this change in PR #13837, but since this change seems inconsistent with the core modifications of #13837, I have extracted it into PR #13882 for processing.
--
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] API, Spark 4.0: Add 'skip_file_list' option to RewriteTablePathProcedure for optional file-list generation. [iceberg]
Posted by "manuzhang (via GitHub)" <gi...@apache.org>.
manuzhang commented on code in PR #13837:
URL: https://github.com/apache/iceberg/pull/13837#discussion_r2281194879
##########
api/src/main/java/org/apache/iceberg/actions/RewriteTablePath.java:
##########
@@ -86,6 +86,16 @@ public interface RewriteTablePath extends Action<RewriteTablePath, RewriteTableP
*/
RewriteTablePath stagingLocation(String stagingLocation);
+ /**
+ * Whether to skip saving the file list location.
+ *
+ * @param skipFileList true to skip saving the file list, false to include it
+ * @return this instance for method chaining
+ */
+ default RewriteTablePath saveFileList(boolean skipFileList) {
Review Comment:
The method name and the parameter name have opposite meanings.
`saveFileList(true)` is actually not to save file list.
--
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] API, Spark 4.0: Add 'skip_file_list' option to RewriteTablePathProcedure for optional file-list generation. [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #13837:
URL: https://github.com/apache/iceberg/pull/13837#discussion_r2290273696
##########
spark/v4.0/spark-extensions/src/test/java/org/apache/iceberg/spark/extensions/TestRewriteTablePathProcedure.java:
##########
@@ -171,8 +177,79 @@ public void testProcedureWithInvalidInput() {
"Cannot find provided version file %s in metadata log.", "v11.metadata.json");
}
+ @TestTemplate
+ public void testRewriteTablePathWithSkipFileList() {
Review Comment:
```suggestion
public void testRewriteTablePathWithoutFileList() {
```
--
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] API, Spark 4.0: Add 'skip_file_list' option to RewriteTablePathProcedure for optional file-list generation. [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #13837:
URL: https://github.com/apache/iceberg/pull/13837#discussion_r2290333510
##########
spark/v4.0/spark/src/main/java/org/apache/iceberg/spark/actions/RewriteTablePathSparkAction.java:
##########
@@ -304,12 +307,29 @@ private String rebuildMetadata() {
.collect(Collectors.toSet());
rewritePositionDeletes(deleteFiles);
+ ImmutableRewriteTablePath.Result.Builder builder =
+ ImmutableRewriteTablePath.Result.builder()
+ .stagingLocation(stagingDir)
+ .rewrittenDeleteFilesCount(deleteFiles.size())
+ .rewrittenManifestFilesCount(metaFiles.size())
+ .latestVersion(RewriteTablePathUtil.fileName(endVersionName));
+
+ // skip file list
+ if (skipFileList) {
+ return builder.build();
+ }
+
Set<Pair<String, String>> copyPlan = Sets.newHashSet();
copyPlan.addAll(rewriteVersionResult.copyPlan());
copyPlan.addAll(rewriteManifestListResult.copyPlan());
copyPlan.addAll(rewriteManifestResult.copyPlan());
+ String fileListLocation = saveFileList(copyPlan);
- return saveFileList(copyPlan);
+ return builder
+ .stagingLocation(stagingDir)
+ .fileListLocation(fileListLocation)
Review Comment:
I think another issue is actually that `fileListLocation` is as mandatory property in https://github.com/apache/iceberg/blob/26259d8d0d4e8cc19d5fb8b984c42e8f9b0c6488/api/src/main/java/org/apache/iceberg/actions/RewriteTablePath.java#L121 (it's using Immutables and anything not marked as optional or nullable is a required field to be set when the object is being created). That being said, we can't make it optional/nullable at the API level, as that would break the API contract. Are we going to put a dummy string in there?
--
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