Skip to content

Conversation

@szehon-ho
Copy link
Member

This makes some minor cleanup and add one more test for #2925

This is from comment #4293 (comment) of splitting these changes from that pr into a separate one.

@github-actions github-actions bot added the spark label Mar 10, 2022
@szehon-ho szehon-ho changed the title Cleanup for Spark DataFrame.overwritePartitions isolation level Spark: Cleanup for Spark DataFrame.overwritePartitions isolation level Mar 10, 2022
@szehon-ho
Copy link
Member Author

@aokolnychyi fyi

Copy link
Contributor

@kbendick kbendick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me. Thanks for following up @szehon-ho!

if (validateFromSnapshotId != null) {
dynamicOverwrite.validateFromSnapshot(validateFromSnapshotId);
}
dynamicOverwrite.validateNoConflictingData();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: It seems like this one is repeated in both cases. Since there's no existing else clause, it's probably not worth reducing the duplication as we might need more if statements but thought I'd call it out.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks yea good catch, I was thinking to leave it here to make it more clear in each validation mode what we are validating, and maybe there are future modes where we do not do this as you said. As opposed to the validationSnapshotId which is the basis of all validations.

@szehon-ho szehon-ho changed the title Spark: Cleanup for Spark DataFrame.overwritePartitions isolation level Spark: Cleanup Spark DataFrame.overwritePartitions isolation level handling Mar 14, 2022
@szehon-ho szehon-ho changed the title Spark: Cleanup Spark DataFrame.overwritePartitions isolation level handling Spark 3.2: Cleanup Spark DataFrame.overwritePartitions isolation level handling Mar 14, 2022
@szehon-ho szehon-ho changed the title Spark 3.2: Cleanup Spark DataFrame.overwritePartitions isolation level handling Spark 3.2: Cleanup DataFrame overwritePartitions isolation level handling Mar 14, 2022
@szehon-ho szehon-ho merged commit 12ec66c into apache:master Mar 14, 2022
@szehon-ho
Copy link
Member Author

Thanks @kbendick for the review!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants