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

Compiling an expression involving an equality of two isnull statements gives unexpected SQL #1307

Closed
jeffgortmaker opened this Issue Jan 28, 2018 · 4 comments

Comments

Projects
None yet
2 participants
@jeffgortmaker

jeffgortmaker commented Jan 28, 2018

The following code

t = ibis.table([('a', 'string'), ('b', 'string')], 'table')
print(ibis.impala.compiler.to_sql(t[t.a.isnull() == t.b.isnull()]))

gives

SELECT *
FROM t
WHERE `a` IS NULL = `b` IS NULL

whereas I would have expected something like

SELECT *
FROM t
WHERE (`a` IS NULL) = (`b` IS NULL)

which isn't logically the same as the first.

@cpcloud cpcloud self-assigned this Jan 28, 2018

@cpcloud

This comment has been minimized.

Member

cpcloud commented Jan 28, 2018

Can you write down how you think the operands associate in the first case?

These are logically equivalent according to the way the parser is written down here: https://github.com/cloudera/Impala/blob/1bfb4cc5277c278459905f5b986f3604d444e669/fe/src/main/cup/sql-parser.cup#L2994

The IS keyword has lower precedence than the = symbol, but the expr nonterminal separates expressions into non-predicate and predicates and the first production in the predicate rule is expr IS NULL, which comes before comparison_predicate which is where the = token is matched.

@cpcloud

This comment has been minimized.

Member

cpcloud commented Jan 28, 2018

Hm, the postgres parser behaves differently here. It parses a is null = b is null as ((a is null) = b) is null which makes sense since both operators are left associative and = is parsed before IS.

I might be misreading the impala parser. Let me squint at it some more.

@jeffgortmaker

This comment has been minimized.

jeffgortmaker commented Jan 28, 2018

Thanks, yes! I just tried out things with SQLite (the same SQL is created by ibis.sqlite.compile), and I think it parses things in the same order as postgres. So I'm guessing Impala is doing that too.

@cpcloud cpcloud added this to the 0.13 milestone Jan 29, 2018

@cpcloud

This comment has been minimized.

Member

cpcloud commented Feb 7, 2018

@kszucs I'm going to take a crack at this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment