refactor: remove redundant partitioned_by_file_group file scan field by Phoenix500526 · Pull Request #23189 · apache/datafusion

Phoenix500526 · 2026-06-25T15:25:46Z

Which issue does this PR close?

Closes Remove redundant partitioned_by_file_group file scan field #23099 .

Rationale for this change

FileScanConfig had two overlapping ways to declare a file scan's output
partitioning:

partitioned_by_file_group: bool — a shorthand meaning "the file groups are
organized by Hive partition column values, so the output is Hash-partitioned on
those columns", and
output_partitioning: Option<Partitioning> — a general, explicit declared
partitioning (added in Add ListingOptions::output_partitioning and FileScanConfig::output_partitioning for pre-defined file partitioning #22657).

The bool is just a lazy shorthand for one specific output_partitioning value
(Partitioning::Hash over the partition columns), and every place that consumed
it (output_partitioning(), repartitioned(), create_sibling_state()) already
checked output_partitioning.is_some() || partitioned_by_file_group. Keeping both
is redundant and the ListingTable builder ended up setting both. This PR makes
output_partitioning the single source of truth.

What changes are included in this PR?

Following the issue's first option ("Remove partitioned_by_file_group"):

Remove FileScanConfig::partitioned_by_file_group, the corresponding
FileScanConfigBuilder field, and the with_partitioned_by_file_group builder
method.
ListingTable::scan now derives the partition-column Partitioning::Hash
itself (once its file groups are finalized, so the partition count is correct)
and passes it through the existing with_output_partitioning. The previous
with_output_partitioning(declared) + with_partitioned_by_file_group(...)
double-set is collapsed into one branch.
hash_partitioning_from_partition_fields is made pub so ListingTable
(a separate crate) can reuse the derivation instead of duplicating the
column-index resolution.
proto already round-trips output_partitioning, so no behavior is lost: the
now-vestigial partitioned_by_file_group wire field is left unset on write and
ignored on read. The field is kept in the .proto definition for backward
compatibility.
output_partitioning() / create_sibling_state() / repartitioned() now key
solely off output_partitioning.

Are these changes tested?

Yes — by existing tests, updated to the new single-field model:

datafusion-datasource: test_output_partitioning_with_partition_columns,
test_output_partitioning_no_partition_columns,
test_declared_output_partitioning_projects_with_scan, and the file_stream
work-stealing test morsel_partitioned_by_file_group_keeps_files_local (which
verifies that a declared output partitioning keeps each stream's files local).
datafusion-proto: roundtrip_parquet_exec_output_partitioning (and the other
roundtrip_parquet_exec_* cases) cover the partitioning round-trip. The old
roundtrip_parquet_exec_partitioned_by_file_group test exercised the removed
API and is dropped, as its coverage is subsumed by the output_partitioning
round-trip test.

All of the above pass, along with cargo fmt --all --check and
cargo clippy --all-targets --all-features -- -D warnings for the affected
crates.

Are there any user-facing changes?

Yes — public API changes :

Removed: the public FileScanConfig::partitioned_by_file_group field and the
FileScanConfigBuilder::with_partitioned_by_file_group method. Callers should
set with_output_partitioning(Some(Partitioning::Hash(..))) instead (or use the
now-public hash_partitioning_from_partition_fields helper).
Added: hash_partitioning_from_partition_fields is now pub.

Query results, optimizer decisions (e.g. eliding RepartitionExec), and the
serialized (proto) wire format are unchanged. There is one display-only
change: EXPLAIN now renders output_partitioning=Hash(...) on DataSourceExec
for partition-grouped scans. The scan already produced that partitioning before
(it was derived lazily inside output_partitioning()); it is now stored on the
output_partitioning field and therefore shown. The
repartition_subset_satisfaction and preserve_file_partitioning slt expected
plans are updated accordingly.

cargo-semver-checks will flag the removals as breaking, which is expected for
this cleanup.

github-actions · 2026-06-25T15:32:07Z

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

     Cloning apache/main
    Building datafusion-catalog-listing v54.0.0 (current)
       Built [  45.756s] (current)
     Parsing datafusion-catalog-listing v54.0.0 (current)
      Parsed [   0.013s] (current)
    Building datafusion-catalog-listing v54.0.0 (baseline)
       Built [  44.335s] (baseline)
     Parsing datafusion-catalog-listing v54.0.0 (baseline)
      Parsed [   0.013s] (baseline)
    Checking datafusion-catalog-listing v54.0.0 -> v54.0.0 (no change; assume patch)
     Checked [   0.113s] 223 checks: 223 pass, 30 skip
     Summary no semver update required
    Finished [  91.594s] datafusion-catalog-listing
    Building datafusion-common v54.0.0 (current)
       Built [  32.882s] (current)
     Parsing datafusion-common v54.0.0 (current)
      Parsed [   0.065s] (current)
    Building datafusion-common v54.0.0 (baseline)
       Built [  32.988s] (baseline)
     Parsing datafusion-common v54.0.0 (baseline)
      Parsed [   0.065s] (baseline)
    Checking datafusion-common v54.0.0 -> v54.0.0 (no change; assume patch)
     Checked [   1.079s] 223 checks: 223 pass, 30 skip
     Summary no semver update required
    Finished [  68.525s] datafusion-common
    Building datafusion-datasource v54.0.0 (current)
       Built [  36.987s] (current)
     Parsing datafusion-datasource v54.0.0 (current)
      Parsed [   0.034s] (current)
    Building datafusion-datasource v54.0.0 (baseline)
       Built [  37.047s] (baseline)
     Parsing datafusion-datasource v54.0.0 (baseline)
      Parsed [   0.035s] (baseline)
    Checking datafusion-datasource v54.0.0 -> v54.0.0 (no change; assume patch)
     Checked [   0.377s] 223 checks: 221 pass, 2 fail, 0 warn, 30 skip

--- failure inherent_method_missing: pub method removed or renamed ---

Description:
A publicly-visible method or associated fn is no longer available under its prior name. It may have been renamed or removed entirely.
        ref: https://doc.rust-lang.org/cargo/reference/semver.html#item-remove
       impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.48.0/src/lints/inherent_method_missing.ron

Failed in:
  FileScanConfigBuilder::with_partitioned_by_file_group, previously in file /home/runner/work/datafusion/datafusion/target/semver-checks/git-apache_main/6446decf518f29a21c68c2302ea8df8144d02676/datafusion/datasource/src/file_scan_config/mod.rs:526

--- failure struct_pub_field_missing: pub struct's pub field removed or renamed ---

Description:
A publicly-visible struct has at least one public field that is no longer available under its prior name. It may have been renamed or removed entirely.
        ref: https://doc.rust-lang.org/cargo/reference/semver.html#item-remove
       impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.48.0/src/lints/struct_pub_field_missing.ron

Failed in:
  field partitioned_by_file_group of struct FileScanConfig, previously in file /home/runner/work/datafusion/datafusion/target/semver-checks/git-apache_main/6446decf518f29a21c68c2302ea8df8144d02676/datafusion/datasource/src/file_scan_config/mod.rs:217

     Summary semver requires new major version: 2 major and 0 minor checks failed
    Finished [  76.271s] datafusion-datasource
    Building datafusion-proto v54.0.0 (current)
       Built [  59.052s] (current)
     Parsing datafusion-proto v54.0.0 (current)
      Parsed [   0.019s] (current)
    Building datafusion-proto v54.0.0 (baseline)
       Built [  59.460s] (baseline)
     Parsing datafusion-proto v54.0.0 (baseline)
      Parsed [   0.021s] (baseline)
    Checking datafusion-proto v54.0.0 -> v54.0.0 (no change; assume patch)
     Checked [   0.372s] 223 checks: 223 pass, 30 skip
     Summary no semver update required
    Finished [ 120.235s] datafusion-proto
    Building datafusion-sqllogictest v54.0.0 (current)
       Built [ 186.838s] (current)
     Parsing datafusion-sqllogictest v54.0.0 (current)
      Parsed [   0.022s] (current)
    Building datafusion-sqllogictest v54.0.0 (baseline)
       Built [ 182.873s] (baseline)
     Parsing datafusion-sqllogictest v54.0.0 (baseline)
      Parsed [   0.024s] (baseline)
    Checking datafusion-sqllogictest v54.0.0 -> v54.0.0 (no change; assume patch)
     Checked [   0.115s] 223 checks: 223 pass, 30 skip
     Summary no semver update required
    Finished [ 373.356s] datafusion-sqllogictest

kosiew

@Phoenix500526
Thanks for the refactor here.
think there is one backward compatibility issue in the protobuf deserialization path that should be addressed before merging.

kosiew

@Phoenix500526
Thanks for the follow-up here. The legacy protobuf compatibility concern looks addressed now. One remaining release-facing item: since this intentionally removes public Rust APIs in favor of output_partitioning, please document the migration path in the version-specific upgrade guide.

kosiew

@Phoenix500526

Looks 👍 to me

alamb · 2026-07-07T12:10:25Z

FYI @gene-bordegaray

gene-bordegaray

Flushing these comments as I think this is pretty critical for the direciton partitining is heading.

We claim hash partitioning on this when in reality we are not. This is not a fault of this PR but something we should cnsider in how we want to handle this.

This will have correctness implications for things like turning on dynamic filters on for a preserved hash parttioning stemming from this.

Seems like the right direction may be another type of partitioning

gene-bordegaray · 2026-07-07T12:27:43Z

            Some(output_partitioning)
+        } else if partitioned_by_file_group {
+            // Files are grouped by partition column values: declare Hash
+            // partitioning on those columns so the optimizer can skip hash


I am realizing this is another type of partitioning we are misrepresenting. See issue: #23236 .

I think in this case we aren't actually hash partitioned. We are some other type of partitioning something like ValuePartitioned

The comment is updated.

The concern is not the comment itself but the actual behavior of this feature is lying to DF. This claims a hash partitioning but in reality we are not partitioned by the hash function at all. Let me know if you have questions 😄

gene-bordegaray · 2026-07-07T12:35:39Z

+/// `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 hash_partitioning_from_partition_fields(


yes, we aren't actually hash it seems

@alamb it seems we have some other type of partitioning we need to represent correctly 🤔

also cc: @gabotechs @adriangb @NGA-TRAN and @jayshrivastava

Thanks for the review. I've renamed it as output_partitioning_from_partition_fields.

alamb · 2026-07-07T14:11:02Z

I merged up to resolve a conflict

`FileScanConfig` had two overlapping ways to declare file scan output partitioning: the `partitioned_by_file_group` bool and `output_partitioning`. Collapse them onto `output_partitioning` as the single source of truth. - Remove the `partitioned_by_file_group` field, the builder field, and the `with_partitioned_by_file_group` builder method. - `ListingTable` now derives the partition-column `Partitioning::Hash` once its file groups are finalized and passes it via `with_output_partitioning`; `hash_partitioning_from_partition_fields` is made `pub` for this. - proto already round-trips `output_partitioning`, so the now-vestigial wire bool is left unset on write and ignored on read (the proto field is kept for backward compatibility). Closes apache#23099. Signed-off-by: Jiawei Zhao <Phoenix500526@163.com>

After collapsing `partitioned_by_file_group` onto `output_partitioning`, the declared Hash partitioning is now stored on the scan and therefore rendered by `DataSourceExec`'s Display. Update the affected sqllogictest expected plans accordingly. Behavior is unchanged; only the EXPLAIN text gains an `output_partitioning=Hash(...)` entry on partition-grouped scans. Signed-off-by: Jiawei Zhao <Phoenix500526@163.com>

Honor the old partitioned_by_file_group protobuf flag when output_partitioning is absent, so previously serialized plans still decode to hash output partitioning. Rename test-only helpers to declared output partitioning wording. Signed-off-by: Jiawei Zhao <Phoenix500526@163.com>

Add an upgrade guide note for the removal of partitioned_by_file_group and its builder API. Point users to output_partitioning and with_output_partitioning. Signed-off-by: Jiawei Zhao <Phoenix500526@163.com>

gene-bordegaray

This PR makes sense from an existing behavior standpoint bu raises concerns with what we should do gong forward 👍

gene-bordegaray · 2026-07-08T12:50:23Z


    Some(Partitioning::Hash(exprs, partition_count))


The point I am making is we need to discuss this.

We are claiming Partitioning::Hash on these while in reality we are not has partitioned we are actaully partitioned by the partition column values. This was the same situation which users faced when decalring Partitioning::Range as hash. This behavior was here before but we should discuss what this means going forward with this feature.

Should we:

Introduce another variant of the Partitioning enum like ValuePartitioned (this would be much easier now that Partitioning::Range has really started to lay the ground work for doing this

Does this justify Partitioning becoming a trait of some type where users can implmenet their own custome partitioning types

Some other option I am not seeing

I think ValuePartitioned is covered by range, right?

We are claiming Partitioning::Hash on these while in reality we are not has partitioned we are actaully partitioned by the partition column values. This was the same situation which users faced when decalring Partitioning::Range as hash.

+1. This is exactly the pain I'm experiencing. It leads to correctness issues ex. dynamic filtering breaks if you are not hash partitioned but claim to be hash partitioned.

Yes. We would have to take the column parttion values, sort them, and represent them as a range partitioning where each partition only contains the intended values.

Example:

partition 1: col = A partition 2: col = Z partition 3: col = M

Represented like:

Range Partitioning: split points: [A, M, Z] sort order: Asc

Thus:

partition 1: col <= A parttion 2: A < col <= M partition 3: M <= col (which contains Z)

I think we shoudl actually pursue this ^ rather than hash in this PR.

@jayshrivastava @gabotechs thoughts?

My first instinct would be to remove Partitioning::Hash in favor of (very rought draft):

pub enum Partitioning { ... #[deprecated] Hash(Vec<Expr>), Value { expressions: Vec<Expr>, // Same as Partitioning::Hash method: &'static str // The value partitioning method (e.g., "ahash(0,0,0,0)", "ahash(1,2,3,4)" ...) } }

Ya this is a great point. A question I have is would we be able to represent the partitioning scheme that is happening with this column value partitioning?

Maybe the method could be an enum with some variants like column baked in?

🤔 That's a good question. However, how would that work regarding partitioning compatibility?

for example, Partitioning::Value { .., method: Ahash0 } is co-partitioned with Partitioning::Value { .., method: Ahash0 }, but would Partitioning::Value { .., method: Column } be co-partitioned with other Partitioning::Value { .., method: Column }?

No we couldnt claim that just given {..., method: Column} unless we gave information about the values / ordering of the partitioned columns. There may be other ways to describe this so we can prove co-partitioning like a having the Column enum variant carry a vec of the values where index position represents the partition number

It seems to me like this thread is raising an excellent point (we could use a more precise partitioning in this case of hive partitioning)

I think main also chooses this Hash partitioning for HIVE tables

Thus I think we should merge this PR in and then file a ticket to discuss/ plan for potentially using Value partitioning for HIVE partitioned tables in the future.

Avoid naming the partitioned-file-group helper as hash-specific. This keeps the existing physical representation while making the API and docs less likely to imply literal hash partitioning. Signed-off-by: Jiawei Zhao <Phoenix500526@163.com>

alamb

Thank you @Phoenix500526 @gabotechs @gene-bordegaray and @kosiew -- let's merge this one in and keep iterating as a follow on

alamb · 2026-07-09T21:12:50Z


    Some(Partitioning::Hash(exprs, partition_count))


It seems to me like this thread is raising an excellent point (we could use a more precise partitioning in this case of hive partitioning)

I think main also chooses this Hash partitioning for HIVE tables

Thus I think we should merge this PR in and then file a ticket to discuss/ plan for potentially using Value partitioning for HIVE partitioned tables in the future.

alamb · 2026-07-09T21:13:53Z

        batch_size: conf.batch_size.map(|s| s as u64),
        projection_exprs,
-        partitioned_by_file_group: Some(conf.partitioned_by_file_group),
+        // Partition grouping is now encoded in `output_partitioning`; this legacy


FWIW we don't typically guartantee wire format compatibility: https://docs.rs/datafusion-proto/latest/datafusion_proto/#version-compatibility

Maybe we can remove the backward compatiblity shim in a follow on

github-actions Bot added catalog Related to the catalog crate proto Related to proto crate datasource Changes to the datasource crate labels Jun 25, 2026

github-actions Bot added the auto detected api change Auto detected API change label Jun 25, 2026

Phoenix500526 force-pushed the issue/23099 branch from e9ed249 to 30a0d3c Compare June 26, 2026 02:14

github-actions Bot added the sqllogictest SQL Logic Tests (.slt) label Jun 26, 2026

kosiew requested changes Jul 2, 2026

View reviewed changes

Comment thread datafusion/proto/src/physical_plan/from_proto.rs

Comment thread datafusion/proto/tests/cases/roundtrip_physical_plan.rs

Comment thread datafusion/datasource/src/file_stream/mod.rs Outdated

Phoenix500526 force-pushed the issue/23099 branch from 30a0d3c to d1f5ed7 Compare July 3, 2026 02:07

kosiew added the api change Changes the API exposed to users of the crate label Jul 4, 2026

kosiew requested changes Jul 4, 2026

View reviewed changes

Comment thread datafusion/datasource/src/file_scan_config/mod.rs

Comment thread datafusion/datasource/src/file_scan_config/mod.rs

github-actions Bot added the documentation Improvements or additions to documentation label Jul 4, 2026

kosiew approved these changes Jul 7, 2026

View reviewed changes

gene-bordegaray reviewed Jul 7, 2026

View reviewed changes

Phoenix500526 added 4 commits July 8, 2026 20:35

docs: document file scan partitioning migration

f6b19e1

Add an upgrade guide note for the removal of partitioned_by_file_group and its builder API. Point users to output_partitioning and with_output_partitioning. Signed-off-by: Jiawei Zhao <Phoenix500526@163.com>

Phoenix500526 force-pushed the issue/23099 branch from f5bb5fb to 2f4967b Compare July 8, 2026 12:38

github-actions Bot added the common Related to common crate label Jul 8, 2026

gene-bordegaray reviewed Jul 8, 2026

View reviewed changes

refactor: clarify scan partitioning helper

9d6db0d

Avoid naming the partitioned-file-group helper as hash-specific. This keeps the existing physical representation while making the API and docs less likely to imply literal hash partitioning. Signed-off-by: Jiawei Zhao <Phoenix500526@163.com>

Phoenix500526 force-pushed the issue/23099 branch from 2f4967b to 9d6db0d Compare July 8, 2026 12:54

alamb approved these changes Jul 9, 2026

View reviewed changes

alamb added this pull request to the merge queue Jul 9, 2026

Merged via the queue into apache:main with commit 66b059e Jul 9, 2026
37 checks passed

Uh oh!

Conversation

Phoenix500526 commented Jun 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Uh oh!

github-actions Bot commented Jun 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kosiew left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kosiew left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

kosiew left a comment

Choose a reason for hiding this comment

Uh oh!

alamb commented Jul 7, 2026

Uh oh!

gene-bordegaray left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gene-bordegaray Jul 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alamb commented Jul 7, 2026

Uh oh!

gene-bordegaray left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gene-bordegaray Jul 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gabotechs Jul 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alamb left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Phoenix500526 commented Jun 25, 2026 •

edited

Loading

github-actions Bot commented Jun 25, 2026 •

edited

Loading

kosiew left a comment •

edited

Loading

gene-bordegaray Jul 7, 2026 •

edited

Loading

gene-bordegaray Jul 8, 2026 •

edited

Loading

gabotechs Jul 9, 2026 •

edited

Loading