You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "limadog9 (via GitHub)" <gi...@apache.org> on 2026/09/30 02:53:39 UTC
[PR] fix: preserve functional dependencies across unnest [datafusion]
limadog9 opened a new pull request, #25892:
URL: https://github.com/apache/datafusion/pull/25892
## Which issue does this PR close?
- Closes #25844
## Rationale for this change
A list UNNEST can produce several rows for one input key. Retaining the input's functional dependencies makes a later DISTINCT or GROUP BY discard unnested values, and can make an ordered window use the wrong frame. These queries return incorrect results.
## What changes are included in this PR?
- Remap inherited dependency indices to output positions after struct expansion.
- Downgrade dependencies after list unnest, remove unnested list outputs from dependency targets, and discard dependencies whose determinants are unnested.
- Add SQL and plan-level regressions for distinct, grouping, window frames, and shifted indices.
## What is the testing strategy for this PR?
- cargo fmt --all --check
- cargo test -p datafusion-expr --lib unnest_remaps_functional_dependencies
- cargo test --profile ci --test sqllogictests -- functional_dependencies.slt
## Are there any user-facing changes?
Yes. A list unnest followed by DISTINCT, GROUP BY, or an ordered window now returns the expected result.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: preserve functional dependencies across unnest [datafusion]
Posted by "limadog9 (via GitHub)" <gi...@apache.org>.
limadog9 commented on PR #25892:
URL: https://github.com/apache/datafusion/pull/25892#issuecomment-6251039710
Thanks for the review, @jayzhan211! Removed the redundant check and squashed all changes into a62969d. Also fixed the null_equality build failure against current main.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: preserve functional dependencies across unnest [datafusion]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #25892:
URL: https://github.com/apache/datafusion/pull/25892#issuecomment-6248552362
<!-- semver-check-comment -->
Thank you for opening this pull request!
Reviewer note: [cargo-semver-checks](https://github.com/obi1kenobi/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>
<summary>Details</summary>
```
Cloning apache/main
Building datafusion-expr v55.1.0 (current)
Built [ 34.104s] (current)
Parsing datafusion-expr v55.1.0 (current)
Parsed [ 0.082s] (current)
Building datafusion-expr v55.1.0 (baseline)
Built [ 33.652s] (baseline)
Parsing datafusion-expr v55.1.0 (baseline)
Parsed [ 0.088s] (baseline)
Checking datafusion-expr v55.1.0 -> v55.1.0 (no change; assume patch)
Checked [ 1.498s] 223 checks: 216 pass, 7 fail, 0 warn, 31 skip
--- failure function_missing: pub fn removed or renamed ---
Description:
A publicly-visible function cannot be imported by its prior path. A `pub use` may have been removed, or the function itself 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.50.0/src/lints/function_missing.ron
Failed in:
function datafusion_expr::expr::in_subquery_tuple_values, previously in file /home/runner/work/datafusion/datafusion/target/semver-checks/git-apache_main/4e7bc058e40aebf60b0e0d900f41dd50e685fc3b/datafusion/expr/src/expr.rs:1417
function datafusion_expr::logical_plan::builder::table_scan_with_filter_and_fetch_and_skip, previously in file /home/runner/work/datafusion/datafusion/target/semver-checks/git-apache_main/4e7bc058e40aebf60b0e0d900f41dd50e685fc3b/datafusion/expr/src/logical_plan/builder.rs:2249
function datafusion_expr::builder::table_scan_with_filter_and_fetch_and_skip, previously in file /home/runner/work/datafusion/datafusion/target/semver-checks/git-apache_main/4e7bc058e40aebf60b0e0d900f41dd50e685fc3b/datafusion/expr/src/logical_plan/builder.rs:2249
function datafusion_expr::utils::passthrough_field_index, previously in file /home/runner/work/datafusion/datafusion/target/semver-checks/git-apache_main/4e7bc058e40aebf60b0e0d900f41dd50e685fc3b/datafusion/expr/src/utils.rs:1033
function datafusion_expr::expr_schema::cast_subquery_columns, previously in file /home/runner/work/datafusion/datafusion/target/semver-checks/git-apache_main/4e7bc058e40aebf60b0e0d900f41dd50e685fc3b/datafusion/expr/src/expr_schema.rs:847
--- 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.50.0/src/lints/inherent_method_missing.ron
Failed in:
LogicalPlanBuilder::table_scan, previously in file /home/runner/work/datafusion/datafusion/target/semver-checks/git-apache_main/4e7bc058e40aebf60b0e0d900f41dd50e685fc3b/datafusion/expr/src/logical_plan/builder.rs:523
LogicalPlanBuilder::table_scan, previously in file /home/runner/work/datafusion/datafusion/target/semver-checks/git-apache_main/4e7bc058e40aebf60b0e0d900f41dd50e685fc3b/datafusion/expr/src/logical_plan/builder.rs:523
LogicalPlanBuilder::table_scan, previously in file /home/runner/work/datafusion/datafusion/target/semver-checks/git-apache_main/4e7bc058e40aebf60b0e0d900f41dd50e685fc3b/datafusion/expr/src/logical_plan/builder.rs:523
LogicalPlanBuilder::table_scan, previously in file /home/runner/work/datafusion/datafusion/target/semver-checks/git-apache_main/4e7bc058e40aebf60b0e0d900f41dd50e685fc3b/datafusion/expr/src/logical_plan/builder.rs:523
TableScanBuilder::with_skip, previously in file /home/runner/work/datafusion/datafusion/target/semver-checks/git-apache_main/4e7bc058e40aebf60b0e0d900f41dd50e685fc3b/datafusion/expr/src/logical_plan/plan.rs:3331
TableScanBuilder::with_skip, previously in file /home/runner/work/datafusion/datafusion/target/semver-checks/git-apache_main/4e7bc058e40aebf60b0e0d900f41dd50e685fc3b/datafusion/expr/src/logical_plan/plan.rs:3331
Join::with_null_aware_value_keys, previously in file /home/runner/work/datafusion/datafusion/target/semver-checks/git-apache_main/4e7bc058e40aebf60b0e0d900f41dd50e685fc3b/datafusion/expr/src/logical_plan/plan.rs:4751
Join::with_null_aware_value_keys, previously in file /home/runner/work/datafusion/datafusion/target/semver-checks/git-apache_main/4e7bc058e40aebf60b0e0d900f41dd50e685fc3b/datafusion/expr/src/logical_plan/plan.rs:4751
CreateExternalCatalog::builder, previously in file /home/runner/work/datafusion/datafusion/target/semver-checks/git-apache_main/4e7bc058e40aebf60b0e0d900f41dd50e685fc3b/datafusion/expr/src/logical_plan/ddl.rs:630
CreateExternalCatalog::builder, previously in file /home/runner/work/datafusion/datafusion/target/semver-checks/git-apache_main/4e7bc058e40aebf60b0e0d900f41dd50e685fc3b/datafusion/expr/src/logical_plan/ddl.rs:630
DropCatalog::builder, previously in file /home/runner/work/datafusion/datafusion/target/semver-checks/git-apache_main/4e7bc058e40aebf60b0e0d900f41dd50e685fc3b/datafusion/expr/src/logical_plan/ddl.rs:843
DropCatalog::builder, previously in file /home/runner/work/datafusion/datafusion/target/semver-checks/git-apache_main/4e7bc058e40aebf60b0e0d900f41dd50e685fc3b/datafusion/expr/src/logical_plan/ddl.rs:843
--- failure module_missing: pub module removed or renamed ---
Description:
A publicly-visible module cannot be imported by its prior path. A `pub use` may have been removed, or the module may have been renamed, removed, or made non-public.
ref: https://doc.rust-lang.org/cargo/reference/semver.html#item-remove
impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.50.0/src/lints/module_missing.ron
Failed in:
mod datafusion_expr::logical_plan::materialized_cte, previously in file /home/runner/work/datafusion/datafusion/target/semver-checks/git-apache_main/4e7bc058e40aebf60b0e0d900f41dd50e685fc3b/datafusion/expr/src/logical_plan/materialized_cte.rs:18
mod datafusion_expr::materialized_cte, previously in file /home/runner/work/datafusion/datafusion/target/semver-checks/git-apache_main/4e7bc058e40aebf60b0e0d900f41dd50e685fc3b/datafusion/expr/src/logical_plan/materialized_cte.rs:18
--- failure struct_missing: pub struct removed or renamed ---
Description:
A publicly-visible struct cannot be imported by its prior path. A `pub use` may have been removed, or the struct itself 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.50.0/src/lints/struct_missing.ron
Failed in:
struct datafusion_expr::logical_plan::materialized_cte::MaterializedCteScan, previously in file /home/runner/work/datafusion/datafusion/target/semver-checks/git-apache_main/4e7bc058e40aebf60b0e0d900f41dd50e685fc3b/datafusion/expr/src/logical_plan/materialized_cte.rs:144
struct datafusion_expr::materialized_cte::MaterializedCteScan, previously in file /home/runner/work/datafusion/datafusion/target/semver-checks/git-apache_main/4e7bc058e40aebf60b0e0d900f41dd50e685fc3b/datafusion/expr/src/logical_plan/materialized_cte.rs:144
struct datafusion_expr::logical_plan::MaterializedCteScan, previously in file /home/runner/work/datafusion/datafusion/target/semver-checks/git-apache_main/4e7bc058e40aebf60b0e0d900f41dd50e685fc3b/datafusion/expr/src/logical_plan/materialized_cte.rs:144
struct datafusion_expr::MaterializedCteScan, previously in file /home/runner/work/datafusion/datafusion/target/semver-checks/git-apache_main/4e7bc058e40aebf60b0e0d900f41dd50e685fc3b/datafusion/expr/src/logical_plan/materialized_cte.rs:144
struct datafusion_expr::logical_plan::materialized_cte::MaterializedCte, previously in file /home/runner/work/datafusion/datafusion/target/semver-checks/git-apache_main/4e7bc058e40aebf60b0e0d900f41dd50e685fc3b/datafusion/expr/src/logical_plan/materialized_cte.rs:69
struct datafusion_expr::materialized_cte::MaterializedCte, previously in file /home/runner/work/datafusion/datafusion/target/semver-checks/git-apache_main/4e7bc058e40aebf60b0e0d900f41dd50e685fc3b/datafusion/expr/src/logical_plan/materialized_cte.rs:69
struct datafusion_expr::logical_plan::MaterializedCte, previously in file /home/runner/work/datafusion/datafusion/target/semver-checks/git-apache_main/4e7bc058e40aebf60b0e0d900f41dd50e685fc3b/datafusion/expr/src/logical_plan/materialized_cte.rs:69
struct datafusion_expr::MaterializedCte, previously in file /home/runner/work/datafusion/datafusion/target/semver-checks/git-apache_main/4e7bc058e40aebf60b0e0d900f41dd50e685fc3b/datafusion/expr/src/logical_plan/materialized_cte.rs:69
struct datafusion_expr::logical_plan::materialized_cte::MaterializedCteId, previously in file /home/runner/work/datafusion/datafusion/target/semver-checks/git-apache_main/4e7bc058e40aebf60b0e0d900f41dd50e685fc3b/datafusion/expr/src/logical_plan/materialized_cte.rs:44
struct datafusion_expr::materialized_cte::MaterializedCteId, previously in file /home/runner/work/datafusion/datafusion/target/semver-checks/git-apache_main/4e7bc058e40aebf60b0e0d900f41dd50e685fc3b/datafusion/expr/src/logical_plan/materialized_cte.rs:44
struct datafusion_expr::logical_plan::MaterializedCteId, previously in file /home/runner/work/datafusion/datafusion/target/semver-checks/git-apache_main/4e7bc058e40aebf60b0e0d900f41dd50e685fc3b/datafusion/expr/src/logical_plan/materialized_cte.rs:44
struct datafusion_expr::MaterializedCteId, previously in file /home/runner/work/datafusion/datafusion/target/semver-checks/git-apache_main/4e7bc058e40aebf60b0e0d900f41dd50e685fc3b/datafusion/expr/src/logical_plan/materialized_cte.rs:44
--- 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.50.0/src/lints/struct_pub_field_missing.ron
Failed in:
field skip of struct TableScan, previously in file /home/runner/work/datafusion/datafusion/target/semver-checks/git-apache_main/4e7bc058e40aebf60b0e0d900f41dd50e685fc3b/datafusion/expr/src/logical_plan/plan.rs:3175
field skip of struct TableScan, previously in file /home/runner/work/datafusion/datafusion/target/semver-checks/git-apache_main/4e7bc058e40aebf60b0e0d900f41dd50e685fc3b/datafusion/expr/src/logical_plan/plan.rs:3175
field null_aware_value_keys of struct Join, previously in file /home/runner/work/datafusion/datafusion/target/semver-checks/git-apache_main/4e7bc058e40aebf60b0e0d900f41dd50e685fc3b/datafusion/expr/src/logical_plan/plan.rs:4517
field null_aware_value_keys of struct Join, previously in file /home/runner/work/datafusion/datafusion/target/semver-checks/git-apache_main/4e7bc058e40aebf60b0e0d900f41dd50e685fc3b/datafusion/expr/src/logical_plan/plan.rs:4517
--- failure trait_method_missing: pub trait method removed or renamed ---
Description:
A trait method is no longer callable, and may have been renamed or removed entirely.
ref: https://doc.rust-lang.org/cargo/reference/semver.html#trait-item-signature
impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.50.0/src/lints/trait_method_missing.ron
Failed in:
method supports_skip_pushdown of trait TableSource, previously in file /home/runner/work/datafusion/datafusion/target/semver-checks/git-apache_main/4e7bc058e40aebf60b0e0d900f41dd50e685fc3b/datafusion/expr/src/table_source.rs:122
--- failure trait_method_parameter_count_changed: pub trait method parameter count changed ---
Description:
A trait method now takes a different number of parameters.
ref: https://doc.rust-lang.org/cargo/reference/semver.html#trait-item-signature
impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.50.0/src/lints/trait_method_parameter_count_changed.ron
Failed in:
ExprPlanner::plan_scalar now takes 1 instead of 2 parameters, in file /home/runner/work/datafusion/datafusion/datafusion/expr/src/planner.rs:269
Summary semver requires new major version: 7 major and 0 minor checks failed
Finished [ 70.971s] datafusion-expr
Building datafusion-sqllogictest v55.1.0 (current)
Built [ 110.077s] (current)
Parsing datafusion-sqllogictest v55.1.0 (current)
Parsed [ 0.024s] (current)
Building datafusion-sqllogictest v55.1.0 (baseline)
Built [ 106.535s] (baseline)
Parsing datafusion-sqllogictest v55.1.0 (baseline)
Parsed [ 0.025s] (baseline)
Checking datafusion-sqllogictest v55.1.0 -> v55.1.0 (no change; assume patch)
Checked [ 0.088s] 223 checks: 222 pass, 1 fail, 0 warn, 31 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.50.0/src/lints/inherent_method_missing.ron
Failed in:
DataFusionSubstraitRoundTrip::with_optimization, previously in file /home/runner/work/datafusion/datafusion/target/semver-checks/git-apache_main/4e7bc058e40aebf60b0e0d900f41dd50e685fc3b/datafusion/sqllogictest/src/engines/datafusion_substrait_roundtrip_engine/runner.rs:60
Summary semver requires new major version: 1 major and 0 minor checks failed
Finished [ 219.814s] datafusion-sqllogictest
```
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: preserve functional dependencies across unnest [datafusion]
Posted by "kumarUjjawal (via GitHub)" <gi...@apache.org>.
kumarUjjawal commented on PR #25892:
URL: https://github.com/apache/datafusion/pull/25892#issuecomment-5907841055
cc @adriangb @Dandandan
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: preserve functional dependencies across unnest [datafusion]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #25892:
URL: https://github.com/apache/datafusion/pull/25892#issuecomment-5908329882
## [Codecov](https://app.codecov.io/gh/apache/datafusion/pull/25892?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `92.85714%` with `6 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 82.58%. Comparing base ([`1d9be2e`](https://app.codecov.io/gh/apache/datafusion/commit/1d9be2e10794994fc1495c35363658dc7db0adf7?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`4436b32`](https://app.codecov.io/gh/apache/datafusion/commit/4436b3250b2830d0a337a7f8cee3bf4f5c5ea44d?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 4 commits behind head on main.
| [Files with missing lines](https://app.codecov.io/gh/apache/datafusion/pull/25892?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [datafusion/expr/src/logical\_plan/plan.rs](https://app.codecov.io/gh/apache/datafusion/pull/25892?src=pr&el=tree&filepath=datafusion%2Fexpr%2Fsrc%2Flogical_plan%2Fplan.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-ZGF0YWZ1c2lvbi9leHByL3NyYy9sb2dpY2FsX3BsYW4vcGxhbi5ycw==) | 92.85% | [0 Missing and 6 partials :warning: ](https://app.codecov.io/gh/apache/datafusion/pull/25892?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## main #25892 +/- ##
=======================================
Coverage 82.58% 82.58%
=======================================
Files 1144 1144
Lines 441510 441593 +83
Branches 441510 441593 +83
=======================================
+ Hits 364615 364693 +78
+ Misses 54844 54843 -1
- Partials 22051 22057 +6
```
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/datafusion/pull/25892?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: preserve functional dependencies across unnest [datafusion]
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 commented on PR #25892:
URL: https://github.com/apache/datafusion/pull/25892#issuecomment-6268814175
Thanks @limadog9 !
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: preserve functional dependencies across unnest [datafusion]
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 merged PR #25892:
URL: https://github.com/apache/datafusion/pull/25892
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: preserve functional dependencies across unnest [datafusion]
Posted by "limadog9 (via GitHub)" <gi...@apache.org>.
limadog9 commented on PR #25892:
URL: https://github.com/apache/datafusion/pull/25892#issuecomment-6182945364
Hi @adriangb @Dandandan, following up on the earlier review ping for this fix for #25844. It corrects functional dependency propagation through UNNEST, with SQL and plan-level regressions for DISTINCT, GROUP BY, and ordered windows.
Could you review when you have a chance? The latest Actions runs are still `action_required`; could a maintainer approve CI as well? Thanks!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: preserve functional dependencies across unnest [datafusion]
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 commented on code in PR #25892:
URL: https://github.com/apache/datafusion/pull/25892#discussion_r4306412212
##########
datafusion/expr/src/logical_plan/plan.rs:
##########
@@ -5190,8 +5190,49 @@ impl Unnest {
let metadata = input_schema.metadata().clone();
let df_schema = DFSchema::new_with_metadata(fields, metadata)?;
- // We can use the existing functional dependencies:
- let deps = input_schema.functional_dependencies().clone();
+ let list_indices: HashSet<_> =
+ list_columns.iter().map(|(index, _)| *index).collect();
+ // Struct unnest may replace one input column with several output columns,
+ // so remap dependencies using the input index of each output column.
+ let remap_indices = |indices: &[usize], exclude_lists: bool| {
+ dependency_indices
+ .iter()
+ .enumerate()
+ .filter_map(|(output_index, input_index)| {
+ (indices.contains(input_index)
+ && (!exclude_lists || !list_indices.contains(input_index)))
+ .then_some(output_index)
+ })
+ .collect()
+ };
+ let deps = FunctionalDependencies::new(
+ input_schema
+ .functional_dependencies()
+ .iter()
+ // A list input no longer determines the rows produced by unnest.
+ // A struct input is also unsafe as a source: a null struct and
+ // a non-null struct with all-null fields have the same outputs.
+ .filter(|dep| {
+ dep.source_indices.iter().all(|index| {
+ !list_indices.contains(index)
+ && !struct_columns.contains(index)
+ && dependency_indices.contains(index)
Review Comment:
nit: `dependency_indices.contains(index)` can't be false once the first two checks pass. Every column that isn't unnested is pushed to `dependency_indices`, and an unnested column is always in `list_indices` or `struct_columns`, because `get_unnested_columns` errors on any other type
##########
datafusion/expr/src/logical_plan/plan.rs:
##########
@@ -5190,8 +5190,49 @@ impl Unnest {
let metadata = input_schema.metadata().clone();
let df_schema = DFSchema::new_with_metadata(fields, metadata)?;
- // We can use the existing functional dependencies:
- let deps = input_schema.functional_dependencies().clone();
+ let list_indices: HashSet<_> =
+ list_columns.iter().map(|(index, _)| *index).collect();
+ // Struct unnest may replace one input column with several output columns,
+ // so remap dependencies using the input index of each output column.
+ let remap_indices = |indices: &[usize], exclude_lists: bool| {
+ dependency_indices
+ .iter()
+ .enumerate()
+ .filter_map(|(output_index, input_index)| {
+ (indices.contains(input_index)
+ && (!exclude_lists || !list_indices.contains(input_index)))
+ .then_some(output_index)
+ })
+ .collect()
+ };
+ let deps = FunctionalDependencies::new(
+ input_schema
+ .functional_dependencies()
+ .iter()
+ // A list input no longer determines the rows produced by unnest.
+ // A struct input is also unsafe as a source: a null struct and
+ // a non-null struct with all-null fields have the same outputs.
+ .filter(|dep| {
+ dep.source_indices.iter().all(|index| {
+ !list_indices.contains(index)
+ && !struct_columns.contains(index)
+ && dependency_indices.contains(index)
Review Comment:
```suggestion
!list_indices.contains(index) && !struct_columns.contains(index)
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org