You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "manuzhang (via GitHub)" <gi...@apache.org> on 2023/12/11 13:08:21 UTC
[PR] Spark 3.5: Add Spark application id to summary of RewriteDataFilesSparkAction [iceberg]
manuzhang opened a new pull request, #9273:
URL: https://github.com/apache/iceberg/pull/9273
(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] Core: Add EnvironmentContext to commit summary [iceberg]
Posted by "manuzhang (via GitHub)" <gi...@apache.org>.
manuzhang commented on PR #9273:
URL: https://github.com/apache/iceberg/pull/9273#issuecomment-2019398747
@nastra how can we move this 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] Core: Add EnvironmentContext to commit summary [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on PR #9273:
URL: https://github.com/apache/iceberg/pull/9273#issuecomment-2019859878
@amogh-jahagirdar or @RussellSpitzer can you guys also take a look at this please?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Add EnvironmentContext to commit summary [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #9273:
URL: https://github.com/apache/iceberg/pull/9273#discussion_r1497028069
##########
spark/v3.5/spark-extensions/src/test/java/org/apache/iceberg/spark/extensions/TestRewriteDataFilesProcedure.java:
##########
@@ -109,6 +110,7 @@ public void testRewriteDataFilesOnPartitionTable() {
List<Object[]> actualRecords = currentData();
assertEquals("Data after compaction should not change", expectedRecords, actualRecords);
+ assertThat(snapshotSummary()).containsKey(CatalogProperties.APP_ID);
Review Comment:
I think it's generally better to just add a separate small test that makes sure app-id / engine-version / engine-name are present in the summary
--
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 3.5: Add Spark application id to summary of RewriteDataFilesSparkAction [iceberg]
Posted by "ajantha-bhat (via GitHub)" <gi...@apache.org>.
ajantha-bhat commented on code in PR #9273:
URL: https://github.com/apache/iceberg/pull/9273#discussion_r1452857004
##########
spark/v3.5/spark-extensions/src/test/java/org/apache/iceberg/spark/extensions/TestRewriteDataFilesProcedure.java:
##########
@@ -848,6 +849,16 @@ public void testRewriteWithUntranslatedOrUnconvertedFilter() {
.hasMessageContaining("Cannot convert Spark filter");
}
+ @Test
+ public void testAddApplicationIdInSummary() {
+ createPartitionTable();
+ // create 5 files for each partition (c2 = 'foo' and c2 = 'bar')
+ insertData(10);
+ sql("CALL %s.system.rewrite_data_files(table => '%s')", catalogName, tableIdent);
+
+ assert (snapshotSummary().containsKey(CatalogProperties.APP_ID));
Review Comment:
nit: we can probably add this validation in `testRewriteDataFilesOnPartitionTable` itself. We may not want to increase the CI time by creating table, loading data and rewriting just to validate this.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Add EnvironmentContext to commit summary [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #9273:
URL: https://github.com/apache/iceberg/pull/9273#discussion_r1498980055
##########
spark/v3.5/spark-extensions/src/test/java/org/apache/iceberg/spark/extensions/TestRewriteDataFilesProcedure.java:
##########
@@ -848,6 +850,19 @@ public void testRewriteWithUntranslatedOrUnconvertedFilter() {
.hasMessageContaining("Cannot convert Spark filter");
}
+ @Test
+ public void testRewriteDataFilesSummary() {
+ createTable();
+ // create 10 files under non-partitioned table
+ insertData(10);
+ sql("CALL %s.system.rewrite_data_files(table => '%s')", catalogName, tableIdent);
+
+ Map<String, String> summary = snapshotSummary();
+ assertThat(summary.get(EnvironmentContext.ENGINE_NAME)).isEqualTo("spark");
Review Comment:
```suggestion
assertThat(summary)
.containsEntry(EnvironmentContext.ENGINE_NAME, "spark")
.containsKey(CatalogProperties.APP_ID)
.hasEntrySatisfying(EnvironmentContext.ENGINE_VERSION, <condition>);
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Add EnvironmentContext to commit summary [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #9273:
URL: https://github.com/apache/iceberg/pull/9273#discussion_r1497064767
##########
spark/v3.5/spark-extensions/src/test/java/org/apache/iceberg/spark/extensions/TestRewriteDataFilesProcedure.java:
##########
@@ -109,6 +110,7 @@ public void testRewriteDataFilesOnPartitionTable() {
List<Object[]> actualRecords = currentData();
assertEquals("Data after compaction should not change", expectedRecords, actualRecords);
+ assertThat(snapshotSummary()).containsKey(CatalogProperties.APP_ID);
Review Comment:
I'd vote for having a separate small test that specifically just tests that xyz is in the summary
--
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 3.5: Add Spark application id to summary of RewriteDataFilesSparkAction [iceberg]
Posted by "manuzhang (via GitHub)" <gi...@apache.org>.
manuzhang commented on PR #9273:
URL: https://github.com/apache/iceberg/pull/9273#issuecomment-1851176081
@amogh-jahagirdar @nastra could you please take a look?
--
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 3.5: Add Spark application id to summary of RewriteDataFilesSparkAction [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #9273:
URL: https://github.com/apache/iceberg/pull/9273#discussion_r1495773405
##########
core/src/main/java/org/apache/iceberg/SnapshotProducer.java:
##########
@@ -351,6 +351,7 @@ private Map<String, String> summary(TableMetadata previous) {
SnapshotSummary.ADDED_EQ_DELETES_PROP,
SnapshotSummary.REMOVED_EQ_DELETES_PROP);
+ builder.putAll(EnvironmentContext.get());
Review Comment:
can you add a simple test to `TestSnapshotSummary` to indicate that `iceberg-version` is part of the produced summary map
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Add EnvironmentContext to commit summary [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on PR #9273:
URL: https://github.com/apache/iceberg/pull/9273#issuecomment-2045466272
Sorry for the delay on reviewing this @manuzhang thanks for adding this, I think the environment details in the summary will be quite helpful. I'll go ahead and merge. Thanks @nastra @ajantha-bhat for reviewing!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Add EnvironmentContext to commit summary [iceberg]
Posted by "manuzhang (via GitHub)" <gi...@apache.org>.
manuzhang commented on code in PR #9273:
URL: https://github.com/apache/iceberg/pull/9273#discussion_r1497056414
##########
spark/v3.5/spark-extensions/src/test/java/org/apache/iceberg/spark/extensions/TestRewriteDataFilesProcedure.java:
##########
@@ -109,6 +110,7 @@ public void testRewriteDataFilesOnPartitionTable() {
List<Object[]> actualRecords = currentData();
assertEquals("Data after compaction should not change", expectedRecords, actualRecords);
+ assertThat(snapshotSummary()).containsKey(CatalogProperties.APP_ID);
Review Comment:
@ajantha-bhat suggested adding validation instead of a separate test not to increase CI time. I'm ok with both ways.
--
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 3.5: Add Spark application id to summary of RewriteDataFilesSparkAction [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #9273:
URL: https://github.com/apache/iceberg/pull/9273#discussion_r1423705750
##########
spark/v3.5/spark/src/main/java/org/apache/iceberg/spark/actions/RewritePositionDeleteFilesSparkAction.java:
##########
@@ -215,7 +215,9 @@ private ExecutorService rewriteService() {
}
private RewritePositionDeletesCommitManager commitManager() {
- return new RewritePositionDeletesCommitManager(table);
+ Map<String, String> summary = Maps.newHashMap();
+ summary.put("spark.app.id", spark().sparkContext().applicationId());
Review Comment:
other idea: what about setting the app ID in `EnvironmentContext` and then later just retrieving all properties via `EnvironmentContext.get()`?. This would also include the Iceberg and Spark version
--
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 3.5: Add Spark application id to summary of RewriteDataFilesSparkAction [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #9273:
URL: https://github.com/apache/iceberg/pull/9273#discussion_r1424962135
##########
spark/v3.5/spark/src/main/java/org/apache/iceberg/spark/actions/RewritePositionDeleteFilesSparkAction.java:
##########
@@ -215,7 +215,9 @@ private ExecutorService rewriteService() {
}
private RewritePositionDeletesCommitManager commitManager() {
- return new RewritePositionDeletesCommitManager(table);
+ Map<String, String> summary = Maps.newHashMap();
+ summary.put("spark.app.id", spark().sparkContext().applicationId());
Review Comment:
I don't know yet, I'd have to think more about potential implications of this. I'll try and find some time this week to review this
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Add EnvironmentContext to commit summary [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar merged PR #9273:
URL: https://github.com/apache/iceberg/pull/9273
--
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 3.5: Add Spark application id to summary of RewriteDataFilesSparkAction [iceberg]
Posted by "manuzhang (via GitHub)" <gi...@apache.org>.
manuzhang commented on code in PR #9273:
URL: https://github.com/apache/iceberg/pull/9273#discussion_r1423851348
##########
spark/v3.5/spark/src/main/java/org/apache/iceberg/spark/actions/RewritePositionDeleteFilesSparkAction.java:
##########
@@ -215,7 +215,9 @@ private ExecutorService rewriteService() {
}
private RewritePositionDeletesCommitManager commitManager() {
- return new RewritePositionDeletesCommitManager(table);
+ Map<String, String> summary = Maps.newHashMap();
+ summary.put("spark.app.id", spark().sparkContext().applicationId());
Review Comment:
It's already set for `SparkCatalog`. If we go down this path, can we put all of `EnvironmentContext.get()` into summary in `SnapshotProducer`?
--
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 3.5: Add Spark application id to summary of RewriteDataFilesSparkAction [iceberg]
Posted by "manuzhang (via GitHub)" <gi...@apache.org>.
manuzhang commented on code in PR #9273:
URL: https://github.com/apache/iceberg/pull/9273#discussion_r1444012677
##########
spark/v3.5/spark/src/main/java/org/apache/iceberg/spark/actions/RewritePositionDeleteFilesSparkAction.java:
##########
@@ -215,7 +215,9 @@ private ExecutorService rewriteService() {
}
private RewritePositionDeletesCommitManager commitManager() {
- return new RewritePositionDeletesCommitManager(table);
+ Map<String, String> summary = Maps.newHashMap();
+ summary.put("spark.app.id", spark().sparkContext().applicationId());
Review Comment:
@nastra do you get a chance to review this idea?
--
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 3.5: Add Spark application id to summary of RewriteDataFilesSparkAction [iceberg]
Posted by "manuzhang (via GitHub)" <gi...@apache.org>.
manuzhang commented on PR #9273:
URL: https://github.com/apache/iceberg/pull/9273#issuecomment-1855330976
The failed check is due to a flaky test. I created https://github.com/apache/iceberg/pull/9294 to fix it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Spark 3.5: Add Spark application id to summary of RewriteDataFilesSparkAction [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #9273:
URL: https://github.com/apache/iceberg/pull/9273#discussion_r1423705750
##########
spark/v3.5/spark/src/main/java/org/apache/iceberg/spark/actions/RewritePositionDeleteFilesSparkAction.java:
##########
@@ -215,7 +215,9 @@ private ExecutorService rewriteService() {
}
private RewritePositionDeletesCommitManager commitManager() {
- return new RewritePositionDeletesCommitManager(table);
+ Map<String, String> summary = Maps.newHashMap();
+ summary.put("spark.app.id", spark().sparkContext().applicationId());
Review Comment:
other idea: what about setting the app ID via `EnvironmentContext.set(..)` and then later just retrieving all properties via `EnvironmentContext.get()`?. This would also include the Iceberg and Spark version
--
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 3.5: Add Spark application id to summary of RewriteDataFilesSparkAction [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #9273:
URL: https://github.com/apache/iceberg/pull/9273#discussion_r1452605769
##########
spark/v3.5/spark/src/main/java/org/apache/iceberg/spark/actions/RewritePositionDeleteFilesSparkAction.java:
##########
@@ -215,7 +215,9 @@ private ExecutorService rewriteService() {
}
private RewritePositionDeletesCommitManager commitManager() {
- return new RewritePositionDeletesCommitManager(table);
+ Map<String, String> summary = Maps.newHashMap();
+ summary.put("spark.app.id", spark().sparkContext().applicationId());
Review Comment:
@manuzhang sorry, due to other priorities I did not have time yet to look at this, but I'll try to this week
--
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