github-actions[bot] commented on code in PR #69221:
URL: https://github.com/apache/doris/pull/69221#discussion_r4322231066
##########
be/src/storage/segment/variant/variant_column_writer_impl.cpp:
##########
@@ -596,6 +600,17 @@ Status VariantSubcolumnWriter::_append_v2(const
VariantColumnData& column, size_
column.row_pos, column.row_pos +
num_rows, source->size());
}
DORIS_CHECK(outer_nulls.empty() || outer_nulls.size() == num_rows);
+ if (_v2_builder == nullptr) {
+ RETURN_IF_ERROR(_initialize_v2_builder());
+ if (_running_type != nullptr) {
+ // Continue from the type inferred by the sealed chunks. Appending
a value that the
+ // previous chunks' type cannot hold (for example a bool after
integers) then takes
+ // the builder's own JSONB fallback inside this chunk instead of
being merged away.
+ RETURN_IF_ERROR(_v2_builder->convert_to(_running_type));
Review Comment:
[P1] Preserve value-dependent fallback across chunk boundaries
Seeding an empty builder with `_running_type` does not reproduce promotions
that depend on the sealed values. For an untyped extracted path, fill one chunk
with scale-1 Decimal128 values near the precision-38 maximum, then append
`1e-38` in the next chunk. The empty builder widens successfully to
Decimal128(38,38), but the earlier values overflow that type. At finalization,
`chunk.builder->convert_to(flush_type)` uses `promote(..., true)`, filters
their row IDs, and writes NULLs instead (or fails subsequent path-stat checks).
The uninterrupted builder preserves these values by falling back to JSONB, as
`PreservesRowsWhenInferredDecimalPromotionOverflows` already checks. Evaluate
inferred promotions against retained chunks and use a lossless whole-path
fallback; cast-null filtering should remain limited to explicit storage
coercion.
##########
be/src/storage/segment/variant/v2/variant_path_builder.cpp:
##########
@@ -1038,6 +1038,10 @@ bool cast_introduced_null(const IColumn& source, const
IColumn& target) {
} // namespace
+DataTypePtr variant_path_least_common_type(const DataTypePtr& left, const
DataTypePtr& right) {
+ return path_least_common_type(left, right);
Review Comment:
[P1] Unwrap nullable arrays before merging chunk types
`VariantPathBuilder::type()` returns `Nullable(...)`, so finalization passes
nullable arrays through this wrapper. The internal `path_least_common_type`
checks for `DataTypeArray` directly; those checks miss the nullable wrapper and
delegate to `get_least_supertype_jsonb`, whose recursive decimal merge selects
the first scale. If an untyped extracted path has `[1.23]` (scale 2) in an
earlier chunk and `[1.2345]` (scale 4) in a later chunk, the second builder
correctly widens to scale 4, but this final merge chooses scale 2 and the final
cast changes `[1.2345]` to `[1.23]`. Single-builder inference retains scale 4.
Unwrap the outer nullable types before dispatching so array elements use the
metadata-aware common-type rules, and cover this with a multi-chunk
decimal-array test.
--
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]
