Skip to content

Pass the input schema to stats_projection for ProjectionExpr #17123

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
alamb merged 6 commits into from
Aug 13, 2025
Merged

Pass the input schema to stats_projection for ProjectionExpr #17123

alamb merged 6 commits into from
Aug 13, 2025

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