Skip to content

fix: preserve JSON scalar types in boolean comparisons - #27540

Open
jiangxinmeng1 wants to merge 15 commits into
matrixorigin:mainfrom
jiangxinmeng1:fix-27529-json-string-bool
Open

fix: preserve JSON scalar types in boolean comparisons#27540
jiangxinmeng1 wants to merge 15 commits into
matrixorigin:mainfrom
jiangxinmeng1:fix-27529-json-string-bool

Conversation

@jiangxinmeng1

Copy link
Copy Markdown
Contributor

What type of PR is this?

  • API-change
  • BUG
  • Improvement
  • Documentation
  • Feature
  • Test and CI
  • Code Refactoring

Which issue(s) this PR fixes:

issue #27529

What this PR does / why we need it:

Fix JSON boolean comparisons that incorrectly treat JSON strings such as "true" and "false" as boolean values.

  • Preserve JSON scalar types when casting JSON to BOOL; JSON strings now evaluate to SQL NULL.

  • Preserve prepared-parameter types in JSON equality comparisons, including boolean, numeric, and string
    parameters.

  • Add regression coverage for direct SQL, prepared statements, JSON NULL, missing paths, and JSON string values.

  • Update related [Bug]: JSON_EXTRACT boolean comparison fails with JSON BOOL cast error #27187 coverage and distributed BVT cases.

@qodo-code-review

Copy link
Copy Markdown

Qodo reviews are paused for this user.

Troubleshooting steps vary by plan Learn more →

On a Teams plan?
Reviews resume once this user has a paid seat and their Git account is linked in Qodo.
Link Git account →

Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center?
These require an Enterprise plan - Contact us
Contact us →

@aunjgr aunjgr left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Deep-reviewed exact head 5a32fcb710893a16da22faf1480e5dc700efed1a against merge base ba8954d543af0bdfb1948a8b89f77f5235ac9259.

JSON-to-BOOL now preserves scalar category: JSON booleans/numbers retain coercive boolean behavior, JSON strings produce SQL NULL rather than parsing their contents, and JSON null/missing paths remain NULL. Equality prepared parameters are normalized from transport TEXT into typed JSON boolean/integer/float/decimal/string values before comparison; ordering parameters retain their separate exact-number path. Both operand orientations and =/inequality public paths are covered through function, planner, prepared-protocol, embedded-cluster, and BVT oracles.

The internal function ID is appended without renumbering existing functions, and the normalization is applied only to a direct dynamic parameter paired with JSON. No blocking correctness, compatibility, lifecycle, or material hot-path issue found.

@XuPeng-SH XuPeng-SH left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Deep review of exact head 5a32fcb710893a16da22faf1480e5dc700efed1a.

[P1] Preserve prepared JSON scalar types for null-safe equality (<=>) as well

The new normalization is only wired into = / <> (and parser-normalized !=). <=> is another equality operator, but it still compares the binary-protocol parameter through its transport TEXT representation.

A deterministic embedded-engine counterexample:

-- direct controls
select json_extract('{"v":true}', '$.v') <=> true;       -- true
select json_extract('{"v":"true"}', '$.v') <=> true;     -- false

-- prepared once, execute with two boolean true parameters
select json_extract('{"v":true}', '$.v') <=> ?,
       json_extract('{"v":"true"}', '$.v') <=> ?;
-- actual:   false, false
-- expected: true,  false

So the PR still makes a JSON boolean unequal to a boolean parameter for a supported equality operator; the scalar-provenance fix is not closed over the operator family. Please route <=> through the same type-aware JSON parameter normalization while preserving its SQL NULL-safe semantics, and add prepared-protocol regressions for both operand orientations covering boolean, string, and NULL values.

Validation performed: focused tests pass at -count=20; focused race tests pass at -race -count=5; the two embedded issue regressions pass; go vet passes for the affected packages.

@aptend aptend left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Deep review on exact head 5a32fcb710893a16da22faf1480e5dc700efed1a.

The direct = / <> fix works for the covered scalar domains, and JSON strings now remain distinct from JSON booleans. Two prepared equality paths still lose the boolean parameter category: null-safe equality and NOT IN. Both are blocking correctness gaps; see the inline counterexamples.

