Skip to content

Refactor: Do not silently ignore errors in stats_projection #17154

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.

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

alamb
Copy link
Contributor

@alamb alamb commented Aug 12, 2025

Which issue does this PR close?

Rationale for this change

@hareshkh tracked down a performance issue in the stats projection code where we were silently ignoring errors when evaluating the stats projection. This both

  1. Silently masked a real bug (fixed in Pass the input schema to stats_projection for ProjectionExpr #17123)
  2. Caused a performance issue (each DataFusion error allocates a String)

Let's not ignore errors silently, but instead return them to the caller.

What changes are included in this PR?

  1. Return errors rather than ignoring them

Are these changes tested?

Yes, by existing CI tests

Are there any user-facing changes?

No

@github-actions github-actions bot added the physical-plan Changes to the physical-plan crate label Aug 12, 2025
@alamb alamb marked this pull request as draft August 12, 2025 17:35
@alamb alamb force-pushed the alamb/do_not_ignore_erros branch from 61a5139 to cee1f47 Compare August 12, 2025 17:38
@alamb
Copy link
Contributor Author

alamb commented Aug 12, 2025

Many tests fail if we do not also include the fix in

@alamb alamb force-pushed the alamb/do_not_ignore_erros branch from cee1f47 to 8854ffd Compare August 13, 2025 12:45
@alamb alamb marked this pull request as ready for review August 13, 2025 12:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
physical-plan Changes to the physical-plan crate
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants