[fix](variant) Keep empty JSON objects intact when reading VARIANT - #67476
[fix](variant) Keep empty JSON objects intact when reading VARIANT#67476morningman wants to merge 2 commits into
Conversation
A VARIANT row that holds an empty JSON object carries no payload at all, so
what makes it read back as `{}` lives in the column's shape rather than in its
data. Two paths lost that, and both returned a different, valid-looking value
for a persisted `{}`.
Over Arrow Flight a `{}` came back as an empty string. The Arrow Flight result
writer re-materializes every block through MutableBlock, one copy more than the
MySQL writer does, and unlike insert_indices_from that copy is not finalized.
An unfinalized Subcolumn keeps one part per source range, and
Subcolumn::insert_range_from appends a new part instead of rewriting the
earlier ones, so a later typed part promotes only the column-level least common
type. The "untyped root serializes as an empty object" rule in
serialize_text_json only consulted that column-level type, so the rows still
sitting in an untyped part fell through to the Nothing serde and rendered as an
empty string. Apply the rule per part as well. This showed up for `SELECT` over
Arrow Flight whenever a sort merged rows from more than one source block: the
rows of the run merged first came back as `""` while the rest came back as
`{}`, which is why it looked like a per-connection or first-read problem.
CAST(VARIANT AS STRING/JSON) returned SQL NULL for the same value whenever the
column held no path at all. Such a column is still a scalar variant - its root
simply never got a type - and is_scalar_variant() short-circuited
is_root_valuable, so the cast took the scalar-root fast path, found nothing to
convert and produced NULL for every row, while the very same value in a column
that also holds paths came back as `{}`. Require the root to carry a value,
fall back to serializing the tree when no row's root does, and let a
STRING/JSONB target serialize the tree before the all-defaults branch turns it
into NULL. Rows whose root does hold a value keep the root conversion, which is
what unwraps a JSON string into its text.
Both fixes are read-path only; nothing about how a VARIANT is stored changes.
Fixes apache#67367
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Aju9eFEiRNAJUozsDjE9aj
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
run buildall |
|
run buildall |
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Request changes: the empty-object preservation goal is clear and the added coverage exercises the reported Arrow/JDBC and full-batch cast paths, but three blocking correctness gaps remain.
- The per-part
INVALID_TYPEcheck also matches array shells overNothing, changing a valid empty array from[]to{}in the mixed-part state this PR targets. - The new root-value scan uses physical payloads outside the logical cast input, so an inactive suffix or an outer-null masked scalar can route a visible empty object through root conversion and turn it into SQL NULL.
- The newly reachable all-default JSONB fallback produces a logical-prefix data column with a full-source null map, causing prefix casts to throw on a nullable-column size mismatch.
Critical-checkpoint conclusions:
- Goal and scope: the intended behavior is well defined and the diff is focused, but the new predicates/executor path are broader than the tested cases.
- Correctness, special conditions, and parallel paths: STRING-family, JSONB, root/tree serialization, READ/WRITE finalization, range/index copies, and Arrow/JDBC materialization were traced; the three inline findings are independently reachable and blocking.
- Lifecycle, ownership, concurrency, and error propagation: private Variant finalization and copied-column lifetimes are sound; no additional ownership, race, lock, cleanup, or unchecked-status issue was found.
- Configuration, compatibility, persistence, transactions, and FE/BE boundaries: this change adds none, and no separate issue was found in those checkpoints.
- Performance: the added root scan is linear in the batch; once bounded to logical rows it is not a separate blocker.
- Tests: the new unit/regression cases cover the primary empty-object scenarios, but miss mixed empty arrays, outer-null retained payloads, inactive prefixes, and prefix JSONB output sizing. No builds or tests were run because the review bundle explicitly prohibited them.
| // still sit in an untyped part. Render those rows as an empty JSON object here too, | ||
| // instead of letting the Nothing serde emit an empty string. See #67367. | ||
| if (get_base_type_of_array(data_types[i])->get_primitive_type() == | ||
| PrimitiveType::INVALID_TYPE) { |
There was a problem hiding this comment.
This also matches Array(Nothing), which is the normal type of an empty array, so the new early return changes [] to {} in an unfinalized mixed-part root. For example, a doc-mode/root-only Variant run containing [] followed by a run containing [1] retains an Array(Nothing) first part while promoting the column LCT; scalar-root serialization reaches this line and now emits {} instead of letting DataTypeArraySerDe preserve the array delimiters. Please restrict this special case to a scalar Nothing part (zero array dimensions) and add the corresponding mixed-copy regression.
There was a problem hiding this comment.
Confirmed and fixed in c2d5878 — this was a real regression, not a hypothetical one.
get_base_type_of_array() strips every array layer, so Nullable(Array(Nullable(Nothing))) reduced to Nullable(Nothing) and matched the new early return. And that type is exactly what an empty array gets: FieldVisitorToScalarType inserts nothing into type_indexes for [], so get_least_supertype_jsonb() over the empty set returns Nothing (get_least_supertype.cpp:258-261), and with num_dimensions == 1 create_array_of_type() builds Nullable(Array(Nullable(Nothing))) (column_variant.cpp:85-97).
The rule now keys off the part's own type, which is precisely "scalar Nothing, zero dimensions": DataTypeNullable::get_primitive_type() forwards to its nested type (data_type_nullable.h:58), DataTypeNothing gives INVALID_TYPE, and DataTypeArray gives TYPE_ARRAY.
Added ColumnVariantEmptyObjectTest.empty_array_part_is_not_an_empty_object for the mixed-copy shape you describe. Against the previous check it fails with
serialize_subcolumn_row(subcolumn, 0)
Which is: "{}" expected "[]"
part type Nullable(Array(Nullable(Nothing))), column type Nullable(Array(Nullable(TINYINT)))
and it passes with this change. All 6 cases in the suite pass.
| if (nullable_root != nullptr) { | ||
| const auto& root_null_map = nullable_root->get_null_map_data(); | ||
| is_root_valuable = std::any_of(root_null_map.begin(), root_null_map.end(), | ||
| [](UInt8 is_null) { return is_null == 0; }); |
There was a problem hiding this comment.
Please scope this test to the logical rows being cast, not every physical root payload. prepare_remove_nullable supplies null_map separately and preserves nested payloads, so CAST(IF(k=1, v, NULL) AS STRING) over v = [{}, 123] counts the masked 123, takes root conversion, and returns NULL for the visible {} row. The same happens when a prepared cast is called with input_rows_count == 1 on a two-row root [NULL, 123], because this scans past the active prefix. Limit the scan to [0, input_rows_count) and require the outer null map (when present) to be clear; cover both STRING and JSONB.
There was a problem hiding this comment.
Scan tightened in c2d5878: it is now bounded by input_rows_count and skips the rows the outer null map masks out.
The outer-null-map half is real — prepare_remove_nullable unnests the argument and hands arg_null_map over separately (function_cast.cpp:206-219), and need_replace_null_data_to_default returns false for STRING/JSONB targets, so the masked rows keep their payloads.
One correction for the record: it was not a regression from this PR. In your v = [{}, 123] example the pre-PR code also returned NULL for the visible {} row, because is_root_valuable was simply is_scalar_variant(). The new any_of can only ever lower is_root_valuable, never raise it. So that case is the mixed-root gap the PR description calls out as deliberately out of scope — but honouring the mask closes this sub-case of it, so it is in.
On the prefix half, I could not find a live caller where input_rows_count < col_from.size() for this path: vcast_expr.cpp:122 and :171 pass temp_block.rows(), the per-row try_cast path at :236 passes a 1-row cut with count 1, prepare_remove_nullable forwards input_rows_count unchanged over same-size unnested columns, and const arguments go through a temporary block sized by its own rows(). The variant is also clone_finalized() before this point, so the root's size equals the column's. Bounding the scan costs nothing either way, so it is bounded.
| @@ -125,6 +148,9 @@ inline Status cast_from_variant_impl(FunctionContext* context, Block& block, | |||
| return cast_from_generic_to_jsonb(context, finalized_block, arguments, result, | |||
| input_rows_count); | |||
There was a problem hiding this comment.
This newly activates cast_from_generic_to_jsonb for all-default Variant roots, but that helper sizes its null map with the full col_from.size() while producing only input_rows_count nested values. A prepared JSONB cast of a two-row [{}, {}] source with input_rows_count == 1 therefore throws when ColumnNullable checks the 1-vs-2 sizes; all roots are null, so limiting the scan above does not address it. Please make the JSONB fallback obey the logical row count (or pass it a prefix-cut column) and cover this prefix case.
There was a problem hiding this comment.
Deliberately not addressed in this PR: the mechanism is real, but it predates this change and I could not find a reachable path to it.
ColumnUInt8::create(col_from.size(), 0) against a loop over input_rows_count has been there since #50940 (bd3c453, 2025-07-17), and cast_from_generic_to_jsonb is the shared generic-to-JSONB path for array/map/struct as well. If input_rows_count < col_from.size() were reachable, CAST(<array> AS JSON) would already throw today, independently of variant.
Reachability is the same question as the prefix half of the sibling comment, and the answer is the same: every cast entry point passes the full row count (vcast_expr.cpp:122, :171), the per-row try_cast path passes a 1-row cut with count 1 (:236), prepare_remove_nullable forwards the count unchanged over same-size columns, and const arguments run through a temporary block sized by its own rows().
If you would like it hardened regardless, ColumnUInt8::create(input_rows_count, 0) is a no-op whenever the sizes agree. I would rather send that as a separate PR against the shared helper than widen this one, since it changes a function every generic-to-JSONB cast goes through. Happy to do that if a maintainer prefers.
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
TPC-H: Total hot run time: 16874 ms |
TPC-DS: Total hot run time: 82232 ms |
ClickBench: Total hot run time: 14.54 s |
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
…ect rule
Two follow-ups on the per-part empty-object rule and the root-value scan.
The per-part rule keyed off get_base_type_of_array(), which strips every array
layer, so it also matched Array(Nothing) - the type an empty array gets, since
an empty array carries no element type. That is not an absent value the way an
untyped part is: it is `[]`, and the array serde renders it. In an unfinalized
subcolumn whose later part promotes the column-level least common type past
Array(Nothing), a `[]` sitting in the unpromoted part came back as `{}`. Key
the rule off the part's own type instead, which is exactly "scalar Nothing".
The root-value scan walked the whole root null map. Only the rows this call
converts should have a say: prepare_remove_nullable hands the outer null map
over as a separate argument and leaves the masked rows' payloads in place, so a
masked row whose root carries a value kept every visible row on the root
conversion. Bound the scan by input_rows_count and skip masked rows.
ColumnVariantEmptyObjectTest.empty_array_part_is_not_an_empty_object covers the
first one: it fails on the previous check with
`part type Nullable(Array(Nullable(Nothing))), column type
Nullable(Array(Nullable(TINYINT)))` returning `{}` instead of `[]`, and passes
with this change. All 6 cases in the suite pass.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FE4hQXyTkfCbqkMGbokU6e
|
run buildall |
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
3 participants
What problem does this PR solve?
Issue Number: close #67367
Related PR: #65615 (Arrow Flight SQL tracking issue)
Problem Summary:
A
VARIANTrow that holds an empty JSON object carries no payload at all, so what makes it read back as{}lives in the column's shape rather than in its data. Two read paths lost that and returned a different, valid-looking value for a persisted{}.1. Over Arrow Flight a
{}came back as an empty string.The Arrow Flight result writer re-materializes every block through
MutableBlock(varrow_flight_result_writer.cpp), one copy more than the MySQL writer does, and unlikeinsert_indices_fromthat copy is not finalized. An unfinalizedSubcolumnkeeps one part per source range, andSubcolumn::insert_range_fromappends a new part instead of rewriting the earlier ones, so a later typed part promotes only the column-level least common type:The "untyped root serializes as an empty object" rule in
Subcolumn::serialize_text_jsononly consulted that column-level type, so the rows still sitting in an untyped part fell through to theNothingserde and rendered as an empty string. The fix applies the same rule per part.Because the trigger is the block shape (a sort merging rows from more than one source block), not the connection, the original report looked like a per-connection or first-read problem. It is neither: the same connection returns
{}for a query shape that does not merge runs, and a fresh connection returns""for one that does.2.
CAST(VARIANT AS STRING/JSON)returned SQLNULLfor the same value whenever the column held no path at all — on both protocols:Such a column is still a scalar variant — its root simply never got a type — and
is_scalar_variant()short-circuitedis_root_valuable, so the cast took the scalar-root fast path, found nothing to convert and producedNULLfor every row. ANOT NULLcolumn returnedNULLthis way too. The fix requires the root to carry a value, falls back to serializing the tree when no row's root does, and lets a STRING/JSONB target serialize the tree before the all-defaults branch turns it intoNULL. Rows whose root does hold a value keep the root conversion, which is what unwraps a JSON string into its text.Both fixes are read-path only; nothing about how a
VARIANTis stored changes.Known remaining gap, deliberately out of scope: in a column that mixes
{}with scalar roots (e.g.123),CASTof the{}rows still returnsNULL. Closing that needs a row-level substitution in the root fast path, which would change the existing JSON-string unwrapping contract thatvariant_p0/column_namedepends on.Release note
Fix
VARIANTempty JSON objects being returned as an empty string over Arrow Flight SQL, and as SQLNULLbyCAST(VARIANT AS STRING/JSON)when the column holds no path.Check List (For Author)
New coverage, both verified to fail without this change and pass with it:
be/test/core/column/column_variant_test.cpp— 5 cases underColumnVariantEmptyObjectTest.empty_object_survives_copy_of_mixed_type_partsreproduces the exact shape from the log above and fails on rows 0-2 without the fix. Note for anyone extending these: the source parts must disagree on type (oneREAD_MODE, oneWRITE_MODEfinalize) and the copy must be left unfinalized, otherwise the test passes either way.regression-test/suites/variant_p0/test_variant_empty_object_cast.groovy— the cast, across the all-empty / with-NULL / mixed shapes, plus an assertion that both shapes agree.regression-test/suites/arrow_flight_sql_p0/test_select_variant.groovy— the sorted multi-block read over Arrow Flight, with a JDBC-vs-ArrowassertEqualsso the two protocols must return the same rows.Manual: reproduced and re-verified end to end against a local single FE + single BE cluster through both the MySQL protocol and the Python
adbc_driver_flightsqlclient from the original report.Regression sweep of
variant_p0+arrow_flight_sql_p0(174 suites). The remaining failures were each confirmed to reproduce on a pristine build of the same commit without this change:test_all_prdefine_type_to_sparse(decimal256 precision),test_outfile_csv_variant_type(S3curlCode: 43),test_sql_cache_over_arrow_flight(FE-sideIllegalStateException),test_variant_compaction_with_sparse_limit(array rendering spacing).Behavior changed:
{}instead of an empty string over Arrow Flight, andCAST(VARIANT AS STRING/JSON)on a column that holds no path returns{}instead ofNULL. Both make the value agree with whatSELECT <variant>and every other shape of the same column already returned.Does this need documentation?
🤖 Generated with Claude Code
https://claude.ai/code/session_01Aju9eFEiRNAJUozsDjE9aj