github-actions[bot] commented on code in PR #65975:
URL: https://github.com/apache/doris/pull/65975#discussion_r3655195606
##########
be/src/exec/common/hash_table/hash_map_context.h:
##########
@@ -375,19 +375,28 @@ struct MethodStringNoCache : public MethodBase<TData> {
}
void init_serialized_keys_impl(const ColumnRawPtrs& key_columns, uint32_t
num_rows,
- DorisVector<StringRef>& stored_keys) {
+ DorisVector<StringRef>& stored_keys, const
uint8_t* null_map) {
const IColumn& column = *key_columns[0];
const auto& nested_column =
is_column_nullable(column)
? assert_cast<const
ColumnNullable&>(column).get_nested_column()
: column;
- auto serialized_str = [](const auto& column_string,
DorisVector<StringRef>& stored_keys) {
+ // For join, rows with null keys are routed to a dedicated null bucket
and matched by
+ // raw key comparison, so their keys must be normalized to a canonical
empty StringRef
+ // to make null keys equal (e.g. single-column null-safe equal join).
The nested
+ // column of a null row may hold residual bytes left by expression
evaluation.
+ // Real empty strings are not affected: they are not null, so they are
still hashed
+ // into normal buckets and never meet the null keys in the null bucket.
+ auto serialized_str = [null_map](const auto& column_string,
+ DorisVector<StringRef>& stored_keys) {
const auto& offsets = column_string.get_offsets();
const auto* chars = column_string.get_chars().data();
stored_keys.resize(column_string.size());
for (size_t row = 0; row < column_string.size(); row++) {
- stored_keys[row] =
- StringRef(chars + offsets[row - 1], offsets[row] -
offsets[row - 1]);
+ stored_keys[row] = (null_map != nullptr && null_map[row])
+ ? StringRef()
Review Comment:
`StringRef()` leaves `data == nullptr`, but a retained single-key null-safe
bucket eventually compares the build/probe refs through `StringRef::eq`, which
unconditionally calls `memcmp(data, other.data, size)`. For `NULL <=> NULL`,
that becomes `memcmp(nullptr, nullptr, 0)`; `memcmp` requires pointers to
objects (and libc headers declare these arguments nonnull), so this new
path—and the line 150 unit assertion that compares against another default
ref—has undefined behavior. Please use valid canonical backing storage and
adjust the assertion to avoid a default null pointer, or short-circuit
zero-length equality before `memcmp`.
```suggestion
? StringRef("", 0)
```
--
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]
