Ishaan400 commented on PR #26164:
URL: https://github.com/apache/datafusion/pull/26164#issuecomment-6093086663
Thanks for working on this, @siddubakka! I agree the direct pattern looks
right: `x IS NOT NULL AND x` (and `x IS NULL OR x`) can never be NULL.
I think the recursive search through AND/OR chains is unsafe, though. A
guard deeper in the chain doesn't make the whole expression non-nullable when
other operands are nullable. For example, `y AND x IS NOT NULL AND x` with `y =
NULL, x = true` evaluates to `(NULL AND true) AND true = NULL`, but
`nullable()` now returns `false`. Because the simplifier rewrites `e IS NULL`
to `false` when `e` is not nullable, this produces wrong results. With this PR
the following returns `false` for row 1 (expected `true`), and the `count(*)`
returns 0 (expected 1). The OR mirror `y OR x IS NULL OR x` fails the same way
for `y = NULL, x = false`. Both are correct on `main`:
```sql
CREATE TABLE t(id INT, y BOOLEAN, x BOOLEAN) AS VALUES
(1, NULL, true), (2, true, true), (3, NULL, NULL), (4, false, false), (5,
NULL, false);
SELECT id, y AND x IS NOT NULL AND x, (y AND x IS NOT NULL AND x) IS NULL
FROM t ORDER BY id;
-- expected: 1 NULL true | 2 true false | 3 false false | 4 false false | 5
false false
SELECT count(*) FROM t WHERE (y AND x IS NOT NULL AND x) IS NULL;
-- expected: 1
SELECT id, (y OR x IS NULL OR x) IS NULL FROM t ORDER BY id;
-- expected: only row 5 is true
```
Two related observations:
- The change is in `Expr::nullable()`, but plan schemas are built through
`to_field()`, which still computes `lhs || rhs` for AND/OR. So the two now
disagree, and I don't think this change affects the schema mismatch from the
issue.
- Does #25978 still reproduce on current `main`? Locally, the issue's
original query and the new `issue_25978.slt` both pass at this PR's base commit
with the `expr_schema.rs` change reverted. The original query does fail at the
55.1.0 version bump (`f2b2ffd40`), and bisecting points to #25143 (`587e74c84`)
as the commit where it starts passing. If that's right, maybe this PR could
become a regression test for the issue, plus the direct-pattern case if you'd
like to keep it, implemented consistently with `to_field()`.
Happy to share the full reproduction if useful.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
