github-actions[bot] commented on code in PR #64407:
URL: https://github.com/apache/doris/pull/64407#discussion_r3394471814
##########
be/src/exprs/function/function_jsonb.cpp:
##########
@@ -216,13 +216,11 @@ class FunctionJsonbParseBase : public IFunction {
}
const ColumnString* col_from_string = nullptr;
- if (col_from->is_nullable()) {
- const auto& nullable_col = assert_cast<const
ColumnNullable&>(*col_from);
-
+ if (const auto* nullable_col =
check_and_get_column<ColumnNullable>(col_from.get())) {
VectorizedUtils::update_null_map(null_map->get_data(),
- nullable_col.get_null_map_data());
+
nullable_col->get_null_map_data());
Review Comment:
When the first argument is `ColumnConst(ColumnNullable)` but the fallback
JSONB value is row-varying, `unpack_if_const` leaves `col_from` as the one-row
nested `ColumnNullable` and sets `col_from_is_const = true`. This call still
merges that null map as if it had `input_rows_count` entries, so rows after 0
read past the const null map or copy unrelated bytes. Please pass the const
flag here, as the jsonb path extractor does below, or materialize before
merging.
```suggestion
nullable_col->get_null_map_data(), col_from_is_const);
```
##########
be/src/core/column/column_const.h:
##########
@@ -270,8 +270,7 @@ class ColumnConst final : public COWHelper<IColumn,
ColumnConst> {
return false;
}
- // ColumnConst is not nullable, but may be concrete nullable.
- bool is_concrete_nullable() const override { return
is_column_nullable(*data); }
+ bool is_nullable() const override { return is_column_nullable(*data); }
Review Comment:
This changes the global contract of `IColumn::is_nullable()`, but not all
callers that use it as a physical `ColumnNullable` test have been migrated. One
concrete existing path is
`be/src/exprs/function/functions_multi_string_position.cpp`: with a vector
haystack and a const nullable needles array, `needles_column->is_nullable()` is
now true, `remove_nullable(needles_column)` lets execution proceed through the
const-array path, and the final nullable masking does `assert_cast<const
ColumnNullable*>(needles_column.get())` even though the object is still
`ColumnConst`. That turns this semantic change into a crash/invalid cast for
mixed vector plus const-nullable arguments. Please keep `is_nullable()`
physical and introduce a separate semantic predicate, or finish
auditing/migrating the remaining callers before flipping this method.
--
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]
