Spark 3.4: WAP branch not propagated when using DELETE without WHERE#7900
Conversation
|
@nastra Could you please review |
nastra
left a comment
There was a problem hiding this comment.
this looks mostly good to me with a few small things. Thanks for working on this @rakesh-das08
|
@jackye1995 @aokolnychyi @RussellSpitzer could any of you also please review this? |
|
@nastra Thank you for reviewing this :) |
| spark, DataSourceV2Relation.create(metadataTable, Some.empty(), Some.empty(), options)); | ||
| } | ||
|
|
||
| public static String wapBranch(SparkSession spark, String branch) { |
There was a problem hiding this comment.
I'm not sure it's clear enough from this name what this method is doing, can we add a java doc or possibly a new name?
Looks like it returns the WAP Branch if it is configured in the Spark conf and then the string that is passed in otherwise?
There was a problem hiding this comment.
HI Russell, I have added Javadoc for this method. Please review.
There was a problem hiding this comment.
I don't think this is much clearer. What I think this method is more likely defined as "determine write branch?" It is not restricted to WAP and also validates the configuration of non WAP config? It seems like it handles multiple concerns here
There was a problem hiding this comment.
Thanks @RussellSpitzer .
Would request your review once more. Made some changes in Javadoc
There was a problem hiding this comment.
Rather than having this function do both things we should probably have two functions.
- Validate configuration
- Choose branch
In general if our javadoc says a function is doing two different things, it means that function needs to be split.
so code could look like
deleteWhere() {
validateConfig
branch = branch != null ? branch : wapBranch
if (branch != null) {
deleteFiles.toBranch(branch)
}|
Note I think this pr should also be title Spark 3.4: |
|
@RussellSpitzer, could you please review. Made changes as per your review comments. |
|
@RussellSpitzer could you please re-review this one? I think it would be great to get this into 1.3.1 |
|
|
||
| @Test | ||
| public void testDeleteToCustomWapBranchWithoutWhereClause() throws NoSuchTableException { | ||
| assumeThat(branch) |
There was a problem hiding this comment.
Why do we have this assumption? Shouldn't this be something we are configuring in the test?
|
@nastra told me this is matching the style used elsewhere, so let's merge for now and fix all of the style issues later in a followup. |
|
Thank you @RussellSpitzer for reviewing this and merging. |
* Hive: Set commit state as Unknown before throwing CommitStateUnknownException (apache#7931) (apache#8029) * Spark 3.4: WAP branch not propagated when using DELETE without WHERE (apache#7900) (apache#8028) * Core: Include all reachable snapshots with v1 format and REF snapshot mode (apache#7621) (apache#8027) * Spark 3.3: Backport 'WAP branch not propagated when using DELETE without WHERE' (apache#8033) (apache#8036) * Flink: remove the creation of default database in FlinkCatalog open method (apache#7795) (apache#8039) * Core: Handle optional fields (apache#8050) (apache#8064) * Core: Handle allow optional fields We expect: - current-snapshot-id - properties - snapshots to be there, but they are actually optional. * Use AssertJ * Core: Abort file groups should be under same lock as committerService (apache#7933) (apache#8060) * Spark 3.4: Fix rewrite_position_deletes for certain partition types (apache#8059) * Spark 3.3: Fix rewrite_position_deletes for certain partition types (apache#8059) (apache#8069) * Spark: Add actions for disater recovery. * Fix the compile error. * Fix merge conflicts and formatting * All tests are working and code integrated with Spark 3.3 * Fix union error and snapshots test * Fix Spark broadcast error * Add RewritePositionDeleteFilesSparkAction --------- Co-authored-by: Eduard Tudenhoefner <etudenhoefner@gmail.com> Co-authored-by: Fokko Driesprong <fokko@apache.org> Co-authored-by: Xianyang Liu <liu-xianyang@hotmail.com> Co-authored-by: Szehon Ho <szehon.apache@gmail.com> Co-authored-by: Yufei Gu <yufei_gu@apple.com> Co-authored-by: yufeigu <yufei@apache.org> Co-authored-by: Laith Alzyoud <laith.alzyoud@revolut.com> Co-authored-by: vaultah <4944562+vaultah@users.noreply.github.com>
This resolves #7851