Skip to content

Conversation

@scovich
Copy link
Collaborator

@scovich scovich commented Feb 21, 2025

What changes are proposed in this pull request?

Add basic support for partition pruning by combining two pieces of existing infra:

  1. The log replay row visitor already needs to parse partition values and already filters out unwanted rows
  2. The default predicate evaluator works directly with scalars

Result: partition pruning gets applied during log replay, just before deduplication so we don't have to remember pruned files.

WARNING: The implementation currently has a flaw, in case the history contains a table-replace that affected partition columns. For example, changing a value column into a non-nullable partition column, or an incompatible type change to a partition column. In such cases, the remove actions generated by the table-replace operation (for old files) would have the wrong type or even be entirely absent. While the code can handle an absent partition value, an incompatibly typed value would cause a parsing error that fails the whole query. Note that stats-based data skipping already has the same flaw, so we are not making the problem worse. We will fix the problem for both as a follow-up item, tracked by #712

NOTE: While this is a convenient way to achieve partition pruning in the immediate term, Delta checkpoints can provide strongly-typed stats_parsed and partitionValues_parsed columns which would have a completely different access.

  • For stats vs. stats_parsed, the likely solution is simple enough because we already json-parse stats into a strongly-typed nested struct in order to evaluate the data skipping predicate over its record batch. We just avoid the parsing overhead if stats_parsed is already available.
  • The partitionValues field poses a bigger challenge, because it's a string-string map, not a JSON literal. In order to turn it into a strongly-typed nested struct, we would need a SQL expression that can extract the string values and try-cast them to the desired types. That's ugly enough we might prefer to keep completely different code paths for parsed vs. string partition values, but then there's a risk that partition pruning behavior changes depending on which path got invoked.

How was this change tested?

New unit tests, and adjusted one unit test that assumed no partition pruning.

@scovich scovich added the merge hold Don't allow the PR to merge label Feb 21, 2025
@scovich scovich requested review from hntd187 and nicklan February 21, 2025 22:53
@codecov
Copy link

codecov bot commented Feb 21, 2025

Codecov Report

Attention: Patch coverage is 86.36364% with 9 lines in your changes missing coverage. Please review.

Project coverage is 84.29%. Comparing base (cd5ea20) to head (30baa42).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
kernel/src/scan/log_replay.rs 86.15% 6 Missing and 3 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #713      +/-   ##
==========================================
+ Coverage   84.25%   84.29%   +0.04%     
==========================================
  Files          77       77              
  Lines       19051    19099      +48     
  Branches    19051    19099      +48     
==========================================
+ Hits        16051    16100      +49     
+ Misses       2202     2201       -1     
  Partials      798      798              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@github-actions github-actions bot added the breaking-change Change that require a major version bump label Feb 21, 2025
Copy link
Member

@nicklan nicklan left a comment

Choose a reason for hiding this comment

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

Sorry for the loooong delay on reviewing this!

Looks great. Nice how simple it was actually to fit it in once you found the right shape.

Copy link
Collaborator

@zachschuermann zachschuermann left a comment

Choose a reason for hiding this comment

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

Looks great thanks!! just a couple quick things.

And for the TODOs in the description: it sounds like the first is sufficiently covered by #712. should we make an issue or two for the stats_parsed and partitionValues_parsed additions?

@scovich scovich removed the merge hold Don't allow the PR to merge label Mar 12, 2025
@scovich
Copy link
Collaborator Author

scovich commented Mar 12, 2025

After offline discussion, we will deal with the schema change issue as a follow-up because it already existed with stats parsing and nobody seems to have hit the corner case yet.

@scovich scovich merged commit db86d97 into delta-io:main Mar 12, 2025
21 checks passed
@OussamaSaoudi OussamaSaoudi mentioned this pull request Jul 16, 2025
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

breaking-change Change that require a major version bump

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants