fix(plan): handle NULL left operand in IN predicates - #25236
Conversation
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
XuPeng-SH
left a comment
There was a problem hiding this comment.
I found two substantive blockers here.
NULL IN (...)is now folded before volatility checks, so RHS side effects disappear.
The new early return replaces the whole predicate with a constant NULL before the RHS expressions are preserved in the plan. That bypasses the normal “do not fold volatile functions” safeguard. So a query like SELECT NULL IN (nextval('s')) would no longer advance the sequence, which is an observable behavior change and not compatible with the normal planner rule that volatile expressions must not be folded away.
Suggested fix: don’t replace the predicate with a constant at bind time. Keep the normal RHS/type-checking path and only fold when the RHS is provably side-effect-free.
- The early return skips RHS validation, so invalid scalar-vs-row
INlists can now be silently accepted.
Because the new return happens before the normal RHS comparison-building/validation path, malformed RHS items are never checked if the left operand is a literal NULL. A query like NULL IN ((1,2)) should still be rejected as an invalid scalar-vs-row comparison, not accepted as a constant NULL.
Suggested fix: keep the normal RHS shape/type validation, then handle the constant-NULL result after the list form is known to be valid.
The current SQL regression covers the original reported bug, but it does not protect either of these new semantic regressions.
5e0acb2 to
8cbfadc
Compare
aunjgr
left a comment
There was a problem hiding this comment.
All three concerns from the earlier review are resolved:
- Volatility:
NULL IN (nextval('s'))correctly advances the sequence (regression provescurrval = 1). - RHS validation:
NULL IN ((1,2))now usesErrOperandColumns(ER_OPERAND_COLUMNS, SQLSTATE 21000) — proper user-facing error, not internal error. - No early folding: validation happens before the NULL special-case.
LGTM.
|
Queued — the merge queue status continues in this comment ↓. |
Merge Queue Status
This pull request spent 1 hour 11 minutes 33 seconds in the queue, including 1 hour 11 minutes 11 seconds running CI. Required conditions to merge
|
What type of PR is this?
Which issue(s) this PR fixes:
issue #25221
What this PR does / why we need it:
NULL IN (1, NULL).NULL IN (1, NULL)andNULL NOT IN (1, NULL).