Fix three-valued NULL semantics for tuple IN (SELECT ...) - #3685
Open
vishnujayvel wants to merge 1 commit into
Open
Fix three-valued NULL semantics for tuple IN (SELECT ...)#3685vishnujayvel wants to merge 1 commit into
vishnujayvel wants to merge 1 commit into
Conversation
InSubquery.Eval treated hash misses and null-typed right columns as definite FALSE. Row constructors must return NULL when no TRUE match exists but a comparison is unknown (partial NULLs), matching list-form IN and NullUnsafeCompareTuples from dolthub#3651.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes the remaining
IN (SELECT ...)NULL stragglers left open by #3651.Related lineage: dolthub/dolt#11024 (tuple/NULL equality); our earlier draft #3640 was superseded by #3651 and is not reused here.
Context
#3651 (thanks @nicktobey) fixed NullUnsafe tuple comparisons for equality and list-form
IN, and wiredNullUnsafeCompareTuplesinto the hash-hit path ofInSubquery.Eval. The PR body notes:Projection-form
InSubquerystill returned definite FALSE in several three-valued cases.Problem
For row constructors, SQL membership is three-valued:
(1, 5) IN (SELECT 1, NULL …)(1, NULL) IN (SELECT 1, 2 …)(NULL, NULL) IN (SELECT 1, 2 …)(1, 5) NOT IN (SELECT 1, NULL …)(1, NULL) IN (SELECT 2, NULL …)Two gaps in
sql/plan/insubquery.go:NULLvianilKey. Partial-NULL right rows (and lefts with NULL components) never hash-hit, so membership fell through to definite FALSE.SELECT 1, NULLtypes the right astuple(tinyint, null). Converting a non-NULL left component into a null-typed column fails (ErrValueNotNil), and the old code mapped any Convert error to FALSE.WHERE forms are largely OK post-#3651 (unnest to join); residual bugs were in expression-form
InSubquery.Eval.Fix
Minimal continuation of #3651 idioms:
TupleType, scan subquery rows withNullUnsafeCompareTuples(TRUE wins; else anyhasNil && cmp == 0→ NULL; else FALSE).NOT INisNot(InSubquery)— NULL falls out naturally.No planner redesign; no dead guards; comments match house density.
Performance note (tuple hash-miss path)
On the tuple hash-miss / convert-failure path,
evalTupleInSubquerycallsSubquery.EvalMultipleonce per outerEval(sql/plan/insubquery.go). WhencanCacheResults()is true (correlated.Empty() && !volatile,sql/plan/subquery.go), the result cache thatHashMultiplealready populated is shared, soEvalMultiplereturns the cached slice — uncorrelated dual/constant subqueries (all the cases fixed here) pay a linear scan of cached rows, not a re-execution.When
canCacheResults()is false (correlated or volatile subqueries), results are not cacheable, so on the tuple miss path the subquery now executes twice per outer row — once forHashMultiple's hash build and once for the scan — versus once before this change. Scalar / non-tuple paths are unchanged. If that cost matters for correlated tupleIN, a follow-up could precompute ahas-partial-NULL-rowsflag (or retain the row slice) at hash build so the miss path avoids re-executing; left as the maintainer's call.Tests
Promoted the two BrokenQueries counterexamples into
QueryTests(mainvar QueryTests, adjacent to existing scalarNULL IN (SELECT …)cases):(1, 5) IN (SELECT 1, NULL FROM dual)→ NULL(1, 5) IN (SELECT * FROM (SELECT 1, NULL … UNION ALL SELECT 2, 3 …) t)→ NULLand added five new cases:
(1, NULL) IN (SELECT 1, 2 FROM dual)→ NULL(NULL, NULL) IN (SELECT 1, 2 FROM dual)→ NULL(1, 5) NOT IN (SELECT 1, NULL FROM dual)→ NULL(1, NULL) IN (SELECT 2, NULL FROM dual)→ FALSE (definite-mismatch control)(1, 5) IN (SELECT 1, 5 FROM dual)→ TRUE (exact-match baseline)(7 entries total;
KeylessQueriesleft byte-identical to pin.)Verification
Pin:
1fed79c2dd1c7efd12c5e329818b3ab911057542(upstream/main, contains #3651)Full
./enginetest/run end-to-end at base and at this branch (-count=1): identical results — the only failure on both sides is the pre-existingTestRegex(42 subtests, environment-dependent regex expectations; the sorted failing-subtest name sets are identical between base and branch), with zero new failures and zero disappeared. No intentional unrelated changes.Files
sql/plan/insubquery.go— Eval +evalTupleInSubqueryenginetest/queries/queries.go— promote/add QueryTests