Validation: full CGo-backed tests for ./pkg/sql/plan and ./pkg/sql/plan/function; the two embedded issue regressions; focused race tests at -count=20; go vet; compile-only tests; dependency listing; gofmt -d; git diff --check; clean-worktree check; and merge-tree against current main. Repository tests pass. A temporary binary-protocol embedded counterexample returned (false, false, true) for JSON boolean <=> boolean parameter, JSON string <=> boolean parameter, and JSON boolean NOT IN boolean parameter respectively; expected (true, false, false).

Comment thread pkg/sql/plan/base_binder.go
Comment thread pkg/sql/plan/base_binder.go Outdated

@XuPeng-SH XuPeng-SH left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Deep re-review of exact head 954e6db0ac1fd055342e8ba730ae0799e1f9276d after my previous review on 32850e898bd65a3e250360dc4ed5880ed646f3b1.

The small boolean parity case is fixed, but the replacement comparison engine has blocking correctness and performance problems:

[P1 correctness] Integer precision is lost. With a binary-protocol int64(9007199254740993) parameter, json_extract(json_array(9007199254740992), "$[0]") = ? returns true; the equivalent typed-literal expression returns false. I reproduced this through the embedded database/sql test. preparedJSONValue converts both ByteJSON operands through MarshalJSON plus json.Unmarshal(any), so integers and decimals become float64 and distinct values above 2^53 collapse. This violates the stated typed-parameter parity contract.

[P1 execution contract] comparePreparedJSON ignores FunctionSelectList. A focused function test with row 1 masked still produced a non-NULL comparison result for row 1. Equality operators must leave masked rows NULL and must not evaluate them; otherwise CASE/short-circuit execution semantics and error isolation are broken.

[P1 performance] This is a vector comparison hot path, but every row now performs two ByteJSON-to-text marshals, two standard-library JSON decodes into any, dynamic type switches, and sometimes fmt.Sscan or another JSON marshal. normalizeJsonComparisonParam also re-encodes a constant parameter and builds per-row metadata for the whole batch. This adds substantial allocation and CPU amplification to JSON predicates.

Please preserve the native ByteJSON scalar representation and route each PrepareParamKind through the existing typed cast/comparison semantics instead of rebuilding those semantics with any and float64. Honor selectList, add a constant-parameter fast path, and preserve direct-expression error behavior for unsupported scalar/category casts. Regressions should cover signed/unsigned boundaries around 2^53 and max values, decimals, invalid/object/array casts, both operand orientations, masked rows, and =, <=>, IN, and NOT IN. Author-provided focused cases otherwise pass.

@XuPeng-SH

Copy link
Copy Markdown
Contributor

已在 7496fc7b96 系统性处理最新 Request Changes:

  • 移除 ByteJSON → JSON text → any/float64 的逐行往返,比较直接使用原生 ByteJSON 与 SQL 参数类别;修复 2^53、int64/uint64 边界精度。
  • 复用 JSON→BOOL/数值 cast 语义,保持 prepared 参数与同类型 SQL literal 一致;覆盖 =<=>、IN/NOT IN 和双向操作数。
  • 正确执行 FunctionSelectList:masked row 保持 NULL 且不求值;object/array 非法 cast 返回错误,服务保持可用。
  • 常量参数只编码一次、共享 varlena payload,uniform provenance 不分配 row sidecar。

验证:

  • ./pkg/sql/plan/function 全量通过;./pkg/sql/plan 全量通过。
  • focused -race -count=20 通过;最终语义修改后 focused -race -count=10 通过。
  • embedded TestIssue27529JSONStringsDoNotCompareAsBooleansTestIssue27187JSONExtractBooleanComparison 通过。
  • go buildgo vetgit diff --check 通过。
  • 同一 1024-row benchmark:原 head 1.061–1.087ms/op、475KB/op、14336 allocs/op;当前 16.15–16.29µs/op、0 B/op、0 allocs/op(约 65×)。

@XuPeng-SH XuPeng-SH left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Deep re-review of exact head 9b0f114c00468a1a88fc9a5be19a3d8e1f2d300b.

The previous 2^53 precision, FunctionSelectList, and per-row marshal/allocation blockers are fixed, and the original #27529 regressions now pass. One design-level P1 remains: the comparison adapter still does not preserve the concrete numeric SQL parameter type.

