Skip to content

Pass the input schema to stats_projection for ProjectionExpr#17123

Merged
alamb merged 6 commits intoapache:mainfrom
hareshkh:hareshkh/project-expr-part-stat
Aug 13, 2025
Merged

Pass the input schema to stats_projection for ProjectionExpr#17123
alamb merged 6 commits intoapache:mainfrom
hareshkh:hareshkh/project-expr-part-stat

Conversation

@hareshkh
Copy link
Contributor

@hareshkh hareshkh commented Aug 11, 2025 •
edited
Loading

Which issue does this PR close?

Rationale for this change

bounds_check() requires input schema, while schema field is the output schema.

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

@github-actions github-actions bot added the physical-plan Changes to the physical-plan crate label Aug 11, 2025
@alamb
Copy link
Contributor

alamb commented Aug 11, 2025

Thank you for this fix @hareshkh 🙏

can you please add a test so we don't accidentally break this again in the future?

@hareshkh
Copy link
Contributor Author

hareshkh commented Aug 12, 2025 •
edited
Loading

@alamb - Added a test, checked that it fails if the wrong schema is supplied. Can you please take a pass again?

Also, when this fix merges, is it possible to cherry-pick it on top of 48.0.1 - to release maybe a 48.0.2?

alamb
alamb approved these changes Aug 12, 2025
View reviewed changes
Copy link
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

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

Thank you @hareshkh

I also pushed a commit to this PR to solve the CI error.

Is there any chance you can make a PR to the branch-49 branch so we can include this in the 49.0.1 release as well?

datafusion/physical-plan/src/projection.rs Outdated

assert_eq!(stats.num_rows, Precision::Exact(10));
assert_eq!(stats.column_statistics.len(), 2, "Expected 2 columns in projection statistics");
assert_eq!(stats.total_byte_size.is_exact().unwrap_or(false), true);
Copy link
Contributor

@alamb alamb Aug 12, 2025

Choose a reason for hiding this comment

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

I verified this test covers the change by running the test without the code change and it fails like this

assertion `left == right` failed
  left: false
 right: true

Left:  false
Right: true

@alamb alamb mentioned this pull request Aug 12, 2025
Closed
5 tasks
@alamb
Copy link
Contributor

alamb commented Aug 12, 2025

@alamb - Added a test, checked that it fails if the wrong schema is supplied. Can you please take a pass again?

Also, when this fix merges, is it possible to cherry-pick it on top of 48.0.1 - to release maybe a 48.0.2?

We can consider doing a 48.0.2 release -

If you want to do so can you please file a new ticket to discuss the possibility here

It seems like this might be something we want to include in the upcoming 49.0.1 release at least

@alamb alamb mentioned this pull request Aug 12, 2025
Merged
@hareshkh
Copy link
Contributor Author

hareshkh commented Aug 12, 2025

@alamb : Yes, once this PR merges, I can create a PR to the branch-49

@alamb alamb merged commit 0b7186e into apache:main Aug 13, 2025
27 checks passed
@alamb
Copy link
Contributor

alamb commented Aug 13, 2025

@alamb : Yes, once this PR merges, I can create a PR to the branch-49

Merged! Thank you.

alamb added a commit to alamb/datafusion that referenced this pull request Aug 13, 2025
@hareshkh @alamb
Pass the input schema to stats_projection for ProjectionExpr (apache#…
ab23824 
…17123)

* Pass the input schema to stats_projection for ProjectionExpr

* Adds a test

* fmt

* clippy

---------

Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>
alamb added a commit to alamb/datafusion that referenced this pull request Aug 13, 2025
@hareshkh @alamb
Pass the input schema to stats_projection for ProjectionExpr (apache#…
732c07b 
…17123)

* Pass the input schema to stats_projection for ProjectionExpr

* Adds a test

* fmt

* clippy

---------

Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>
@alamb alamb mentioned this pull request Aug 13, 2025
Merged
@alamb
Copy link
Contributor

alamb commented Aug 13, 2025

Made a backport PR:

@hareshkh hareshkh deleted the hareshkh/project-expr-part-stat branch August 13, 2025 16:04
alamb added a commit that referenced this pull request Aug 13, 2025
@alamb @hareshkh
Pass the input schema to stats_projection for ProjectionExpr (#17123) (…
52e4ef8 
…#17174)

* Pass the input schema to stats_projection for ProjectionExpr

* Adds a test

* fmt

* clippy

---------

Co-authored-by: Haresh Khanna <kh.haresh@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@alamb alamb alamb approved these changes

Assignees

No one assigned

Labels

physical-plan Changes to the physical-plan crate

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Column::bounds_check goes into error path from ProjectionExpr::partition_statistics()

2 participants

@hareshkh @alamb