eakmanrq / sqlframe

Turning PySpark Into a Universal DataFrame API
https://sqlframe.readthedocs.io/en/stable/
MIT License
174 stars 3 forks source link

Inverting boolean filter with OR not retaining parenthesis on .sql() (for Spark) #74

Closed TinoSM closed 2 weeks ago

TinoSM commented 3 weeks ago

Before you file an issue

I have checked with sqlglot 18 (which had spark dataframes functionality embedded) and also with sqlframes newest version (which uses sqlglot 25.0.3).

pip install https://codeload.github.com/eakmanrq/sqlframe/zip/refs/heads/main Collecting https://codeload.github.com/eakmanrq/sqlframe/zip/refs/heads/main Using cached https://codeload.github.com/eakmanrq/sqlframe/zip/refs/heads/main Installing build dependencies ... done Getting requirements to build wheel ... done Installing backend dependencies ... done Preparing metadata (pyproject.toml) ... done Requirement already satisfied: prettytable<3.11.0 in ./.venv/lib/python3.11/site-packages (from sqlframe==0.0.0) (3.10.0) Requirement already satisfied: sqlglot<25.2,>=24.0.0 in ./.venv/lib/python3.11/site-packages (from sqlframe==0.0.0) (25.0.3) Requirement already satisfied: typing-extensions<5,>=4.8 in ./.venv/lib/python3.11/site-packages (from sqlframe==0.0.0) (4.12.2) Requirement already satisfied: wcwidth in ./.venv/lib/python3.11/site-packages (from prettytable<3.11.0->sqlframe==0.0.0) (0.2.13)

Fully reproducible code snippet

import sqlframe.spark.functions as F
col1=(F.lit(False) | F.lit(True))
(~col1).sql(dialect="spark")

xx=~sqlframe.spark.Column(
                    sqlglot.expressions.Paren(this=col1.expression) #type: ignore
                )
xx.sql(dialect="spark")

Output:


It looks like the NOT sql() representation is missing something
>>> col1.__invert__().sql()
'NOT FALSE OR TRUE'
>>> col1.__invert__()
Not(
  this=Or(
    this=Boolean(this=False),
    expression=Boolean(this=True)))

>>> xx.sql(dialect="spark")
'NOT (FALSE OR TRUE)'

Official Documentation Please include links to official SQL documentation related to your issue.

eakmanrq commented 2 weeks ago

Thanks for the detailed report! Fixed in 1.9.0.

TinoSM commented 2 weeks ago

Ty!