Repository navigation
fix: declare Hive partitioned scans as Range - #25279
gstamatakis95 wants to merge 5 commits into
Conversation
github-actions
Bot
added
sqllogictest
gstamatakis95
changed the title
|
Thanks for opening this! I'll try to review soon. One quick reference without having reviewed (sorry!): it's possible that there is some code reuse possible across the various TableProviders: see #22395 (comment) |
| /// Builds output partitioning over `partition_cols` (resolved to their indices in | ||
| /// `schema`) with `partition_count` partitions. Returns `None` when there are no | ||
| /// partition columns. Callers use this to declare the output partitioning of a scan | ||
| /// whose file groups are organized by partition column values. | ||
| pub fn output_partitioning_from_partition_fields( | ||
| schema: &Schema, | ||
| partition_cols: &Fields, | ||
| partition_count: usize, | ||
| ) -> Option<Partitioning> { |
There was a problem hiding this comment.
This still returns Partioning::Hash by default, which means external consumers might still get the old behavior.
Should it be deprecated and replaced with a new function/method like?:
pub fn output_range_partitioning_from_split_points(
schema: &Schema,
partition_cols: &Fields,
split_points: Vec<SplitPoint>,
) -> Result<Option<Partitioning>>;...which could then be used in ListingTable::scan as well?
There was a problem hiding this comment.
Deprecated it in favor of output_range_partitioning_from_split_points with the signature you suggested and ListingTable::scan now uses it.
| let ordering = table_partition_cols | ||
| .iter() | ||
| .map(|field| { | ||
| Expr::Column(Column::from_name(field.name())).sort(true, true) |
There was a problem hiding this comment.
If these bools are meant to correspond to SortOptions::default(), maybe they should be derived from it instead.
There was a problem hiding this comment.
The ordering now comes from PhysicalSortExpr::new_default, which is SortOptions::default(), the same default used to cut the groups.
|
Thank you for opening this pull request! Reviewer note: cargo-semver-checks reported the current version number is not SemVer-compatible with the changes in this pull request (compared against the base branch). Details |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #25279 +/- ##
==========================================
- Coverage 82.87% 82.87% -0.01%
==========================================
Files 1153 1153
Lines 457043 457138 +95
Branches 457043 457138 +95
==========================================
+ Hits 378777 378855 +78
- Misses 55268 55278 +10
- Partials 22998 23005 +7 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
…e-partitioning # Conflicts: # datafusion/sqllogictest/test_files/preserve_file_partitioning.slt # datafusion/sqllogictest/test_files/repartition_subset_satisfaction.slt
RangePartitioning::new was deprecated on main in 56.0.0, so the two new round trip tests failed clippy with -D warnings after merging main. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
alamb
left a comment
alamb
left a comment
There was a problem hiding this comment.
This looks good to me -- thank you @gstamatakis95 and @stuhood
I took the liberty of merging up from main and resolving a conflict
@gene-bordegaray or @Phoenix500526 I think you wrote most of the code changed in this PR (e.g. #19124 , #22207, and #22777) -- can you also take a look?
OK，@alamb I'll find time to take a look at this tomorrow. |
Phoenix500526
left a comment
Phoenix500526
left a comment
There was a problem hiding this comment.
Thanks for the fix! This approach looks good to me. Could you update the PR description to match the implementation? It currently says there is no new public helper and that the dynamic-filter guard was removed, but the code adds output_range_partitioning_from_split_points and retains the guard.
| /// Builds `Partitioning::Hash` over `partition_cols` (resolved to their indices in | ||
| /// `schema`). Returns `None` when there are no partition columns. | ||
| #[deprecated( | ||
| since = "56.0.0", | ||
| note = "Use `output_range_partitioning_from_split_points` instead" | ||
| )] |
There was a problem hiding this comment.
Should we add a short migration example to the 56.0.0 upgrade guide covering the new ListFilesResult field and the replacement for the deprecated Hash helper? It would help clarify how to derive split points from the file groups.
…#781) This change 1. Updates the only test we have on pre-partitioned data to use `Range` partitioning instead of `Hash` - Previously, the tests in this file used to declare the data source as `Hash` partitioned, which is incorrect because data is not partitioned by DataFusion's hash function. - Upstream, the same behavior is being merged as well. apache/datafusion#25279 migrates the `preserve_file_partitioning` config option to apply `Range` to a data source instead of `Hash` because declaring a data source has `Hash` is wrong - Now, we use `with_output_partitioning` to just declare a `Range`-partitioned data source Informs #628
5 participants
Which issue does this PR close?
Rationale for this change
What changes are included in this PR?
What is the testing strategy for this PR?
Are there any user-facing changes?