json: extract non-finite numeric values without JSON quotes - #174766
Open
Alignyx wants to merge 1 commit into
Open
json: extract non-finite numeric values without JSON quotes#174766Alignyx wants to merge 1 commit into
Alignyx wants to merge 1 commit into
Conversation
Non-finite numeric JSON values are formatted with quotes so that their JSON representation remains valid. Text extraction incorrectly reused that representation, returning extra quotes for locally constructed or binary-encoded NaN and infinities. A remotely serialized constant instead became a JSON string and returned the unquoted text. Use decimal text directly in jsonNumber.AsText. Keep JSON formatting, numeric type tags, constructors and binary/index encoding unchanged, including for legacy numeric JSON. This does not normalize the internal JSON types across a textual roundtrip. Add native coverage for float/decimal non-finite values, decoded and encoded scalars, legacy encoding bytes, nested leaves, runtime and stored SQL values, and unchanged finite scales and container formatting. Resolves: cockroachdb#144837 Epic: none Release note (bug fix): Fixed JSON text extraction operators such as ->> and #>> returning extra quotes around NaN and infinities constructed from numeric values. Results now agree with text extraction after remote constant serialization. JSON formatting and stored numeric JSON types remain unchanged.
|
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.
Summary
Fix extra JSON quotation marks in text extracted from numerically constructed
NaN,Infinity, and-Infinity. In #144837, the same typed SQL query returnsdifferent text depending on whether a non-finite JSON constant is evaluated
locally or serialized into a remote expression.
The production change is confined to
jsonNumber.AsText: return the decimal'stext instead of its JSON serialization. JSON formatting, constructors, internal
numeric type tags, comparison, and binary/index encoding are unchanged. The PR
includes native unit and SQL regressions, including fixed legacy encoding bytes.
This is a text-extraction consistency fix, not a normalization of non-finite
JSON values into JSON strings throughout the system.
Problem and expected behavior
A reduced query retains the report's
FLOAT8cast and makes the extraction indexa runtime column, so the complete extraction cannot simply be constant-folded:
For
i = 0, JSON displayjis"NaN", but extracted SQL texttshould beNaN, without the two JSON wrapper quotes. For the out-of-range scalar indexi = 1, extraction remains SQL NULL. These are different observations: validJSON serialization needs the quotes, whereas scalar text extraction does not.
The baseline local query returns the five-character text
"NaN"; the remotelyserialized constant returns the three-character text
NaN. Numericallyconstructed infinities have the same extraction defect. An explicit JSON string
already extracts without its JSON wrapper quotes.
Metamorphic analysis and correct-result oracle
Primary relation: execution-location invariance
For this deterministic query over the same data and SQL types, moving evaluation
from the gateway to a remote node must preserve the extracted SQL text. Switching
between vectorized and row execution must preserve it as well.
The investigation used the identical query above in a three-node cluster. The
table lease was placed on node 2. With
distsql = off, SQL execution was localon node 1; with
distsql = always, the distributed arm executed on node 2.Actual
EXPLAIN ANALYZEnode/distribution observations were checked, rather thantreating a setting change alone as proof of remote execution. The data, explicit
FLOAT8cast, projection, and runtime extraction index were unchanged.This relation detects an inconsistency without assuming that either plan is
correct simply because it ran locally or remotely. The intended unquoted text
is independently supported by the scalar text-extraction contract, the existing
JSON-string extraction behavior, and the following auxiliary controls.
Auxiliary text oracles, with an important type boundary
The following local controls both extract
NaNat index 0:These controls change the representation boundary, not just the physical plan.
They must not be described as preserving every internal JSON property:
That distinction persists with this patch. The controls are oracles for the
extracted text and clues to the representation boundary; the unchanged typed
query under execution-location transformation is the primary metamorphic relation.
Runtime
to_json(f)andDECIMALinputs were also checked to distinguishconstant serialization from numeric construction during execution.
Repeated observations
Three complete baseline runs and three complete candidate runs agreed. The
following table describes text at
i = 0; index 1 remains SQL NULL in every arm."NaN"NaN"NaN"NaNNaNNaN"NaN"NaNNaNNaNIn particular, remote execution is not universally correct on the baseline:
the runtime-column arm still constructs a numeric JSON value remotely and exhibits
the defect. This separates the serialization boundary from the node or engine
itself. All nine W/C arms agree after the repair.
Root cause and execution-path localization
tree.AsJSONsends FLOAT and DECIMAL datums toFromFloat64andFromDecimal.These constructors create a
jsonNumber, including for non-finite decimal forms.The following existing paths then disagree:
jsonNumber.String()callsFormat(). For non-finitevalues,
Format()writes quotes aroundapd.Decimal.String()to producevalid JSON text, such as
"NaN".jsonNumber.AsText()previously calledj.String(), inadvertently returning that JSON-formatted text, quotes included.ExprFactory.Make's expression-text boundary.DJSON.Formatemits a SQL literalcontaining the JSON text;
DeserializeExprparses/type-checks it on the remoteside. The quoted non-finite representation is then a genuine
jsonString.Its existing
AsText()returns the string contents without JSON wrapper quotes.to_json(f)constructs the numeric JSON valueduring execution, so it does not benefit from that folded-constant conversion.
This identifies the faulty interface as numeric text extraction reusing JSON
serialization, rather than the decimal value, scan contents, or a particular
execution engine.
The plan comparison supports that source-level explanation:
rendered
EXPLAIN (OPT, VERBOSE)output. JSON rendering hides the internalnumber/string distinction; identical plan text is not proof of identical datums.
constant arm records full distribution and SQL/KV work on node 2, with network
usage. The scan and render each process two rows.
consistent with the expression-representation boundary, not missing data.
These are source-path checks combined with optimizer and actual-plan observations,
not instrumented, ordered branch-coverage traces.
How the repair works and why this boundary was chosen
The change is:
For finite numbers,
Format()already writes this exact decimal text withoutquotes. Using it directly therefore preserves finite formatting and decimal scale,
including the tested
1.2300and-12.50cases. For non-finite numbers, it avoidsonly the JSON wrapper quotes.
Scalar/index/key
->>and path#>>extraction ultimately useAsText()on theselected value, so the correction applies to numeric leaves in arrays and objects
as well as standalone scalars.
JSONEncoded.AsText()decodes and delegates to thesame method. Consequently, legacy binary numeric JSON
also receives the corrected extraction behavior without changing its stored type.
The alternative of making constructors return JSON strings would change type tags,
comparison and encoding behavior, and would not by itself fix already encoded
numeric values. Keyside numeric JSON decoding also reconstructs values through
FromDecimal. This PR deliberately leaves those contracts alone.The historical dea0313b change
added non-finite JSON quoting and a parseability regression. This repair preserves
that behavior: JSON serialization still emits
"NaN","Infinity", and"-Infinity". Extracting a whole container still returns its JSON representation,including quoted non-finite leaves; extracting the leaf returns its text contents.
Completed local validation
The following results apply to repair commit
474855b6b74253da0ce4a55ef57bd731c233fa65, based on8812064a015d2faf99d3fc7e15880f94042954b0. They are completed local validation,not a claim that upstream PR CI has passed.
TestJSONNonFiniteAsTextcovers FLOAT andDECIMAL non-finite values, finite controls, decoded/encoded values with repeated
extraction, fully
decoded values, nested leaves and container formatting. Baseline has 21 failing
non-finite leaf subtests and 15 passing finite leaf subtests; the unchanged
regression passes completely with the candidate.
Number and decimal tags. Tests decode those bytes, check type and decimal
components, repeat extraction and re-encode to the same bytes. JSON
String()and
Format()remain unchanged and produce parseable JSON.regression_144837injson_builtinscoversFLOAT/DECIMAL NaN and both infinities, valid scalar indices 0/-1, out-of-range
indices, runtime numeric and stored JSON columns, object/array/path extraction,
finite scale, JSON formatting/type and string-versus-SQL NULL controls. It fails
on baseline and passes on the candidate. The baseline run stops at the first
failing FLOAT query; later statements are not claimed to have separate native-red
observations.
pass the archived verifier's exact expected-outcome checks. These include actual
local/remote nodes, two-row scan/render counts, strict CSV quote interpretation,
and type/NULL/finite controls. Twelve corrupted or mislabeled evidence variants
and four malformed CSV variants are rejected.
mismatches and zero candidate mismatches; eight defect scenarios are corrected.
Coverage includes scalar/key/nested extraction, array/each text extraction,
stored scalars and container leaves, with finite, string/NULL, full-container
and internal Number-type controls. Ten verifier rejection self-tests pass.
eval unit targets, plus the full SQL target across 16 shards, pass locally.
Complete
json,json_builtins,postgres_jsonbandjson_indexlogic filespass under
local,local-vec-off,fakedist,fakedist-vec-offandfakedist-disk(20 file/configuration executions).One preliminary candidate replay stopped during lease relocation before any W/C
query because its target replica was not yet ready. That setup-only attempt was
excluded and retained separately; the three complete candidate runs used the
unchanged SQL and strict actual-node checks.
The PR contains three files (+148/-1): the small
json.goproduction change,json_test.gocoverage and thejson_builtinsregression. Supplemental replay andheld-out logs/verifiers are retained in the local investigation record, not added
as files to this PR.
Current limitations
JSON can still have type
number, while its parsed textual representation hastype
string. This PR does not unify those types or establish equivalence forevery type-sensitive comparison, JSONPath expression or other JSON operation.
AsText()extraction path. It does not redesign everyJSON-to-SQL conversion or change JSON parsing, numeric constructors,
Format(),Compare(), numeric type tags, or binary/index encoding.extracted TEXT, stored computed results or expression-index entries derived
from the old quoted extraction are not automatically recomputed. Preserving
JSON storage encoding does not establish that all such derived data needs no
operational follow-up; affected cases require separate assessment.
each version independently; they are not an upgrade of the same storage directory
or a mixed-version cluster qualification. Fixed legacy byte tests separately
cover the numeric binary representation. No performance benchmark or blanket
backport certification is supplied.
If a broader normalization of non-finite JSON types is desired, it should be
reviewed separately with explicit decisions about legacy numeric values,
comparison/index semantics and derived-data migration. This PR makes no claim
that such a migration or normalization design is already implemented.
Resolves: #144837
Epic: none
Release note (bug fix): Fixed JSON text extraction operators such as ->>
and #>> returning extra quotes around NaN and infinities constructed from
numeric values. Results now agree with text extraction after remote
constant serialization. JSON formatting and stored numeric JSON types
remain unchanged.