sql/jsonpath: preserve boolean results for empty comparisons - #174736
Open
Alignyx wants to merge 1 commit into
Open
sql/jsonpath: preserve boolean results for empty comparisons#174736Alignyx wants to merge 1 commit into
Alignyx wants to merge 1 commit into
Conversation
Comparisons over missing lax members have no matching pair and should return false. Treating their nil result sequence as an evaluation failure instead returns JSON null, causing a following type() method to report "null" instead of "boolean". Evaluate predicate operands with an unsilenced copy of the context so valid empty sequences remain distinguishable from suppressed errors. Preserve actual operand errors as unknown and leave the enclosing context and type-method implementation unchanged. Add regression coverage observing the predicate value, its type, and its unknown status together. Retain genuine unknown and strict/silent error controls, and correct existing expectations for the shared predicate bug. Resolves: cockroachdb#145399 See also: cockroachdb#154589 Epic: none Release note (bug fix): Fixed JSONPath comparisons over missing lax members returning JSON null rather than false, which could cause a following type() method to return "null" rather than "boolean".
|
Thank you for contributing to CockroachDB. Please ensure you have followed the guidelines for creating a PR. My owl senses detect your PR is good for review. Please keep an eye out for any test failures in CI. 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf. |
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.
Problem
The query reported in #145399 returns the JSON string
"null"instead of"boolean":In the default lax mode,
$[*].aselects no items from this input. The comparison has existential semantics: it is true if some selected item satisfies> 3. With no items and no operand-evaluation error, it must be false. Its type is therefore"boolean".The defect occurs before
.type(): the comparison incorrectly producesunknown, which is converted to JSONnull. The type method correctly reports the type of that wrong input. JSONnullhere is not SQL NULL.Metamorphic relations used to establish the correct result
The investigation compared the original query W with two semantics-preserving transformations, C_materialized and C_guard, on the same database state. It observed the predicate value, its
.type(), andis unknowntogether, rather than checking the displayed type alone.Both relations below are scoped to lax evaluation, an error-free path operand producing a sequence of scalar terminal values, and the fixed, error-free scalar right operand
3. Incompatible scalar comparisons are retained as genuine-unknown controls. These are not unconditional rewrite rules for strict paths, suppressed evaluation errors, arbitrary nested arrays, or context-dependent paths.The input matrix was:
MR1: materialize and unwrap the same selected sequence
Let S be the scalar sequence selected by
$[*].a. Materializing S as a JSON array and then automatically unwrapping that root array in a lax comparison preserves the items and their JSON types. ThusS > 3must agree withunwrap(materialize(S)) > 3:The direct root
$is important to the diagnosis. An initial probe using$[*]after materialization still reached the faulty nil-empty representation and was rejected as a correct-result oracle. Comparing the root array instead uses existing automatic unwrapping, producing a non-nil empty slice for[]. It preserves the intended sequence while exposing the representation-dependent result.MR2: guard an existential comparison with existence of its operand
Under the conditions above,
P(S) = (S > 3)must equalexists(S) && P(S): when S is empty, both are false; when S is nonempty, the guard is true and preserves P, including a genuine unknown result.This guard is not an oracle for arbitrary error-producing operands: its short circuit can hide an error. Such cases were tested separately as preservation controls, not counted as equivalent votes.
Observed results
Three complete baseline runs agreed. Both transformed queries agreed on all eight inputs; W disagreed only on the three empty-sequence inputs. Three candidate runs restored agreement. The tuples below are
(predicate, type, is unknown);nulldenotes JSON null.(null, "null", true)(false, "boolean", false)(false, "boolean", false)(false, "boolean", false)(true, "boolean", false)(null, "null", true)(false, "boolean", false)(true, "boolean", false)The expectation is justified by the existential semantics and the two different equivalent evaluation paths, not merely by choosing the majority output. The issue's PostgreSQL result also agrees with the corrected exact reproducer.
How the root cause was localized
For W and C_guard,
EXPLAIN (OPT, VERBOSE)retained the same relational structure; the relevant difference was the JSONPath scalar argument.EXPLAIN ANALYZEshowed the same localscan -> project set -> sortpipeline, with eight decoded input rows and eight emitted rows at each operator. The separate SELECT results above establish the value discrepancy; EXPLAIN ANALYZE itself is not a value oracle.Row-engine and forced-DistSQL-setting runs reproduced the same result matrix. The captured W/C analyzed plans ran on n1, so this evidence does not claim a cross-node execution trace. It localizes the discrepancy to scalar JSONPath evaluation rather than row loss or a different relational operator pipeline. The following branch-level explanation comes from source inspection, not from instrumented branch tracing.
In
pkg/util/jsonpath/eval/operation.go,evalPredicatepreviously treatedleft == nilorright == nilasunknown, even when the returned error was nil. However, a successful lax path with no matches can legitimately return a nil slice. The original query therefore followed this path:The materialized-root arm represents the same empty sequence as a non-nil empty slice and reaches the existing false result. The existence-guard arm yields false for the empty selection. This explains both the observed metamorphic violation and its precise source-level divergence.
There is a second constraint: with
silent=true, a real evaluation error can also be suppressed into(nil, nil). Simply removing the nil checks would therefore conflate genuine errors with successful empty results and incorrectly change some unknown predicates to false.Repair and rationale
evalPredicatenow copies the evaluation context and disables silent suppression only while evaluating its operands:Both operands use this copy. The existing error classification is retained: non-ignorable errors still propagate, and ignorable operand errors produce
unknown. When evaluation succeeds, a nil slice is accepted as a valid empty sequence and reaches the existing no-matching-pair result, false. Non-nil empty sequences follow the same semantics.This is preferable to special-casing
.type()because it repairs the predicate value itself and preserves the type of genuinely unknown predicates. It is preferable to merely removing the nil guards because it preserves real-error behavior. Copying the context keeps the enclosing expression's silent mode and existing evaluation state intact; it does not change global silent-mode policy. Theevalreturn-value comment is updated to document the actual ambiguity instead of treating nilness as a reliable failure indicator.The repair also respects the relevant implementation history:
Regression coverage and completed local validation
The checked-in
regression_145399section adds the exact reported query, the eight-input value/type/unknown matrix, a right-side-empty comparison, and strict-missing-member, silent arithmetic-error, and actual JSON-null type controls. The new regression was red on the baseline and green on the candidate with unchanged test bytes. Existing expectations injsonb_path_exists_index_accelerationare corrected for the same shared predicate behavior. No.type()implementation is modified.The complete MR transformations above were investigation SQL; the committed native regression asserts their established outcomes and preservation controls rather than adding the full MR harness to the repository.
Completed local validation for commit
48ebc1147e82e6174ca522022fcf1dfe53a296b6, based on8812064a015d2faf99d3fc7e15880f94042954b0, includes:local,local-vec-off,fakedist,fakedist-vec-off, andfakedist-disk, including actual RUN/PASS records for this regression in every configuration.pkg/sql/sem/eval, JSONPath parser, and SQL package test targets; SQL ran in 16 shards.These are completed local results, not a claim that upstream PR CI has passed.
Scope and related issue
This fixes #145399 at its predicate root cause and covers the related empty-operand behavior handled by the same evaluator. It deliberately preserves genuine unknown comparisons, strict structural errors, ignorable-error conversion to unknown, non-ignorable error propagation, and outer silent behavior. It does not redesign
.type(), JSON null semantics, or all JSONPath compatibility behavior; the MR assumptions above are not broadened into general optimizer rewrites.#154589 has the same underlying nil-empty predicate defect, exposed through regex. The two production-file changes and existing acceleration-fixture corrections are identical to that issue's prepared fork repair. This PR carries the shared fix with the dedicated #145399 regression; it is not a second independent implementation, and it does not include #154589's separate new regression section. There is no separate upstream PR for that fork repair at submission time. The shared production patch should be landed once, not as two unrelated fixes.
Resolves: #145399
See also: #154589
Epic: none
Release note (bug fix): Fixed JSONPath comparisons over missing lax members returning JSON null rather than false, which could cause a following type() method to return "null" rather than "boolean".