PrepareParamKind collapses every integer width/signedness into Integer and both FLOAT/DOUBLE into Float. comparePreparedJSONScalars then compares raw signed/unsigned integers or converts both operands to float64. That differs from the existing typed-literal resolver, which casts JSON to the concrete peer type.

Two deterministic public-path counterexamples:

  1. JSON uint64 max = CAST(int64 max AS SIGNED) correctly returns invalid argument operator cast, bad value [JSON -> BIGINT]. The binary-protocol prepared equivalent with an int64(math.MaxInt64) argument returns false with no error, bypassing the required JSON-to-BIGINT overflow check.
  2. JSON 16777217 = CAST(16777216 AS FLOAT) is true after the required float32 rounding. The text-prepared equivalent using a FLOAT user variable returns false, because the adapter compares both values as float64.

Please preserve the concrete runtime target OID (including integer signedness/width and FLOAT versus DOUBLE), or route through the existing typed cast/comparison path, so prepared execution has the same values and errors as the equivalent typed literal. Add boundary regressions for overflow/error parity and float32 rounding across the affected equality paths and operand orientations.

Validation on this head: focused planner/function/cast/function-ID tests pass; both embedded issue regressions pass; the two counterexamples above fail exactly as described; the 1024-row integer benchmark is 16.17-16.69 us/op with 0 B/op and 0 allocs/op; git diff --check and gofmt pass. I did not wait for the still-running full CI because these are deterministic semantic failures.

@XuPeng-SH XuPeng-SH left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Deep re-review of exact head 0a61cc2 after fixing the concrete numeric-type blocker. Prepared JSON equality now preserves integer width and signedness plus FLOAT32 semantics across binary and text EXECUTE, including remote process dispatch with a version-gated, fail-closed metadata contract. The direct/prepared BIGINT overflow and FLOAT32 rounding counterexamples now agree. Exact metadata is scoped to affected JSON parameters and allocated lazily; unrelated and common BOOL/DOUBLE/DECIMAL paths keep the existing fast path. Native ByteJSON comparison remains 0 B/op and 0 allocs/op. Full affected package tests and the embedded public SQL regression pass locally. Q1-Q3 audit found no new cleanup, wait, or unbounded per-row state paths. No remaining blocking correctness, performance, compatibility, or unhappy-path issue found.

@XuPeng-SH XuPeng-SH left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Deep review of exact head 0a61cc2b23fd9a3c7faf71ec4aabac6f35f3d4e7 against base/merge-base dddd58c852c674ebce83d1b224f74cf646999814.

[P1 compatibility] Gate the new hidden function itself during rolling upgrades, not only its typed metadata.

This PR unconditionally rewrites a direct prepared parameter paired with JSON equality to __mo_json_comparison_param, whose new function ID is 577. The base/old CN function table ends at ID 576 (FUNCTION_END_NUMBER = 577), so an old receiver cannot build that expression: GetFunctionById decodes 577 and returns function overload id not found.

The v30 guard in PrepareParamMetadataForRemote does not close this compatibility boundary. It requires v30 only when a 12N payload contains an exact integer/FLOAT32 type. BOOL, string, DOUBLE, DECIMAL, and NULL comparison parameters keep the 4N kind payload, which is explicitly accepted from v12 onward. Therefore, while the deployment rollout gate is still v29, a new coordinator can build and remotely dispatch a plan containing function 577 to an old CN; a valid existing prepared JSON equality query then fails depending on remote placement.

Please gate creation or remote dispatch of every plan containing this new function on MORPC v30, with a semantics-preserving fallback (or force compatible local execution) below v30. Add a mixed-version remote-expression regression using at least BOOL and string parameters; the existing exact-numeric metadata test covers only the 12*N half of the contract.

The concrete numeric-type fix itself closes the previous BIGINT-overflow and FLOAT32-rounding blockers. I also exercised typed NULL, repeated FLOAT→SIGNED→FLOAT execution of the same prepared statement, and FLOAT(M,D) parity through the embedded SQL path; those passed. No additional resource ownership, wait/goroutine, unbounded-state, or material hot-path blocker was found. I did not wait for the still-running CI.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

kind/bug Something isn't working size/XXL Denotes a PR that changes 2000+ lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants