github-actions[bot] commented on code in PR #67981:
URL: https://github.com/apache/doris/pull/67981#discussion_r4014119734
##########
be/src/exprs/function/cast/cast_base.cpp:
##########
@@ -17,11 +17,70 @@
#include "exprs/function/cast/cast_base.h"
+#include <algorithm>
#include <cstdint>
+#include <utility>
#include "util/jsonb_writer.h"
namespace doris::CastWrapper {
+namespace {
+
+/// A nullable column keeps an all zero NULL map when it has no NULL row, and
there is nothing to
+/// inherit from such a column.
+bool has_masked_row(const NullMap::value_type* null_map, size_t rows) {
+ return std::any_of(null_map, null_map + rows,
+ [](const NullMap::value_type value) { return value !=
0; });
+}
+
+} // namespace
+
+ChildNullMask build_child_null_mask(const NullMap::value_type* parent_null_map,
+ const IColumn::Offsets64* offsets, const
ColumnPtr& child) {
+ if (parent_null_map == nullptr) {
+ return {.column = child, .null_map = nullptr, .mask_holder = nullptr};
+ }
+ const size_t rows = offsets == nullptr ? child->size() : offsets->size();
+ if (!has_masked_row(parent_null_map, rows)) {
+ return {.column = child, .null_map = nullptr, .mask_holder = nullptr};
+ }
+
+ auto mask = ColumnUInt8::create(child->size(), 0);
+ auto& mask_data = mask->get_data();
+ if (offsets == nullptr) {
+ // The children of a STRUCT share the rows of their parent.
+ std::copy_n(parent_null_map, rows, mask_data.begin());
+ } else {
+ // ARRAY and MAP children are flattened, so the NULL of a row has to
be expanded to every
+ // element/entry that belongs to it.
+ for (size_t row = 0; row < rows; ++row) {
+ if (parent_null_map[row] == 0) {
+ continue;
+ }
+ const size_t first_child = row == 0 ? 0 : (*offsets)[row - 1];
+ std::fill(mask_data.begin() + first_child, mask_data.begin() +
(*offsets)[row], 1);
+ }
+ }
+
+ const auto* nullable_child =
check_and_get_column<ColumnNullable>(child.get());
+ if (nullable_child == nullptr) {
+ return {.column = child, .null_map = mask_data.data(), .mask_holder =
std::move(mask)};
+ }
+
+ // The NULL state of a child is the union of its own NULL map and the mask
inherited from the
+ // rows of its parent.
+ auto merged_mask = ColumnUInt8::create(child->size(), 0);
Review Comment:
[P2] Merge the child NULL map into the first buffer
`mask` already has `child->size()` entries and contains all inherited parent
bits, but this branch allocates and zeroes another equally large `merged_mask`,
ORs every byte into it, and then immediately frees `mask`. ARRAY children
always take this nullable branch, and their flattened element count can be much
larger than the row count; MAP and recursive complex casts repeat the cost.
Please OR `child_null_map` into `mask_data` in place and move `mask` into the
returned `ColumnNullable`, avoiding the extra child-cardinality allocation and
initialization.
##########
be/src/exprs/function/cast/cast_to_int.h:
##########
@@ -72,6 +72,11 @@ class CastToImpl<CastMode, FromDataType, ToDataType> :
public CastToBase {
CastParameters params;
params.is_strict = (CastMode == CastModeType::StrictMode);
for (size_t i = 0; i < input_rows_count; ++i) {
+ // The source value of a row marked as null by the input null map
is a hidden
+ // payload and has no SQL semantics, so it must not be checked.
+ if (null_map && null_map[i]) {
+ continue;
Review Comment:
[P1] Initialize the destination before skipping masked rows
`ColumnVector(input_rows_count)` is backed by `PODArray(size)`, which
allocates and advances its end pointer without filling elements, so this
`continue` bypasses the only write to `vec_to[i]`. The outer NULL map does not
guarantee that payload is never read: a chained `CAST(CAST(IF(p, x, NULL) AS
TINYINT) AS STRING)` unwraps the nullable column and
`CastToStringFunction::execute_impl` formats every nested row while ignoring
its `null_map`, reading the indeterminate TINYINT slot before the final NULL
map is restored. The same issue applies to the newly skipped decimal and strict
date/time/TIMESTAMP_NS destinations. Please write each masked slot's valid
default (or value-initialize the target column) before continuing, and cover a
chained nullable consumer.
##########
be/src/exprs/function/cast/cast_to_struct.h:
##########
@@ -68,21 +68,25 @@ WrapperType create_struct_wrapper(FunctionContext* context,
const DataTypePtr& f
size_t elements_num = to_element_types.size();
Columns converted_columns(elements_num);
for (size_t i = 0; i < elements_num; ++i) {
- ColumnWithTypeAndName from_element_column
{from_col_struct->get_column_ptr(i),
- from_element_types[i],
""};
+ /// A field of a row that is NULL is a hidden payload as well.
Fields share the rows of
+ /// their parent, so the mask of the parent can be used as is.
+ auto child_mask =
Review Comment:
[P2] Skip inherited-mask work for unchanged fields
This builds a child mask before knowing whether the prepared child
conversion can use it. For an exact-type field, `prepare_impl` ends at
`create_identity_wrapper`, so a wide `STRUCT` with only one changed field still
rescans the same parent NULL map for every unchanged field and, if any row is
NULL, allocates a row-sized mask for each one. The same happens to an unchanged
MAP key, for example `MAP<INT,INT>` to `MAP<INT,TINYINT>`. These identity
children cannot validate or reject their hidden payload, and the enclosing NULL
already hides it. Please retain whether each child wrapper is an identity and
forward exact-type children directly; also compute the parent masked state once
per MAP/STRUCT invocation.
--
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]
