-
Notifications
You must be signed in to change notification settings - Fork 2.9k
Spark 3.4: Backport Spark actions changes in Spark rewrite_table_path procedure (#12006 #12172 #11929 #12282 #12569) #12568
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
@szehon-ho Could you please help review this PR? Thank you very much! We are currently dealing with a large-scale migration from Hive tables to Iceberg tables in our internal environment, and this feature has been very helpful to us. During the code cherry-pick process, I noticed some differences between the code of Spark 3.5 and Spark 3.4. Therefore, I cherry-picked the latest changes from Spark 3.5 into the Spark 3.4. |
| .isEqualTo(manifestFileCount); | ||
| assertThat(filesToMove.size()).withFailMessage("Wrong total file count").isEqualTo(totalCount); | ||
| assertThat(filesToMove.stream().filter(f -> f.endsWith(".stats")).count()) | ||
| .withFailMessage("Wrong rebuilt Statistic file count") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you please update all of the places in this test class (and in Spark 3.5's version of it) that use withFailMessage() to as()? Using withFailMessage() is typically not what's desired as it will only show the message but not the values of expected/actual, thus making it more difficult to debug when tests fail
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in fact I went ahead and created #12569, so you might want to wait until this is in and port that over as part of this PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nastra Thank you for reviewing and providing valuable feedback! I can wait until your PR is merged, and then further improve this PR based on your changes.
|
should we also backport #12282? |
@szehon-ho Thank you for helping review the code! I will backport #12282. |
d41482c to
c2f00b4
Compare
c2f00b4 to
643ce48
Compare
|
@nastra @szehon-ho Could you please help review this PR again? Thank you very much! The latest commit includes #12282 and #12569. |
|
@nastra Thank you very much for reviewing this pr! |
backport #12006 #12172 #11929 #12282 #12569