github-actions[bot] commented on code in PR #64407:
URL: https://github.com/apache/doris/pull/64407#discussion_r3396008358
##########
be/src/exprs/function/dictionary_util.h:
##########
@@ -43,9 +43,8 @@ Status inline check_dict_input_data(ColumnsWithTypeAndName&
key_data,
IColumn::Filter filter(key_data.front().column->size(), 1);
for (auto& key : key_data) {
- if (key.column->is_nullable()) {
- const auto& null_map = assert_cast<const
ColumnNullable*>(key.column.get())
- ->get_null_map_data(); // Get the
null_map in the key
+ if (const auto* nullable_key =
check_and_get_column<ColumnNullable>(key.column.get())) {
Review Comment:
This misses const-nullable key columns when `skip_null_key` is enabled. A
key expression such as a nullable literal/cast can arrive as
`ColumnConst(ColumnNullable)`: this physical check is false, then
`remove_nullable(key.column)` unwraps it to `ColumnConst(nested, size)` and the
filter remains all ones, so rows with NULL keys are kept, or can become
duplicate/default keys, instead of being skipped. Please use a const-aware
null-map path, such as `VectorizedUtils::get_null_map()` with const handling,
before removing nullability.
##########
be/src/exprs/function/dictionary.cpp:
##########
@@ -120,7 +120,7 @@ void IDictionary::load_values(const std::vector<ColumnPtr>&
values_column) {
column_with_type.column =
cast_to_column<Type>(value_column_without_nullable);
// if original value is nullable, the null_map must be
not null
- if (values_column[i]->is_nullable()) {
+ if (is_column_nullable(*values_column[i])) {
Review Comment:
This now lets `ColumnConst(ColumnNullable)` attribute values pass the type
check above, because `values_column[i]->is_nullable()` is semantic after this
PR. But this branch only preserves the null map for a physical
`ColumnNullable`; for `Const(Nullable)` it falls through while
`value_column_without_nullable = remove_nullable(values_column[i])` unwraps the
const nullable to the nested default value. A dictionary load with a constant
NULL nullable attribute will therefore store that default as a non-null value.
Please handle `Const(Nullable)` here, for example by using the const-aware
null-map helper and preserving the single-value null map semantics, or by
materializing before loading.
--
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]
