Skip to content
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

Potential performance regression with comparisions to scalar values #14291

Open
Tracked by #14008
alamb opened this issue Jan 25, 2025 · 2 comments
Open
Tracked by #14008

Potential performance regression with comparisions to scalar values #14291

alamb opened this issue Jan 25, 2025 · 2 comments
Labels
bug Something isn't working

Comments

@alamb
Copy link
Contributor

alamb commented Jan 25, 2025

Describe the bug

There is concern changes in this PR will cause regression (as it may convert some numbers to Decimal128 rather than more efficient Integer)

However, it is not clear (to me at least) exactly the implications of the change

@berkaysynnada has found a potential performance regression, as described here: #14223 (comment)

cc @nuno-faria @jonahgao and @berkaysynnada and @ozankabak

To Reproduce

No response

Expected behavior

I want to make sure that this issue is resolved before we release datafusion 45.0.0, so filing this ticket so we don't forget

Examples of ways to resolve the ticket:

  • Decide the change was fine
  • Revert the changes
  • Others

Additional context

No response

@alamb
Copy link
Contributor Author

alamb commented Jan 25, 2025

I ran benchmarks with / without this change and did not see any noticable performance difference. See details here

I also created a PR with some tests that show the practical impact of this PR on comparisons (I think it is minimal). See

So in other words I would be inclined to close this ticket without further changes given what I currently know

@berkaysynnada
Copy link
Contributor

https://github.com/synnada-ai/datafusion-upstream/tree/benchmark-cast

The last commit shows the simplest case that is regressed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants