-
Notifications
You must be signed in to change notification settings - Fork 695
test: error when sqlglot tells us a compilation is unsupported #11772
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
base: main
Are you sure you want to change the base?
test: error when sqlglot tells us a compilation is unsupported #11772
Conversation
|
Wow, I am surprised that only 2 backends failed with this enabled! |
|
@cpcloud if I fix those failing tests then does this seem like a good idea? |
76a4475 to
2848be3
Compare
This is for `SELECT * FROM t1 INTERSECT/EXCEPT ALL SELECT * FROM t2` Trino inherits the dialect from Presto. Presto doesn't support the `ALL` modifier, but trino does, see trinodb/trino#5890 We ran into this in ibis, where we are indeed able to compile and successfully execute the ALL modifier, but as soon as [we turned on sqlglot errors](ibis-project/ibis#11772), we then started getting spurious sqlglot errors: https://github.com/ibis-project/ibis/actions/runs/19549750869/job/55977864377?pr=11772
|
I think I fixed the flink error, that was easy. For trino, I started tobymao/sqlglot#6616 to fix one of the trino errors. I still need to work on the other trino error. |
This is for `SELECT * FROM t1 INTERSECT/EXCEPT ALL SELECT * FROM t2` Trino inherits the dialect from Presto. Presto doesn't support the `ALL` modifier, but trino does, see trinodb/trino#5890 We ran into this in ibis, where we are indeed able to compile and successfully execute the ALL modifier, but as soon as [we turned on sqlglot errors](ibis-project/ibis#11772), we then started getting spurious sqlglot errors: https://github.com/ibis-project/ibis/actions/runs/19549750869/job/55977864377?pr=11772
This is for `SELECT * FROM t1 INTERSECT/EXCEPT ALL SELECT * FROM t2` Trino inherits the dialect from Presto. Presto doesn't support the `ALL` modifier, but trino does, see trinodb/trino#5890 We ran into this in ibis, where we are indeed able to compile and successfully execute the ALL modifier, but as soon as [we turned on sqlglot errors](ibis-project/ibis#11772), we then started getting spurious sqlglot errors: https://github.com/ibis-project/ibis/actions/runs/19549750869/job/55977864377?pr=11772
2848be3 to
a0f0f1a
Compare
a0f0f1a to
f75a64e
Compare
|
Repro for the trino error for playing around: import ibis
from ibis.backends.sql.compilers.trino import TrinoCompiler
import sqlglot as sg
data = {"a": [[2], [4]]}
t = ibis.memtable(data, schema=dict(a="!array<int8>"))
predicate = lambda x, i: x + (i - i) > 1
# predicate = partial(lambda x, y, i: x > y + (i * 0), y=1)
filtered = t.a.filter(predicate)
sge = TrinoCompiler().to_sqlglot(filtered)
sge.sql(
dialect="trino",
pretty=True,
copy=False,
unsupported_level=sg.ErrorLevel.RAISE,
)We trigger this when compiling ops.ArrayFilter on trino, and in the temp struct we go through, we just pass a bare identifier expression, which sqlglot can't infer the type of. So the solution was to pass a type hint to sqlglot the type of this struct field. Maybe not ideal sqlglot behavior, I feel like it should be valid to compile an untyped field with no warning, but we can work around it. |
8a0adf7 to
e780287
Compare
|
OK @cpcloud all tests pass now! One possible thing to think about here: There may be cases that work just fine, but still raise sqlglot errors, that we aren't seeing in tests. If we all of a sudden make these error, then we might just be breaking some poor user for no real reason. So, perhaps we want to only enable this strictness check in tests? eg by looking at some environment variable that is set in conftest.py? |
This is a sanity check. If sqlglot tells us we are generating invalid SQL, then we should be handling it better.
I'm not sure how CI will like this. Potentially this will reveal a lot of bad assumptions we have been making.
My other motivation here is that if we merge this, then we can push more logic down into SQLglot. For example, if tobymao/sqlglot#6377 gets merged, then we can rely on sqlglot to tell us that
column.first(include_nulls=False)is illegal to compile on postgres (onlycolumn.first(include_nulls=True)is possible to correctly compile. Currently, we have to do these sorts of checks in our individual backend compilers, for example see #11311