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

fix(compiler): respect order of ops in FloorDivide #10353

Merged
merged 1 commit into from
Oct 24, 2024

Conversation

NickCrews
Copy link
Contributor

@NickCrews NickCrews commented Oct 23, 2024

Consider ibis.literal(6) // (1 / ibis.literal(2)).

Before, this compiled to 6 / 1 / 2 -> 3, when we really want 6 / (1 / 2) -> 12.

This fixes that.

Found in NickCrews/mismo#74

@github-actions github-actions bot added tests Issues or PRs related to tests sql Backends that generate SQL labels Oct 23, 2024
@cpcloud cpcloud changed the title fix: respect order of ops in FloorDivide fix(compiler): respect order of ops in FloorDivide Oct 23, 2024
Copy link
Member

@cpcloud cpcloud left a comment

Choose a reason for hiding this comment

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

Thanks!

@NickCrews
Copy link
Contributor Author

NickCrews commented Oct 23, 2024

Uhhhh it is VERY annoying to get these very opaque flink errors and then not be able to run flink locally to debug. @cpcloud any idea what the problem here is?

@cpcloud cpcloud added this to the 10.0 milestone Oct 23, 2024
@cpcloud
Copy link
Member

cpcloud commented Oct 23, 2024

Ultimately the error isn't opaque in this case:

https://github.com/ibis-project/ibis/actions/runs/11470981543/job/31921152707?pr=10353#step:16:256

image

It is possible to run the Flink backend, but it's a bit of a slog to get it working due to their Python API requiring an ancient version of arrow.

@NickCrews NickCrews force-pushed the floor-divide-parens branch from 6e380f1 to e8f1a75 Compare October 23, 2024 20:46
@NickCrews
Copy link
Contributor Author

Ah, thanks, I didn't see that buried in the rest of the stacktrace. I will look harder next time.

I wonder if this is pointing to that 1/2 in flink performs integer division and results in 0. I made my test case be immune to this, and added a tiny separate test case to check if my hunch is true. If so, then I can open a separate PR that adds that test, marked as xfail on flink.

@NickCrews NickCrews force-pushed the floor-divide-parens branch from e8f1a75 to 5ad6031 Compare October 23, 2024 21:28
@NickCrews NickCrews force-pushed the floor-divide-parens branch from 5ad6031 to fb85936 Compare October 23, 2024 21:34
@cpcloud cpcloud merged commit af24c8e into ibis-project:main Oct 24, 2024
77 checks passed
@NickCrews
Copy link
Contributor Author

NickCrews commented Oct 24, 2024

int division test filed at #10359

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
sql Backends that generate SQL tests Issues or PRs related to tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants