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(comparison): wrap isnull equality check in parens #8366

Merged
merged 2 commits into from
Feb 15, 2024

Conversation

gforsyth
Copy link
Member

Description of changes

We wrap arguments to a sge.Equals in parens if the inputs are
themselves binary operations. There are other operations we will have
to wrap -- for now I'm just adding isNull to the list, but I imagine
this will grow, possibly on a backend-by-backend basis.

I'm going to investigate the mssql and oracle failures but they are
unrelated to this PR (they fail the added test with or without the parentheses)

Issues closed

@gforsyth
Copy link
Member Author

snowflake and bigquery are passing the new test:

🐚 pytest ibis/backends/tests/test_generic.py -k isnull -m "bigquery or snowflake"
================================ test session starts ================================
platform linux -- Python 3.10.13, pytest-8.0.0, pluggy-1.4.0
Using --randomly-seed=4074414150
benchmark: 4.0.0 (defaults: timer=time.perf_counter disable_gc=False min_rounds=5 min_time=0.000005 max_time=1.0 calibration_precision=10 warmup=False warmup_iterations=100000)
rootdir: /home/gil/github.com/ibis-project/ibis
configfile: pyproject.toml
plugins: xdist-3.5.0, cov-4.1.0, mock-3.12.0, snapshot-0.9.0, randomly-3.15.0, hypothesis-6.98.3, benchmark-4.0.0, repeat-0.9.3, clarity-1.0.1, httpserver-1.0.8
collected 3440 items / 3438 deselected / 2 selected                                 

ibis/backends/tests/test_generic.py ..                                        [100%]

======================== 2 passed, 3438 deselected in 38.28s ========================

@cpcloud cpcloud added this to the 9.0 milestone Feb 15, 2024
@cpcloud cpcloud added bug Incorrect behavior inside of ibis sql Backends that generate SQL labels Feb 15, 2024
@cpcloud cpcloud enabled auto-merge (squash) February 15, 2024 15:23

@pytest.mark.broken(["mssql", "oracle"], reason="incorrect syntax")
def test_isnull_equality(con, backend):
ibis.set_backend(con)
Copy link
Member

Choose a reason for hiding this comment

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

This has to be monkeypatched in to avoid altering the default backend for other tests

Copy link
Member Author

Choose a reason for hiding this comment

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

ARGH

I need to write a pre-commit check for this

We wrap arguments to a `sge.Equals` in parens if the inputs are
themselves binary operations.  There are other operations we will have
to wrap -- for now I'm just adding `isNull` to the list, but I imagine
this will grow, possibly on a backend-by-backend basis.

I'm going to investigate the mssql and oracle failures but they are
unrelated to this PR (they fail the added test with or without the parentheses)
@cpcloud cpcloud merged commit 247e2f7 into ibis-project:main Feb 15, 2024
74 checks passed
ncclementi pushed a commit to ncclementi/ibis that referenced this pull request Feb 21, 2024
)

We wrap arguments to a sge.Equals in parens if the inputs are
themselves binary operations. There are other operations we will have
to wrap -- for now I'm just adding isNull to the list, but I imagine
this will grow, possibly on a backend-by-backend basis.

I'm going to investigate the mssql and oracle failures but they are
unrelated to this PR (they fail the added test with or without the parentheses)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Incorrect behavior inside of ibis sql Backends that generate SQL
Projects
None yet
Development

Successfully merging this pull request may close these issues.

bug: missing parens in SQL with IS NULL
2 participants