github-actions[bot] commented on code in PR #67981:
URL: https://github.com/apache/doris/pull/67981#discussion_r4011843501
##########
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]) {
Review Comment:
[P1] Propagate ancestor NULLs into complex children
This loop's `null_map` is only safe when it is aligned with this scalar
child. For `CAST(IF(p, ARRAY(x), NULL) AS ARRAY<TINYINT>)` with
`(p=false,x=128)`, `IF` retains `ARRAY(128)` under an outer NULL.
`create_array_wrapper` forwards that parent map, but the element's
`prepare_remove_nullable` replaces it with the element's own zero null bit, so
this loop still validates 128 and fails the query. MAP and nullable-STRUCT
children have the same masking gap; a non-nullable flattened MAP child can
additionally index the row-sized parent map using an entry index. Please expand
ARRAY/MAP ancestor masks through offsets, merge inherited masks into children,
and cover these complex NULL cases.
##########
be/src/exprs/function/cast/cast_to_date.h:
##########
@@ -97,19 +97,45 @@ class CastToImpl<CastMode, FromDataType, ToDataType> :
public CastToBase {
if constexpr (CastMode == CastModeType::StrictMode) {
MutableColumnPtr column_to = nested_to_type->create_column();
// WON'T write nulls to the result column, just raise errors.
null_map is only used to skip invalid rows
+ Status st;
if constexpr (IsDataTypeInt<FromDataType>) {
- RETURN_IF_ERROR(concrete_serde->template
from_int_strict_mode_batch<FromDataType>(
- *col_from, *column_to));
+ st = concrete_serde->template
from_int_strict_mode_batch<FromDataType>(*col_from,
+
*column_to);
} else if constexpr (IsDataTypeFloat<FromDataType>) {
- RETURN_IF_ERROR(concrete_serde->template
from_float_strict_mode_batch<FromDataType>(
- *col_from, *column_to));
+ st = concrete_serde->template
from_float_strict_mode_batch<FromDataType>(
+ *col_from, *column_to);
} else {
static_assert(IsDataTypeDecimal<FromDataType>);
- RETURN_IF_ERROR(
- concrete_serde->template
from_decimal_strict_mode_batch<FromDataType>(
- *col_from, *column_to));
+ st = concrete_serde->template
from_decimal_strict_mode_batch<FromDataType>(
+ *col_from, *column_to);
+ }
+ if (st.ok() || null_map == nullptr) {
+ RETURN_IF_ERROR(st);
+ block.get_by_position(result).column = std::move(column_to);
+ } else {
+ // The strict-mode serde batch cannot skip rows, so the
failure may come from the
+ // hidden payload of a row that the input null map marks as
NULL. Cast tolerantly
+ // and only report an error if a visible (non NULL) row
failed; NULL rows are NULL
+ // in the final result anyway.
+ auto nullable_col_to =
create_empty_nullable_column(nested_to_type);
+ if constexpr (IsDataTypeInt<FromDataType>) {
+ RETURN_IF_ERROR(concrete_serde->template
from_int_batch<FromDataType>(
+ *col_from, *nullable_col_to));
+ } else if constexpr (IsDataTypeFloat<FromDataType>) {
+ RETURN_IF_ERROR(concrete_serde->template
from_float_batch<FromDataType>(
+ *col_from, *nullable_col_to));
+ } else {
+ RETURN_IF_ERROR(concrete_serde->template
from_decimal_batch<FromDataType>(
+ *col_from, *nullable_col_to));
+ }
+ const auto& result_null_map =
nullable_col_to->get_null_map_data();
+ for (size_t i = 0; i < col_from->size(); ++i) {
+ if (null_map[i] == 0 && result_null_map[i] != 0) {
+ return st;
Review Comment:
[P2] Return the visible row's conversion error
If a hidden invalid value precedes a visible invalid one, `st` still belongs
to the hidden row. For values `{1000, 7777777}` with null map `{1, 0}`, the
strict DATEV2 batch saves the error for 1000; the tolerant bitmap then finds
row 1 invalid, but this returns the 1000 error. That payload has no SQL
semantics and the actual offender is 7777777; the new negative test misses this
by checking only `!ok()`. Please make the strict batch skip masked rows or
construct the Status from the detected visible row; the timestamp-ns branch has
the same issue.
##########
be/src/exprs/function/cast/cast_to_date.h:
##########
@@ -97,19 +97,45 @@ class CastToImpl<CastMode, FromDataType, ToDataType> :
public CastToBase {
if constexpr (CastMode == CastModeType::StrictMode) {
MutableColumnPtr column_to = nested_to_type->create_column();
// WON'T write nulls to the result column, just raise errors.
null_map is only used to skip invalid rows
+ Status st;
if constexpr (IsDataTypeInt<FromDataType>) {
- RETURN_IF_ERROR(concrete_serde->template
from_int_strict_mode_batch<FromDataType>(
- *col_from, *column_to));
+ st = concrete_serde->template
from_int_strict_mode_batch<FromDataType>(*col_from,
+
*column_to);
} else if constexpr (IsDataTypeFloat<FromDataType>) {
- RETURN_IF_ERROR(concrete_serde->template
from_float_strict_mode_batch<FromDataType>(
- *col_from, *column_to));
+ st = concrete_serde->template
from_float_strict_mode_batch<FromDataType>(
+ *col_from, *column_to);
} else {
static_assert(IsDataTypeDecimal<FromDataType>);
- RETURN_IF_ERROR(
- concrete_serde->template
from_decimal_strict_mode_batch<FromDataType>(
- *col_from, *column_to));
+ st = concrete_serde->template
from_decimal_strict_mode_batch<FromDataType>(
+ *col_from, *column_to);
+ }
+ if (st.ok() || null_map == nullptr) {
+ RETURN_IF_ERROR(st);
+ block.get_by_position(result).column = std::move(column_to);
+ } else {
+ // The strict-mode serde batch cannot skip rows, so the
failure may come from the
+ // hidden payload of a row that the input null map marks as
NULL. Cast tolerantly
+ // and only report an error if a visible (non NULL) row
failed; NULL rows are NULL
+ // in the final result anyway.
+ auto nullable_col_to =
create_empty_nullable_column(nested_to_type);
Review Comment:
[P2] Avoid retrying the entire batch for ordinary NULLs
Zero is the normal nested payload for a NULL numeric value, and
DATE/DATETIME/DATEV2/DATETIMEV2/TIMESTAMP_NS all reject it. A batch containing
such a NULL therefore allocates/resizes the strict result and converts up to
that row, then allocates another full nullable result here, reconverts all
rows, and scans the full bitmap before succeeding; a late NULL nearly doubles
conversion work. Please make the strict numeric SerDe batches accept the row
null map and skip masked rows in one pass, as the strict string batches already
do; that also keeps errors tied to visible rows.
--
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]
