akashchamp opened a new pull request, #68461:
URL: https://github.com/apache/doris/pull/68461
### What problem does this PR solve?
Issue Number: close #68074
Related PR: None
Problem Summary:
Insert a JSON boolean into a VARIANT column and read it back as JSON: it
comes back as the
number `1`/`0` instead of `true`/`false`, so `{"b": true}` does not
round-trip.
```sql
CREATE TABLE s (id INT, j VARIANT) DUPLICATE KEY(id)
DISTRIBUTED BY HASH(id) BUCKETS 1 PROPERTIES('replication_num'='1');
INSERT INTO s VALUES (0, '{"b": true}');
SELECT CAST(j AS STRING) FROM s; -- was {"b":1}, now {"b":true}
```
The inferred/stored subcolumn type is already correctly `boolean` (with
`describe_extend_variant_column = true`, `DESC s` reports `j.b boolean`);
only the JSON
*output* was wrong. A path holding only booleans is the common case that
printed wrong:
mixing in any other JSON type (string, array, number) makes the path fall
back to JSONB
storage, whose writer already prints `true`/`false` correctly, which is why
the bug is easy
to miss. Nested booleans, `array<boolean>`, and a boolean path that overflows
`variant_max_subcolumns_count` into the sparse column are all affected the
same way.
Root cause: `ColumnVariant::Subcolumn::serialize_text_json()` reconstructs
each subcolumn's
JSON text by delegating to the inferred type's SerDe. For a BOOLEAN
subcolumn that reaches
`DataTypeNumberSerDe<TYPE_BOOLEAN>::serialize_one_cell_to_json()`, which
groups BOOLEAN with
the integer types (`is_int_or_bool`) and calls `write_number()` on the
underlying `UInt8`,
emitting `1`/`0`. That SerDe method is shared with every other
BOOLEAN-to-JSON caller in the
engine, so it could not simply be changed to always emit `true`/`false`.
A second, independent bypass affects `CAST(j['b'] AS STRING)` specifically:
`element_at` on
a scalar VARIANT root is treated by `cast_from_variant_impl()` as directly
castable, which
for a BOOLEAN root resolves to the ordinary SQL BOOLEAN-to-STRING caster
(`fmt::format_int`,
i.e. `0`/`1`) instead of going through the VARIANT JSON serializer above.
This is why
`CAST(j['b'] AS STRING)` stayed wrong even with only the first fix.
This is not the same defect as #68016, which changes VARIANT's
least-supertype merge so a
path holding both a boolean and a number resolves to JSONB instead of a
numeric type. A
boolean-only path never reaches that merge (its type is already `boolean`,
which is
correct); the defect here is downstream of typing, in how `boolean` is
written out as JSON.
The fix:
- `FormatOptions` already had an unused `is_bool_value_num` switch
documented for exactly
this (collection `true`/`false` vs `0`/`1` rendering) but nothing
consulted it. Wire it
into `DataTypeNumberSerDe<TYPE_BOOLEAN>::serialize_one_cell_to_json()`:
when false, write
the JSON literal `true`/`false`; the default stays `true` (`0`/`1`), so
every other
BOOLEAN-to-JSON caller (MySQL/CSV/text output, collections outside
VARIANT, etc.) keeps its
current behavior.
- `ColumnVariant::Subcolumn::serialize_text_json()` already takes its
`FormatOptions` by
value, so it is the single place every VARIANT JSON reconstruction path
funnels through
(dense, nested, array, doc-value snapshot and sparse-column reconstruction
all call it).
Set `is_bool_value_num = false` on that local copy so all of them request
JSON literals,
without touching the shared default used elsewhere.
- `cast_from_variant_impl()`: when a scalar VARIANT root is BOOLEAN and the
destination is a
string type, reuse the same structured-document JSON reconstruction
(`CastToStringFunction::execute_impl`, via the existing
`execute_on_finalized_input` helper)
instead of falling into the generic per-type wrapper. JSON booleans are
never quoted, so
this produces exactly the same text a full-tree reconstruction would.
### Release note
Fix a VARIANT correctness bug where a JSON boolean stored in a
homogeneous-boolean path
(scalar, nested, `array<boolean>`, or the sparse column) reconstructed as
the JSON number
`1`/`0` instead of the JSON boolean literal `true`/`false`, so `{"b": true}`
did not
round-trip through `CAST(... AS STRING)`, `CAST(j['b'] AS STRING)`, or any
JSON function
reading the reconstructed text. Plain (non-VARIANT) BOOLEAN output is
unchanged.
### Check List (For Author)
- Test <!-- At least one of them must be included. -->
- [x] Regression test
- [ ] Unit Test
- [ ] Manual test (add detailed scripts or steps below)
- [ ] No need to test or manual test. Explain why:
- [ ] This is a refactor/code format and no logic has been changed.
- [ ] Previous test can cover this change.
- [ ] No code files have been changed.
- [ ] Other reason <!-- Add your reason? -->
Added
`regression-test/suites/variant_p0/test_variant_boolean_json_output.groovy`,
covering: the scalar `{"b": true}`/`{"b": false}` round-trip, nested
`{"o":{"b":true}}`,
`array<boolean>` via `json_extract` on each element, `CAST(j['b'] AS
STRING)`
(element_at), `json_extract(CAST(j AS STRING), '$.b')`, the
pre-existing-correct
heterogeneous/JSONB control case (boolean sharing a path with a string),
and a plain
(non-VARIANT) `BOOLEAN` column as a compatibility guard that must keep
printing `1`/`0`.
I could not run this suite or reproduce against a live cluster myself:
this sandbox has
no Doris BE build toolchain (the pinned clang-16 LDB toolchain and
thirdparty libraries
aren't installed) and no running Doris cluster. What I did verify
directly: the exact
reconstruction and cast call paths by reading the source against the
reporter's
reproduction and the repository's own triage-bot analysis (both confirmed
the same root
cause and call chain independently), and `clang-format-16 --style=file -n
--Werror`
(installed for this check) reports no diff on any of the three changed C++
files. Please
run CI / the regression suite to confirm; I'm glad to address any failures
it turns up.
- Behavior changed:
- [ ] No.
- [x] Yes. `CAST(<a VARIANT with a homogeneous-boolean or array<boolean>
path> AS
STRING)` — and any other JSON-text reconstruction of that path,
including through the
sparse column and `element_at` — now emits `true`/`false` instead of
`1`/`0`. Every
other BOOLEAN-to-string/JSON path (plain `BOOLEAN` columns, and
VARIANT paths that were
already JSONB-backed and already printed correctly) is unchanged.
- Does this need documentation?
- [x] No.
- [ ] Yes. <!-- Add document PR link here. eg:
https://github.com/apache/doris-website/pull/1214 -->
### Check List (For Reviewer who merge this PR)
- [ ] Confirm the release note
- [ ] Confirm test cases
- [ ] Confirm document
- [ ] Add branch pick label <!-- Add branch pick label that this PR should
merge into -->
---
This PR targets `branch-4.1` directly rather than `master`, because master's
VARIANT JSON
serialization was already rewritten as part of the in-progress "Variant V2"
work (see
`be/src/core/data_type_serde/data_type_variant_v2_serde.cpp` /
`be/src/exprs/function/parse/variant_string_parse.h`, where
`Printer::write_primitive()`
already emits `true`/`false` for `TRUE_VALUE`/`FALSE_VALUE`), so this
specific defect does
not reproduce there. `branch-4.1` still uses the pre-V2 implementation
(`be/src/core/column/column_variant.cpp` / `data_type_variant_serde.cpp`,
selected via the
`enable_variant_v2` FE config) that this PR fixes, matching how the issue
was reported
(against 4.1.3). `branch-4.2` has the same pre-V2 implementation and is very
likely affected
too, but I've scoped this PR to the reported version; happy to open a
matching PR there if
maintainers want it.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
