-
Notifications
You must be signed in to change notification settings - Fork 5.4k
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
[Iceberg] Refine the partition specs that really need to be checked #22753
[Iceberg] Refine the partition specs that really need to be checked #22753
Conversation
Thanks for the release note entry! A nit rephrasing suggestion to consider:
|
@steveburnett Thanks for the suggestion, fixed! |
A final nit: I'm sorry but I didn't think about the Order of changes recommendations in the Release Notes Guidelines when I commented. Here's another revision suggestion, apologies for the churn:
|
@steveburnett So grateful for your help in standardizing the release notes, fixed! I also need to learn the Guidelines :-). |
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.
I think the main change is fine. I do think we should probably add tests for the different table format versions+modes though to verify the expected behavior.
presto-iceberg/src/test/java/com/facebook/presto/iceberg/IcebergDistributedSmokeTestBase.java
Show resolved
Hide resolved
presto-iceberg/src/test/java/com/facebook/presto/iceberg/TestIcebergLogicalPlanner.java
Show resolved
Hide resolved
presto-iceberg/src/test/java/com/facebook/presto/iceberg/TestIcebergLogicalPlanner.java
Show resolved
Hide resolved
5caced9
to
67e089f
Compare
@ZacBlanco, to add test case for V2 MOR table, I found that previously we didn't take delete files into consider when executing metadata-deletion/truncate. So I added a commit at the front to fix the behavior of metadata deletion and table truncate, then add the test cases we mentioned above for V2 MOR table. Please take a look when convenient, thanks! |
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.
Two questions regarding the new change
presto-iceberg/src/main/java/com/facebook/presto/iceberg/IcebergAbstractMetadata.java
Show resolved
Hide resolved
presto-iceberg/src/main/java/com/facebook/presto/iceberg/IcebergAbstractMetadata.java
Show resolved
Hide resolved
67e089f
to
d5a1a72
Compare
{ | ||
String tableName = "test_empty_partition_spec_table"; | ||
try { | ||
// Create a table with on partition |
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.
// Create a table with on partition | |
// Create a table with no partition |
d5a1a72
to
f1c00e9
Compare
f1c00e9
to
1f27117
Compare
1f27117
to
fa50c51
Compare
Hi @tdcmeehan, I just special handled some test cases in |
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.
Thank you @hantangwangd
Description
Iceberg table may contains empty partition specs with data all be deleted or rewritten to another partition spec, so that it may contains some manifest files with spec id of these partition specs just to record the information about data files deletion. If we don't filter them out during judgement, we might in some cases prohibit metadata deletion or predicate thoroughly pushdown, even if it's feasible.
This PR refine the partition specs that really need to be checked in an Iceberg table when determining whether supports metadata deletion or predicate thoroughly pushdown.
Test Plan
Contributor checklist
Release Notes