github-actions[bot] commented on code in PR #67476:
URL: https://github.com/apache/doris/pull/67476#discussion_r3922454751
##########
be/src/exprs/function/cast/cast_to_variant.h:
##########
@@ -67,11 +69,32 @@ inline Status cast_from_variant_impl(FunctionContext*
context, Block& block,
// if the root of this variant column is a number column, converting it to
a number column
// is acceptable. However, if the destination type is a string and root is
none scalar root, then
// we should convert the entire tree to a string.
- bool is_root_valuable = variant->is_scalar_variant() ||
- (!variant->is_null_root() &&
- variant->get_root_type()->get_primitive_type() !=
INVALID_TYPE &&
-
!is_string_type(data_type_to->get_primitive_type()) &&
- data_type_to->get_primitive_type() != TYPE_JSONB);
+ // A scalar variant is only worth converting through its root when the
root actually carries a
+ // value. A column whose rows are all empty JSON objects is still "scalar"
- it simply has no
+ // path, so its root never got a type - and routing it through the root
conversion leaves
+ // nothing to convert and turns every row into NULL, instead of the `{}`
that serializing the
+ // whole tree produces for the very same value in a column that does hold
paths. See #67367.
+ const bool root_carries_value = !variant->is_null_root() &&
+
variant->get_root_type()->get_primitive_type() != INVALID_TYPE;
+ const bool to_string_or_jsonb =
is_string_type(data_type_to->get_primitive_type()) ||
+ data_type_to->get_primitive_type() ==
TYPE_JSONB;
+ bool is_root_valuable =
+ root_carries_value && (variant->is_scalar_variant() ||
!to_string_or_jsonb);
+ if (is_root_valuable && to_string_or_jsonb) {
+ // Once the root is typed, a row that holds an empty JSON object shows
up as a NULL root
+ // inside a variant row that is not itself NULL. Converting such a
root to STRING/JSONB
+ // yields NULL, while serializing the tree renders it as `{}` - which
is what `SELECT
+ // <variant>` returns, and what the same value returns from a column
that also holds
+ // paths. When no row's root holds a value there is nothing for the
root conversion to
+ // convert, so serialize the tree instead. Rows whose root does hold a
value keep the root
+ // conversion, which is what unwraps a JSON string into its text. See
#67367.
+ const auto* nullable_root =
check_and_get_column<ColumnNullable>(*variant->get_root());
+ 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; });
Review 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.
##########
be/src/exprs/function/cast/cast_to_variant.h:
##########
@@ -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);
Review 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.
##########
be/src/core/column/column_variant.cpp:
##########
@@ -1439,6 +1439,18 @@ size_t
ColumnVariant::Subcolumn::serialize_text_json(size_t n, BufferWritable& o
const auto& part_type_serde = data_serdes[i];
if (ind < part.size()) {
+ // A part whose own type is Nothing holds no value, exactly like
the untyped root
+ // handled at the top of this function. The two are not the same
check: an unfinalized
+ // subcolumn keeps one part per source range
(Subcolumn::insert_range_from appends a
+ // new part rather than rewriting the earlier ones), and a later
typed part promotes
+ // the column-level least common type, so the check above stops
covering the rows that
+ // 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) {
Review 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.
--
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]
