You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by GitBox <gi...@apache.org> on 2022/07/30 08:20:32 UTC
[GitHub] [iceberg] manuzhang opened a new pull request, #5392: Spark: Fix a separate table cache being created for each rewriteFiles
manuzhang opened a new pull request, #5392:
URL: https://github.com/apache/iceberg/pull/5392
Currently, during Spark's rewrite data files procedure with bin pack strategy, `SparkSession` is cloned to disable AQE in each `rewriteFiles`. Since a cloned `SparkSession` has its own state, V2SessionCatalog is reloaded every time and a separate table cache is created. That means each file group has its own table cache and effectively disables the table cache.
This PR fixes it by cloning `SparkSession` when creating `SparkBinPackStrategy`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] manuzhang commented on pull request #5392: Spark: Fix a separate table cache being created for each rewriteFiles
Posted by GitBox <gi...@apache.org>.
manuzhang commented on PR #5392:
URL: https://github.com/apache/iceberg/pull/5392#issuecomment-1229668900
Gentle ping @rdblue @aokolnychyi @kbendick for another 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
[GitHub] [iceberg] manuzhang commented on pull request #5392: Spark: Fix a separate table cache being created for each rewriteFiles
Posted by GitBox <gi...@apache.org>.
manuzhang commented on PR #5392:
URL: https://github.com/apache/iceberg/pull/5392#issuecomment-1311199510
@RussellSpitzer @rdblue @ajantha-bhat please take another look. Thanks.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] manuzhang commented on pull request #5392: Spark: Fix a separate table cache being created for each rewriteFiles
Posted by GitBox <gi...@apache.org>.
manuzhang commented on PR #5392:
URL: https://github.com/apache/iceberg/pull/5392#issuecomment-1200116054
@rdblue @aokolnychyi @kbendick Please help review and suggest how to add a UT. I haven't found a proper place to add 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
[GitHub] [iceberg] manuzhang commented on pull request #5392: Spark: Fix a separate table cache being created for each rewriteFiles
Posted by GitBox <gi...@apache.org>.
manuzhang commented on PR #5392:
URL: https://github.com/apache/iceberg/pull/5392#issuecomment-1229668900
Gentle ping @rdblue @aokolnychyi @kbendick for another 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
[GitHub] [iceberg] rdblue commented on a diff in pull request #5392: Spark: Fix a separate table cache being created for each rewriteFiles
Posted by GitBox <gi...@apache.org>.
rdblue commented on code in PR #5392:
URL: https://github.com/apache/iceberg/pull/5392#discussion_r934011732
##########
spark/v3.3/spark/src/main/java/org/apache/iceberg/spark/actions/RewriteDataFilesSparkAction.java:
##########
@@ -489,7 +490,10 @@ private String jobDesc(RewriteFileGroup group, RewriteExecutionContext ctx) {
}
private BinPackStrategy binPackStrategy() {
- return new SparkBinPackStrategy(table, spark());
+ // Disable Adaptive Query Execution as this may change the output partitioning of our write
+ SparkSession spark = spark().cloneSession();
+ spark.conf().set(SQLConf.ADAPTIVE_EXECUTION_ENABLED().key(), false);
+ return new SparkBinPackStrategy(table, spark);
Review Comment:
Bin packing is used as the basis for the other strategies. If you're moving this into the action itself, then you should move it into `spark()`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] kbendick commented on pull request #5392: Spark: Fix a separate table cache being created for each rewriteFiles
Posted by GitBox <gi...@apache.org>.
kbendick commented on PR #5392:
URL: https://github.com/apache/iceberg/pull/5392#issuecomment-1200356951
Particularly properly quoted namespaces in table names. At least I assume that's the cache you're talking about. Please correct me if I'm mistaken!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] manuzhang commented on a diff in pull request #5392: Spark: Fix a separate table cache being created for each rewriteFiles
Posted by GitBox <gi...@apache.org>.
manuzhang commented on code in PR #5392:
URL: https://github.com/apache/iceberg/pull/5392#discussion_r934067632
##########
spark/v3.3/spark/src/main/java/org/apache/iceberg/spark/actions/RewriteDataFilesSparkAction.java:
##########
@@ -489,7 +490,10 @@ private String jobDesc(RewriteFileGroup group, RewriteExecutionContext ctx) {
}
private BinPackStrategy binPackStrategy() {
- return new SparkBinPackStrategy(table, spark());
+ // Disable Adaptive Query Execution as this may change the output partitioning of our write
+ SparkSession spark = spark().cloneSession();
+ spark.conf().set(SQLConf.ADAPTIVE_EXECUTION_ENABLED().key(), false);
+ return new SparkBinPackStrategy(table, spark);
Review Comment:
If it's moved into `spark()` then SparkSession is cloned with AQE disabled for all `BaseSparkAction`s. I'm not sure about the side-effect.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] manuzhang commented on a diff in pull request #5392: Spark: Fix a separate table cache being created for each rewriteFiles
Posted by GitBox <gi...@apache.org>.
manuzhang commented on code in PR #5392:
URL: https://github.com/apache/iceberg/pull/5392#discussion_r1001312229
##########
spark/v3.3/spark/src/main/java/org/apache/iceberg/spark/actions/SparkZOrderStrategy.java:
##########
@@ -202,17 +202,14 @@ public Set<DataFile> rewriteFiles(List<FileScanTask> filesToRewrite) {
tableCache().add(groupID, table());
manager().stageTasks(table(), groupID, filesToRewrite);
- // Disable Adaptive Query Execution as this may change the output partitioning of our write
- SparkSession cloneSession = spark().cloneSession();
- cloneSession.conf().set(SQLConf.ADAPTIVE_EXECUTION_ENABLED().key(), false);
-
+ SparkSession spark = spark();
Review Comment:
this is not cloning but `spark()` from `SparkSortStrategy`
##########
spark/v3.3/spark/src/main/java/org/apache/iceberg/spark/actions/SparkZOrderStrategy.java:
##########
@@ -202,17 +202,14 @@ public Set<DataFile> rewriteFiles(List<FileScanTask> filesToRewrite) {
tableCache().add(groupID, table());
manager().stageTasks(table(), groupID, filesToRewrite);
- // Disable Adaptive Query Execution as this may change the output partitioning of our write
- SparkSession cloneSession = spark().cloneSession();
- cloneSession.conf().set(SQLConf.ADAPTIVE_EXECUTION_ENABLED().key(), false);
-
+ SparkSession spark = spark();
Review Comment:
this is not cloning again but `spark()` from `SparkSortStrategy`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] ajantha-bhat commented on a diff in pull request #5392: Spark: Fix a separate table cache being created for each rewriteFiles
Posted by GitBox <gi...@apache.org>.
ajantha-bhat commented on code in PR #5392:
URL: https://github.com/apache/iceberg/pull/5392#discussion_r1001317919
##########
spark/v3.3/spark/src/main/java/org/apache/iceberg/spark/actions/SparkZOrderStrategy.java:
##########
@@ -202,17 +202,14 @@ public Set<DataFile> rewriteFiles(List<FileScanTask> filesToRewrite) {
tableCache().add(groupID, table());
manager().stageTasks(table(), groupID, filesToRewrite);
- // Disable Adaptive Query Execution as this may change the output partitioning of our write
- SparkSession cloneSession = spark().cloneSession();
- cloneSession.conf().set(SQLConf.ADAPTIVE_EXECUTION_ENABLED().key(), false);
-
+ SparkSession spark = spark();
Review Comment:
Got it. This similar method name is really confusing. Just by reading this block of the code couldn't figure out spark() was also there in `SparkSortStrategy` and it is not from `RewriteDataFilesSparkAction`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] manuzhang commented on pull request #5392: Spark: Fix a separate table cache being created for each rewriteFiles
Posted by GitBox <gi...@apache.org>.
manuzhang commented on PR #5392:
URL: https://github.com/apache/iceberg/pull/5392#issuecomment-1219098499
@rdblue and @kbendick any more comments?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] RussellSpitzer commented on a diff in pull request #5392: Spark: Fix a separate table cache being created for each rewriteFiles
Posted by GitBox <gi...@apache.org>.
RussellSpitzer commented on code in PR #5392:
URL: https://github.com/apache/iceberg/pull/5392#discussion_r1013144623
##########
spark/v3.3/spark/src/main/java/org/apache/iceberg/spark/actions/RewriteDataFilesSparkAction.java:
##########
@@ -94,10 +95,14 @@
private boolean useStartingSequenceNumber;
private RewriteJobOrder rewriteJobOrder;
private RewriteStrategy strategy = null;
+ private final SparkSession cloneSession;
RewriteDataFilesSparkAction(SparkSession spark, Table table) {
super(spark);
Review Comment:
I think this needs to be the cloned session, otherwise the subclass calls would be modifying the original session and not the new cloned one correct?
Like here -
https://github.com/apache/iceberg/pull/5392/files#diff-39b303771b5d730c63672bb27597474e2b84a7f1b4b3f8b22fb58352c34f8968R206
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] manuzhang commented on a diff in pull request #5392: Spark: Fix a separate table cache being created for each rewriteFiles
Posted by GitBox <gi...@apache.org>.
manuzhang commented on code in PR #5392:
URL: https://github.com/apache/iceberg/pull/5392#discussion_r1001282956
##########
spark/v3.3/spark/src/main/java/org/apache/iceberg/spark/actions/RewriteDataFilesSparkAction.java:
##########
@@ -185,6 +186,14 @@ public RewriteDataFiles.Result execute() {
}
}
+ @Override
+ protected SparkSession spark() {
Review Comment:
Not sure what you mean by "copy". It is now cloning the SparkSession for this action on `binPackStrategy`, `sortStrategy` and `zOrderStrategy`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue merged pull request #5392: Spark: Fix a separate table cache being created for each rewriteFiles
Posted by GitBox <gi...@apache.org>.
rdblue merged PR #5392:
URL: https://github.com/apache/iceberg/pull/5392
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a diff in pull request #5392: Spark: Fix a separate table cache being created for each rewriteFiles
Posted by GitBox <gi...@apache.org>.
rdblue commented on code in PR #5392:
URL: https://github.com/apache/iceberg/pull/5392#discussion_r1000885175
##########
spark/v3.3/spark/src/main/java/org/apache/iceberg/spark/actions/SparkBinPackStrategy.java:
##########
@@ -60,12 +59,8 @@ public Set<DataFile> rewriteFiles(List<FileScanTask> filesToRewrite) {
tableCache.add(groupID, table);
manager.stageTasks(table, groupID, filesToRewrite);
- // Disable Adaptive Query Execution as this may change the output partitioning of our write
- SparkSession cloneSession = spark.cloneSession();
- cloneSession.conf().set(SQLConf.ADAPTIVE_EXECUTION_ENABLED().key(), false);
-
Dataset<Row> scanDF =
- cloneSession
+ spark
Review Comment:
Does this work? It isn't calling `spark()` that was overridden.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] ajantha-bhat commented on a diff in pull request #5392: Spark: Fix a separate table cache being created for each rewriteFiles
Posted by GitBox <gi...@apache.org>.
ajantha-bhat commented on code in PR #5392:
URL: https://github.com/apache/iceberg/pull/5392#discussion_r1001296692
##########
spark/v3.3/spark/src/main/java/org/apache/iceberg/spark/actions/SparkBinPackStrategy.java:
##########
@@ -60,12 +59,8 @@ public Set<DataFile> rewriteFiles(List<FileScanTask> filesToRewrite) {
tableCache.add(groupID, table);
manager.stageTasks(table, groupID, filesToRewrite);
- // Disable Adaptive Query Execution as this may change the output partitioning of our write
- SparkSession cloneSession = spark.cloneSession();
- cloneSession.conf().set(SQLConf.ADAPTIVE_EXECUTION_ENABLED().key(), false);
-
Dataset<Row> scanDF =
- cloneSession
+ spark
Review Comment:
Isn't the caller passing this `spark` object by calling overridden `spark()`?
https://github.com/apache/iceberg/blob/master/spark/v3.3/spark/src/main/java/org/apache/iceberg/spark/actions/RewriteDataFilesSparkAction.java#L492
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on pull request #5392: Spark: Fix a separate table cache being created for each rewriteFiles
Posted by GitBox <gi...@apache.org>.
rdblue commented on PR #5392:
URL: https://github.com/apache/iceberg/pull/5392#issuecomment-1200275092
I'm interested to hear what @szehon-ho and @RussellSpitzer think about this.
My initial reaction is that this is not something that we should change. We don't want to disable AQE for other Spark work, which is a side-effect of this change. I also don't like that we need to create a new Spark session for each rewrite, but I don't think there is much we can do to avoid it if we want to disable AQE. We could also fail if AQE is on or just accept the AQE results.
Also, is a separate table cache a bug? Since it is only used once, what is the problem with doing it this way? Sure, this won't cache the rewritten table, but is there a behavior problem or are loads just slightly slower?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] kbendick commented on pull request #5392: Spark: Fix a separate table cache being created for each rewriteFiles
Posted by GitBox <gi...@apache.org>.
kbendick commented on PR #5392:
URL: https://github.com/apache/iceberg/pull/5392#issuecomment-1200357123
Ignore my previous comments I had the caches mistaken.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on pull request #5392: Spark: Fix a separate table cache being created for each rewriteFiles
Posted by GitBox <gi...@apache.org>.
rdblue commented on PR #5392:
URL: https://github.com/apache/iceberg/pull/5392#issuecomment-1201838950
> Do you mean session was created for the entire rewrite before?
I'm asking you what the behavior was before this change that you want to fix.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] ajantha-bhat commented on a diff in pull request #5392: Spark: Fix a separate table cache being created for each rewriteFiles
Posted by GitBox <gi...@apache.org>.
ajantha-bhat commented on code in PR #5392:
URL: https://github.com/apache/iceberg/pull/5392#discussion_r1001296692
##########
spark/v3.3/spark/src/main/java/org/apache/iceberg/spark/actions/SparkBinPackStrategy.java:
##########
@@ -60,12 +59,8 @@ public Set<DataFile> rewriteFiles(List<FileScanTask> filesToRewrite) {
tableCache.add(groupID, table);
manager.stageTasks(table, groupID, filesToRewrite);
- // Disable Adaptive Query Execution as this may change the output partitioning of our write
- SparkSession cloneSession = spark.cloneSession();
- cloneSession.conf().set(SQLConf.ADAPTIVE_EXECUTION_ENABLED().key(), false);
-
Dataset<Row> scanDF =
- cloneSession
+ spark
Review Comment:
Isn't the caller getting this `spark` object by calling overridden `spark()`?
https://github.com/apache/iceberg/blob/master/spark/v3.3/spark/src/main/java/org/apache/iceberg/spark/actions/RewriteDataFilesSparkAction.java#L492
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] manuzhang commented on pull request #5392: Spark: Fix a separate table cache being created for each rewriteFiles
Posted by GitBox <gi...@apache.org>.
manuzhang commented on PR #5392:
URL: https://github.com/apache/iceberg/pull/5392#issuecomment-1200325517
@rdblue I've moved into cloning session and disabling AQE into `RewriteDataFilesSparkAction`.
When multiple rewrite actions are submitted concurrently as follows, they will block each other when loading table due to locks in `HiveExternalCatalog`. That hurts the overall performance of rewrite actions.
```
CALL spark_catalog.system.rewrite_data_files(table => 'default.table', options => map('max-concurrent-file-group-rewrites', '200'));
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] manuzhang commented on pull request #5392: Spark: Fix a separate table cache being created for each rewriteFiles
Posted by GitBox <gi...@apache.org>.
manuzhang commented on PR #5392:
URL: https://github.com/apache/iceberg/pull/5392#issuecomment-1200572035
> Is that what was happening before?
Do you mean session was created for the entire rewrite before?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] manuzhang commented on pull request #5392: Spark: Fix a separate table cache being created for each rewriteFiles
Posted by GitBox <gi...@apache.org>.
manuzhang commented on PR #5392:
URL: https://github.com/apache/iceberg/pull/5392#issuecomment-1203423371
Not sure. The furthest I can track is https://github.com/apache/iceberg/pull/2591 and the behavior is same as now.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] RussellSpitzer commented on pull request #5392: Spark: Fix a separate table cache being created for each rewriteFiles
Posted by GitBox <gi...@apache.org>.
RussellSpitzer commented on PR #5392:
URL: https://github.com/apache/iceberg/pull/5392#issuecomment-1302378216
> @manuzhang, it seems reasonable to create a session for the entire rewrite, not just each Spark submission. Is that what was happening before?
Yes basically the old behavior would be to clone the Spark Session for each file group rewrite, rather than once for the entire action.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] manuzhang commented on pull request #5392: Spark: Fix a separate table cache being created for each rewriteFiles
Posted by GitBox <gi...@apache.org>.
manuzhang commented on PR #5392:
URL: https://github.com/apache/iceberg/pull/5392#issuecomment-1304384550
@RussellSpitzer please check again whether it's fixed now.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] manuzhang commented on pull request #5392: Spark: Fix a separate table cache being created for each rewriteFiles
Posted by GitBox <gi...@apache.org>.
manuzhang commented on PR #5392:
URL: https://github.com/apache/iceberg/pull/5392#issuecomment-1204787368
@rdblue Any more concerns or suggestions for this PR?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] ajantha-bhat commented on a diff in pull request #5392: Spark: Fix a separate table cache being created for each rewriteFiles
Posted by GitBox <gi...@apache.org>.
ajantha-bhat commented on code in PR #5392:
URL: https://github.com/apache/iceberg/pull/5392#discussion_r1000089334
##########
spark/v3.3/spark/src/main/java/org/apache/iceberg/spark/actions/SparkBinPackStrategy.java:
##########
@@ -60,12 +59,8 @@ public Set<DataFile> rewriteFiles(List<FileScanTask> filesToRewrite) {
tableCache.add(groupID, table);
manager.stageTasks(table, groupID, filesToRewrite);
- // Disable Adaptive Query Execution as this may change the output partitioning of our write
Review Comment:
similar problems exist in `SparkSortStrategy` and `SparkZOrderStrategy`'s
`rewriteFiles()` too.
So, it makes sense to add a method in `RewriteDataFilesSparkAction` which will return cloned spark session with disabled adaptive execution.
this new method can be called instead of `spark()` in these 3 places.
https://github.com/apache/iceberg/blob/master/spark/v3.3/spark/src/main/java/org/apache/iceberg/spark/actions/RewriteDataFilesSparkAction.java#L491-L501
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] ajantha-bhat commented on a diff in pull request #5392: Spark: Fix a separate table cache being created for each rewriteFiles
Posted by GitBox <gi...@apache.org>.
ajantha-bhat commented on code in PR #5392:
URL: https://github.com/apache/iceberg/pull/5392#discussion_r1000092705
##########
spark/v3.3/spark/src/main/java/org/apache/iceberg/spark/actions/SparkBinPackStrategy.java:
##########
@@ -60,12 +59,8 @@ public Set<DataFile> rewriteFiles(List<FileScanTask> filesToRewrite) {
tableCache.add(groupID, table);
manager.stageTasks(table, groupID, filesToRewrite);
- // Disable Adaptive Query Execution as this may change the output partitioning of our write
Review Comment:
Instead of the new method, `Override` is also fine as @kbendick suggested because `spark()` is called only once per `RewriteDataFilesSparkAction`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on pull request #5392: Spark: Fix a separate table cache being created for each rewriteFiles
Posted by GitBox <gi...@apache.org>.
rdblue commented on PR #5392:
URL: https://github.com/apache/iceberg/pull/5392#issuecomment-1328357822
I think that all of the session references are to the cloned Spark session now. +1.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] manuzhang commented on pull request #5392: Spark: Fix a separate table cache being created for each rewriteFiles
Posted by GitBox <gi...@apache.org>.
manuzhang commented on PR #5392:
URL: https://github.com/apache/iceberg/pull/5392#issuecomment-1328489707
Thanks @rdblue @ajantha-bhat @RussellSpitzer @kbendick @hililiwei for your review.
I opened #6284 and #6285 to back-port the PR to Spark 3.2 and 3.1 respectively. Please help review those PRs as well.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] ajantha-bhat commented on pull request #5392: Spark: Fix a separate table cache being created for each rewriteFiles
Posted by GitBox <gi...@apache.org>.
ajantha-bhat commented on PR #5392:
URL: https://github.com/apache/iceberg/pull/5392#issuecomment-1286388724
> @ajantha-bhat I forgot to remove cloneSession in SparkSortStrategy and SparkZOrderStrategy. Please review again.
Yeah, last time I gave a comment that these other places also have issues and I forgot to recheck😔 whether it was addressed or not. I think now we cover all the places. Thanks.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] kbendick commented on a diff in pull request #5392: Spark: Fix a separate table cache being created for each rewriteFiles
Posted by GitBox <gi...@apache.org>.
kbendick commented on code in PR #5392:
URL: https://github.com/apache/iceberg/pull/5392#discussion_r941724072
##########
spark/v3.3/spark/src/main/java/org/apache/iceberg/spark/actions/RewriteDataFilesSparkAction.java:
##########
@@ -489,7 +490,10 @@ private String jobDesc(RewriteFileGroup group, RewriteExecutionContext ctx) {
}
private BinPackStrategy binPackStrategy() {
- return new SparkBinPackStrategy(table, spark());
+ // Disable Adaptive Query Execution as this may change the output partitioning of our write
+ SparkSession spark = spark().cloneSession();
+ spark.conf().set(SQLConf.ADAPTIVE_EXECUTION_ENABLED().key(), false);
+ return new SparkBinPackStrategy(table, spark);
Review Comment:
Given that `spark()` is protected in `BaseSparkAction`, it could be overridden within this action.
Or another `spark()` method could be added that clones and disables AQE, like `spark(boolean cloneAndDisableAQE)` or something.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] manuzhang commented on a diff in pull request #5392: Spark: Fix a separate table cache being created for each rewriteFiles
Posted by GitBox <gi...@apache.org>.
manuzhang commented on code in PR #5392:
URL: https://github.com/apache/iceberg/pull/5392#discussion_r943054882
##########
spark/v3.3/spark/src/main/java/org/apache/iceberg/spark/actions/RewriteDataFilesSparkAction.java:
##########
@@ -489,7 +490,10 @@ private String jobDesc(RewriteFileGroup group, RewriteExecutionContext ctx) {
}
private BinPackStrategy binPackStrategy() {
- return new SparkBinPackStrategy(table, spark());
+ // Disable Adaptive Query Execution as this may change the output partitioning of our write
+ SparkSession spark = spark().cloneSession();
+ spark.conf().set(SQLConf.ADAPTIVE_EXECUTION_ENABLED().key(), false);
+ return new SparkBinPackStrategy(table, spark);
Review Comment:
I don't see the difference here since we need to invoke the `spark()` from super class to get SparkSession anyway.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] manuzhang commented on a diff in pull request #5392: Spark: Fix a separate table cache being created for each rewriteFiles
Posted by GitBox <gi...@apache.org>.
manuzhang commented on code in PR #5392:
URL: https://github.com/apache/iceberg/pull/5392#discussion_r943054882
##########
spark/v3.3/spark/src/main/java/org/apache/iceberg/spark/actions/RewriteDataFilesSparkAction.java:
##########
@@ -489,7 +490,10 @@ private String jobDesc(RewriteFileGroup group, RewriteExecutionContext ctx) {
}
private BinPackStrategy binPackStrategy() {
- return new SparkBinPackStrategy(table, spark());
+ // Disable Adaptive Query Execution as this may change the output partitioning of our write
+ SparkSession spark = spark().cloneSession();
+ spark.conf().set(SQLConf.ADAPTIVE_EXECUTION_ENABLED().key(), false);
+ return new SparkBinPackStrategy(table, spark);
Review Comment:
I don't see the difference here since we need to invoke the `spark()` from super class first.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] ajantha-bhat commented on a diff in pull request #5392: Spark: Fix a separate table cache being created for each rewriteFiles
Posted by GitBox <gi...@apache.org>.
ajantha-bhat commented on code in PR #5392:
URL: https://github.com/apache/iceberg/pull/5392#discussion_r1001304035
##########
spark/v3.3/spark/src/main/java/org/apache/iceberg/spark/actions/RewriteDataFilesSparkAction.java:
##########
@@ -185,6 +186,14 @@ public RewriteDataFiles.Result execute() {
}
}
+ @Override
+ protected SparkSession spark() {
Review Comment:
I think what he meant is that, have a local variable of `SparkSession` in `RewriteDataFilesSparkAction`, initialize it once by cloning the session and disabling adaptive encoding.
Whenever spark() is called, return that variable instead of cloning again and 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
[GitHub] [iceberg] manuzhang commented on pull request #5392: Spark: Fix a separate table cache being created for each rewriteFiles
Posted by GitBox <gi...@apache.org>.
manuzhang commented on PR #5392:
URL: https://github.com/apache/iceberg/pull/5392#issuecomment-1286378817
@ajantha-bhat I forgot to remove cloneSession in `SparkSortStrategy` and `SparkZOrderStrategy`. Please review 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
[GitHub] [iceberg] RussellSpitzer commented on a diff in pull request #5392: Spark: Fix a separate table cache being created for each rewriteFiles
Posted by GitBox <gi...@apache.org>.
RussellSpitzer commented on code in PR #5392:
URL: https://github.com/apache/iceberg/pull/5392#discussion_r1013157205
##########
spark/v3.3/spark/src/main/java/org/apache/iceberg/spark/actions/RewriteDataFilesSparkAction.java:
##########
@@ -94,10 +95,14 @@
private boolean useStartingSequenceNumber;
private RewriteJobOrder rewriteJobOrder;
private RewriteStrategy strategy = null;
+ private final SparkSession cloneSession;
RewriteDataFilesSparkAction(SparkSession spark, Table table) {
super(spark);
Review Comment:
I think not using the same session will probably also have some impacts on some of the other BaseSparkAction functions like "withJobGroupInfo"
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] kbendick commented on pull request #5392: Spark: Fix a separate table cache being created for each rewriteFiles
Posted by GitBox <gi...@apache.org>.
kbendick commented on PR #5392:
URL: https://github.com/apache/iceberg/pull/5392#issuecomment-1200356790
I'm just catching up on this, but I wanted to address one comment:
> Also, is a separate table cache a bug? Since it is only used once, what is the problem with doing it this way? Sure, this won't cache the rewritten table, but is there a behavior problem or are loads just slightly slower?
The cached table here is admittedly somewhat of a hack in that it's main usage is to deal with issues we had prior to its introduction around properly quoted table names. Actually prior to its usage we passed the full name and namespace around entirely.
So there very well could be another way of organizing this code, but the cache was cleaner than the previous fix (and might have had some added benefit too).
Anton introduced this cache in https://github.com/apache/iceberg/pull/5247, which removed the temporarily added `FULL_IDENT` constant that needed to be passed through everywhere.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] ajantha-bhat commented on a diff in pull request #5392: Spark: Fix a separate table cache being created for each rewriteFiles
Posted by GitBox <gi...@apache.org>.
ajantha-bhat commented on code in PR #5392:
URL: https://github.com/apache/iceberg/pull/5392#discussion_r1001302158
##########
spark/v3.3/spark/src/main/java/org/apache/iceberg/spark/actions/SparkZOrderStrategy.java:
##########
@@ -235,9 +232,9 @@ public Set<DataFile> rewriteFiles(List<FileScanTask> filesToRewrite) {
Dataset<Row> zvalueDF = scanDF.withColumn(Z_COLUMN, zOrderUDF.interleaveBytes(zvalueArray));
- SQLConf sqlConf = cloneSession.sessionState().conf();
+ SQLConf sqlConf = spark().sessionState().conf();
Review Comment:
should be `spark` instead of `spark()`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a diff in pull request #5392: Spark: Fix a separate table cache being created for each rewriteFiles
Posted by GitBox <gi...@apache.org>.
rdblue commented on code in PR #5392:
URL: https://github.com/apache/iceberg/pull/5392#discussion_r1000884781
##########
spark/v3.3/spark/src/main/java/org/apache/iceberg/spark/actions/RewriteDataFilesSparkAction.java:
##########
@@ -185,6 +186,14 @@ public RewriteDataFiles.Result execute() {
}
}
+ @Override
+ protected SparkSession spark() {
Review Comment:
This may only be called once right now, but I think we should not assume that it always will be. Can you update this to keep a copy of the session for this action?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] ajantha-bhat commented on a diff in pull request #5392: Spark: Fix a separate table cache being created for each rewriteFiles
Posted by GitBox <gi...@apache.org>.
ajantha-bhat commented on code in PR #5392:
URL: https://github.com/apache/iceberg/pull/5392#discussion_r1001320398
##########
spark/v3.3/spark/src/main/java/org/apache/iceberg/spark/actions/SparkZOrderStrategy.java:
##########
@@ -202,17 +202,14 @@ public Set<DataFile> rewriteFiles(List<FileScanTask> filesToRewrite) {
tableCache().add(groupID, table());
manager().stageTasks(table(), groupID, filesToRewrite);
- // Disable Adaptive Query Execution as this may change the output partitioning of our write
- SparkSession cloneSession = spark().cloneSession();
- cloneSession.conf().set(SQLConf.ADAPTIVE_EXECUTION_ENABLED().key(), false);
-
+ SparkSession spark = spark();
Review Comment:
Maybe we can add a comment that it is from the parent or rename the method name in the parent.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] manuzhang commented on a diff in pull request #5392: Spark: Fix a separate table cache being created for each rewriteFiles
Posted by GitBox <gi...@apache.org>.
manuzhang commented on code in PR #5392:
URL: https://github.com/apache/iceberg/pull/5392#discussion_r1001325321
##########
spark/v3.3/spark/src/main/java/org/apache/iceberg/spark/actions/SparkZOrderStrategy.java:
##########
@@ -202,17 +202,14 @@ public Set<DataFile> rewriteFiles(List<FileScanTask> filesToRewrite) {
tableCache().add(groupID, table());
manager().stageTasks(table(), groupID, filesToRewrite);
- // Disable Adaptive Query Execution as this may change the output partitioning of our write
- SparkSession cloneSession = spark().cloneSession();
- cloneSession.conf().set(SQLConf.ADAPTIVE_EXECUTION_ENABLED().key(), false);
-
+ SparkSession spark = spark();
Review Comment:
Sure, I'm wondering whether `spark` can be a protected field. The current way is a bit overuse of design patterns, 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
[GitHub] [iceberg] RussellSpitzer commented on a diff in pull request #5392: Spark: Fix a separate table cache being created for each rewriteFiles
Posted by GitBox <gi...@apache.org>.
RussellSpitzer commented on code in PR #5392:
URL: https://github.com/apache/iceberg/pull/5392#discussion_r1013141956
##########
spark/v3.3/spark/src/main/java/org/apache/iceberg/spark/actions/SparkSortStrategy.java:
##########
@@ -119,26 +119,22 @@ public Set<DataFile> rewriteFiles(List<FileScanTask> filesToRewrite) {
tableCache.add(groupID, table);
manager.stageTasks(table, groupID, filesToRewrite);
- // Disable Adaptive Query Execution as this may change the output partitioning of our write
- SparkSession cloneSession = spark.cloneSession();
- cloneSession.conf().set(SQLConf.ADAPTIVE_EXECUTION_ENABLED().key(), false);
-
// Reset Shuffle Partitions for our sort
long numOutputFiles =
numOutputFiles((long) (inputFileSize(filesToRewrite) * sizeEstimateMultiple));
- cloneSession.conf().set(SQLConf.SHUFFLE_PARTITIONS().key(), Math.max(1, numOutputFiles));
+ spark.conf().set(SQLConf.SHUFFLE_PARTITIONS().key(), Math.max(1, numOutputFiles));
Review Comment:
Actually I think this is fine since each Action get's it's own cloned session
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on pull request #5392: Spark: Fix a separate table cache being created for each rewriteFiles
Posted by GitBox <gi...@apache.org>.
rdblue commented on PR #5392:
URL: https://github.com/apache/iceberg/pull/5392#issuecomment-1328357865
Thanks, @manuzhang!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] hililiwei commented on a diff in pull request #5392: Spark: Fix a separate table cache being created for each rewriteFiles
Posted by GitBox <gi...@apache.org>.
hililiwei commented on code in PR #5392:
URL: https://github.com/apache/iceberg/pull/5392#discussion_r1000099556
##########
spark/v3.3/spark/src/main/java/org/apache/iceberg/spark/actions/SparkBinPackStrategy.java:
##########
@@ -60,12 +59,8 @@ public Set<DataFile> rewriteFiles(List<FileScanTask> filesToRewrite) {
tableCache.add(groupID, table);
manager.stageTasks(table, groupID, filesToRewrite);
- // Disable Adaptive Query Execution as this may change the output partitioning of our write
Review Comment:
> Instead of the new method, `Override` is also fine as @kbendick suggested because `spark()` is called only once per `RewriteDataFilesSparkAction`
+1
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] ajantha-bhat commented on a diff in pull request #5392: Spark: Fix a separate table cache being created for each rewriteFiles
Posted by GitBox <gi...@apache.org>.
ajantha-bhat commented on code in PR #5392:
URL: https://github.com/apache/iceberg/pull/5392#discussion_r1001301648
##########
spark/v3.3/spark/src/main/java/org/apache/iceberg/spark/actions/SparkZOrderStrategy.java:
##########
@@ -202,17 +202,14 @@ public Set<DataFile> rewriteFiles(List<FileScanTask> filesToRewrite) {
tableCache().add(groupID, table());
manager().stageTasks(table(), groupID, filesToRewrite);
- // Disable Adaptive Query Execution as this may change the output partitioning of our write
- SparkSession cloneSession = spark().cloneSession();
- cloneSession.conf().set(SQLConf.ADAPTIVE_EXECUTION_ENABLED().key(), false);
-
+ SparkSession spark = spark();
Review Comment:
instead of cloning again, Aren't we suppose to store `spark` coming in the constructor into a variable and use it 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
[GitHub] [iceberg] manuzhang commented on a diff in pull request #5392: Spark: Fix a separate table cache being created for each rewriteFiles
Posted by GitBox <gi...@apache.org>.
manuzhang commented on code in PR #5392:
URL: https://github.com/apache/iceberg/pull/5392#discussion_r1001322740
##########
spark/v3.3/spark/src/main/java/org/apache/iceberg/spark/actions/RewriteDataFilesSparkAction.java:
##########
@@ -185,6 +186,14 @@ public RewriteDataFiles.Result execute() {
}
}
+ @Override
+ protected SparkSession spark() {
Review Comment:
Good point. It looks there's no need for overriding this method but simply cloning in the constructor of `RewriteDataFilesSparkAction`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] kbendick commented on pull request #5392: Spark: Fix a separate table cache being created for each rewriteFiles
Posted by GitBox <gi...@apache.org>.
kbendick commented on PR #5392:
URL: https://github.com/apache/iceberg/pull/5392#issuecomment-1200357545
I will say that I don't love the idea of asking users to disable AQE (e.g. not cloning the session and ensuring AQE is disabled), as much as cloning the session is somewhat of a pain.
People use these statements at the end of queries and disabling AQE would be a bummer.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] manuzhang commented on pull request #5392: Spark: Fix a separate table cache being created for each rewriteFiles
Posted by GitBox <gi...@apache.org>.
manuzhang commented on PR #5392:
URL: https://github.com/apache/iceberg/pull/5392#issuecomment-1200355207
There is another lock in `SessionCatalog#tableExists` for Spark versions before https://github.com/apache/spark/pull/31891 since sessionCatalog is also shared.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on pull request #5392: Spark: Fix a separate table cache being created for each rewriteFiles
Posted by GitBox <gi...@apache.org>.
rdblue commented on PR #5392:
URL: https://github.com/apache/iceberg/pull/5392#issuecomment-1200459641
@manuzhang, it seems reasonable to create a session for the entire rewrite, not just each Spark submission. Is that what was happening before?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] manuzhang commented on pull request #5392: Spark: Fix a separate table cache being created for each rewriteFiles
Posted by GitBox <gi...@apache.org>.
manuzhang commented on PR #5392:
URL: https://github.com/apache/iceberg/pull/5392#issuecomment-1296489819
@ajantha-bhat @rdblue any more comments?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] RussellSpitzer commented on a diff in pull request #5392: Spark: Fix a separate table cache being created for each rewriteFiles
Posted by GitBox <gi...@apache.org>.
RussellSpitzer commented on code in PR #5392:
URL: https://github.com/apache/iceberg/pull/5392#discussion_r1013134444
##########
spark/v3.3/spark/src/main/java/org/apache/iceberg/spark/actions/SparkSortStrategy.java:
##########
@@ -119,26 +119,22 @@ public Set<DataFile> rewriteFiles(List<FileScanTask> filesToRewrite) {
tableCache.add(groupID, table);
manager.stageTasks(table, groupID, filesToRewrite);
- // Disable Adaptive Query Execution as this may change the output partitioning of our write
- SparkSession cloneSession = spark.cloneSession();
- cloneSession.conf().set(SQLConf.ADAPTIVE_EXECUTION_ENABLED().key(), false);
-
// Reset Shuffle Partitions for our sort
long numOutputFiles =
numOutputFiles((long) (inputFileSize(filesToRewrite) * sizeEstimateMultiple));
- cloneSession.conf().set(SQLConf.SHUFFLE_PARTITIONS().key(), Math.max(1, numOutputFiles));
+ spark.conf().set(SQLConf.SHUFFLE_PARTITIONS().key(), Math.max(1, numOutputFiles));
Review Comment:
I'm a little worried that this may be a race condition between various rewrite actions occurring at the same time in the same JVM
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a diff in pull request #5392: Spark: Fix a separate table cache being created for each rewriteFiles
Posted by GitBox <gi...@apache.org>.
rdblue commented on code in PR #5392:
URL: https://github.com/apache/iceberg/pull/5392#discussion_r1014229760
##########
spark/v3.3/spark/src/main/java/org/apache/iceberg/spark/actions/RewriteDataFilesSparkAction.java:
##########
@@ -94,10 +95,14 @@
private boolean useStartingSequenceNumber;
private RewriteJobOrder rewriteJobOrder;
private RewriteStrategy strategy = null;
+ private final SparkSession cloneSession;
RewriteDataFilesSparkAction(SparkSession spark, Table table) {
super(spark);
Review Comment:
I agree. In general, I think this should probably be overriding `spark()` so that the same context is used everywhere consistently.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] RussellSpitzer commented on a diff in pull request #5392: Spark: Fix a separate table cache being created for each rewriteFiles
Posted by GitBox <gi...@apache.org>.
RussellSpitzer commented on code in PR #5392:
URL: https://github.com/apache/iceberg/pull/5392#discussion_r1013136822
##########
spark/v3.3/spark/src/main/java/org/apache/iceberg/spark/actions/SparkZOrderStrategy.java:
##########
@@ -202,17 +202,14 @@ public Set<DataFile> rewriteFiles(List<FileScanTask> filesToRewrite) {
tableCache().add(groupID, table());
manager().stageTasks(table(), groupID, filesToRewrite);
- // Disable Adaptive Query Execution as this may change the output partitioning of our write
- SparkSession cloneSession = spark().cloneSession();
- cloneSession.conf().set(SQLConf.ADAPTIVE_EXECUTION_ENABLED().key(), false);
-
+ SparkSession spark = spark();
Review Comment:
Our current style rules don't allow for protected fields, but we could make an exception here I guess. I think long term we imagined that ZOrderStrategy will just be a special case of SortStrategy once we have the abilty to use a multi arg transform as a Sort Order.
--
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