Copilot commented on code in PR #61287:
URL: https://github.com/apache/doris/pull/61287#discussion_r2924346086
##########
be/src/vec/common/hash_table/hash_map_context.h:
##########
@@ -497,47 +559,56 @@ struct MethodKeysFixed : public MethodBase<TData> {
template <typename T>
void pack_fixeds(size_t row_numbers, const ColumnRawPtrs& key_columns,
const ColumnRawPtrs& nullmap_columns, DorisVector<T>&
result) {
- size_t bitmap_size = get_bitmap_size(nullmap_columns.size());
- // set size to 0 at first, then use resize to call default constructor
on index included from [0, row_numbers) to reset all memory
- result.clear();
+ size_t bitmap_size = nullmap_columns.empty() ? 0 : 1;
+ if (bitmap_size) {
+ // set size to 0 at first, then use resize to call default
constructor on index included from [0, row_numbers) to reset all memory
+ // only need to reset the memory used to bitmap
+ result.clear();
+ }
result.resize(row_numbers);
+ auto* __restrict result_data = reinterpret_cast<char*>(result.data());
+
size_t offset = 0;
+ std::vector<bool> has_null_column(nullmap_columns.size(), false);
if (bitmap_size > 0) {
+ std::vector<const uint8_t*> nullmap_datas;
+ std::vector<uint8_t> bit_offsets;
for (size_t j = 0; j < nullmap_columns.size(); j++) {
if (!nullmap_columns[j]) {
continue;
}
- size_t bucket = j / BITSIZE;
- size_t local_offset = j % BITSIZE;
- const auto& data =
+ const uint8_t* __restrict data =
assert_cast<const
ColumnUInt8&>(*nullmap_columns[j]).get_data().data();
- for (size_t i = 0; i < row_numbers; ++i) {
- *((char*)(&result[i]) + bucket) |= data[i] << local_offset;
+
+ has_null_column[j] = simd::contain_one(data, row_numbers);
+ if (has_null_column[j]) {
+ nullmap_datas.emplace_back(data);
+ bit_offsets.emplace_back(j);
}
}
+ constexpr_int_match<1, BITSIZE, PackNullmapsReducer>::run(
+ int(nullmap_datas.size()), nullmap_datas.data(),
bit_offsets.data(),
+ row_numbers, sizeof(T),
reinterpret_cast<uint8_t*>(result_data));
offset += bitmap_size;
}
for (size_t j = 0; j < key_columns.size(); ++j) {
- const char* data = key_columns[j]->get_raw_data().data;
+ const char* __restrict data = key_columns[j]->get_raw_data().data;
Review Comment:
This introduces two correctness risks: (1) `assume_mutable()` can create a
new column instance when the original is shared; the returned mutable is not
stored back into `key_columns`, so the null replacement may have no lasting
effect. (2) Even if it mutates in-place, you captured `data` before the
mutation; if `assume_mutable()` performs a clone/reallocation, `data` can
become stale. Safer options: avoid mutating `key_columns` here (restore the
previous per-row conditional copy of zero/default for nulls), or
store/propagate the mutable column back and re-fetch `data` after mutation.
##########
be/src/vec/common/hash_table/hash_map_context.h:
##########
@@ -497,47 +559,56 @@ struct MethodKeysFixed : public MethodBase<TData> {
template <typename T>
void pack_fixeds(size_t row_numbers, const ColumnRawPtrs& key_columns,
const ColumnRawPtrs& nullmap_columns, DorisVector<T>&
result) {
- size_t bitmap_size = get_bitmap_size(nullmap_columns.size());
- // set size to 0 at first, then use resize to call default constructor
on index included from [0, row_numbers) to reset all memory
- result.clear();
+ size_t bitmap_size = nullmap_columns.empty() ? 0 : 1;
+ if (bitmap_size) {
+ // set size to 0 at first, then use resize to call default
constructor on index included from [0, row_numbers) to reset all memory
+ // only need to reset the memory used to bitmap
+ result.clear();
+ }
result.resize(row_numbers);
Review Comment:
When `bitmap_size == 0`, the code no longer clears/value-initializes
`result` before writing key bytes. If `sizeof(T)` is larger than the total
bytes written by `pack_fixeds` (common when `key_byte_size < sizeof(T)`),
padding/unused bytes can retain stale data and corrupt hash/equality behavior.
Fix by ensuring the entire `result` buffer is zero-initialized for all rows
(e.g., unconditional `clear()+resize()`, or an explicit memset over
`row_numbers * sizeof(T)` before packing).
##########
fe/fe-core/src/main/java/org/apache/doris/qe/SessionVariable.java:
##########
@@ -1216,7 +1216,7 @@ public void checkQuerySlotCount(String slotCnt) {
// 4096 minus 16 + 16 bytes padding that in padding pod array
Review Comment:
The comment describing the `batchSize` default (\"4096 minus 16 + 16...\")
no longer matches the new default value `8160`. Update the comment to reflect
the new sizing rationale/constraints, or adjust the value if the intent is
still a ~4K payload.
```suggestion
// 8160 + 16 + 16 = 8192
```
##########
be/src/vec/common/hash_table/hash_map_context.h:
##########
@@ -476,6 +475,69 @@ struct MethodOneNumberDirect : public
MethodOneNumber<FieldType, TData> {
}
};
+template <int N>
+void pack_nullmaps_interleaved(const uint8_t* const* datas, const uint8_t*
bit_offsets,
+ size_t row_numbers, size_t stride, uint8_t*
__restrict out) {
Review Comment:
The fixed-key nullmap packing logic was substantially changed (interleaved
packing + skipping columns without nulls). Add focused unit tests to cover: (1)
packing correctness for multiple nullable columns (including sparse nulls), (2)
behavior when some nullable columns have no nulls (should not break bit
positions), and (3) ensuring packed keys are deterministic for null rows (no
dependence on original underlying value bytes).
##########
be/src/vec/columns/column_decimal.cpp:
##########
@@ -486,10 +486,6 @@ void ColumnDecimal<T>::compare_internal(size_t rhs_row_id,
const IColumn& rhs,
template <PrimitiveType T>
void ColumnDecimal<T>::replace_column_null_data(const uint8_t* __restrict
null_map) {
auto s = size();
Review Comment:
Same as `ColumnVector::replace_column_null_data`: removing the no-nulls
early return can introduce a full scan/write even when `null_map` contains no
`1`s. Consider adding a fast-path guard (e.g., `if
(!simd::contain_one(null_map, s)) return;`) to avoid unnecessary work in hot
paths.
```suggestion
auto s = size();
if (!simd::contain_one(null_map, s)) {
return;
}
```
##########
be/src/vec/common/hash_table/hash_key_type.h:
##########
@@ -94,8 +98,7 @@ inline HashKeyType get_hash_key_type_fixed(const
std::vector<vectorized::DataTyp
}
}
- size_t bitmap_size = has_null ?
vectorized::get_bitmap_size(data_types.size()) : 0;
- return get_hash_key_type_with_fixed(bitmap_size + key_byte_size);
+ return get_hash_key_type_with_fixed(has_null + key_byte_size);
Review Comment:
The `>= vectorized::BITSIZE` condition forces exactly `BITSIZE` keys to use
`serialized`, even though other parts of this PR (e.g., bitmap stored in 1 byte
and checks like `<= BITSIZE`) still appear to support up to `BITSIZE` keys. If
`BITSIZE` is 8 (as implied by the 1-byte bitmap), this likely regresses the
fixed-key path for 8 keys. Consider changing the threshold to `>
vectorized::BITSIZE` (or aligning the other checks/packing logic to match this
new restriction).
##########
be/src/vec/common/hash_table/hash_key_type.h:
##########
@@ -80,6 +80,10 @@ inline HashKeyType get_hash_key_type_with_fixed(size_t size)
{
}
inline HashKeyType get_hash_key_type_fixed(const
std::vector<vectorized::DataTypePtr>& data_types) {
+ if (data_types.size() >= vectorized::BITSIZE) {
Review Comment:
The `>= vectorized::BITSIZE` condition forces exactly `BITSIZE` keys to use
`serialized`, even though other parts of this PR (e.g., bitmap stored in 1 byte
and checks like `<= BITSIZE`) still appear to support up to `BITSIZE` keys. If
`BITSIZE` is 8 (as implied by the 1-byte bitmap), this likely regresses the
fixed-key path for 8 keys. Consider changing the threshold to `>
vectorized::BITSIZE` (or aligning the other checks/packing logic to match this
new restriction).
```suggestion
if (data_types.size() > vectorized::BITSIZE) {
```
##########
be/src/vec/columns/column_vector.cpp:
##########
@@ -476,12 +476,9 @@ MutableColumnPtr ColumnVector<T>::permute(const
IColumn::Permutation& perm, size
template <PrimitiveType T>
void ColumnVector<T>::replace_column_null_data(const uint8_t* __restrict
null_map) {
auto s = size();
- size_t null_count = s - simd::count_zero_num((const int8_t*)null_map, s);
- if (0 == null_count) {
- return;
- }
+ auto value = default_value();
for (size_t i = 0; i < s; ++i) {
- data[i] = null_map[i] ? default_value() : data[i];
+ data[i] = null_map[i] ? value : data[i];
}
Review Comment:
The early-exit when the null map contains no nulls was removed, so this now
always scans and potentially writes the full column. If this method is invoked
on many columns/blocks where nulls are rare (and not all call sites gate it
with `simd::contain_one`), this can be a measurable regression. Consider
reintroducing a fast-path check (e.g., `if (!simd::contain_one(null_map, s))
return;`) before the loop.
##########
be/src/vec/common/hash_table/hash_map_context.h:
##########
@@ -497,47 +559,56 @@ struct MethodKeysFixed : public MethodBase<TData> {
template <typename T>
void pack_fixeds(size_t row_numbers, const ColumnRawPtrs& key_columns,
const ColumnRawPtrs& nullmap_columns, DorisVector<T>&
result) {
- size_t bitmap_size = get_bitmap_size(nullmap_columns.size());
- // set size to 0 at first, then use resize to call default constructor
on index included from [0, row_numbers) to reset all memory
- result.clear();
+ size_t bitmap_size = nullmap_columns.empty() ? 0 : 1;
+ if (bitmap_size) {
+ // set size to 0 at first, then use resize to call default
constructor on index included from [0, row_numbers) to reset all memory
+ // only need to reset the memory used to bitmap
+ result.clear();
+ }
result.resize(row_numbers);
+ auto* __restrict result_data = reinterpret_cast<char*>(result.data());
+
size_t offset = 0;
+ std::vector<bool> has_null_column(nullmap_columns.size(), false);
if (bitmap_size > 0) {
+ std::vector<const uint8_t*> nullmap_datas;
+ std::vector<uint8_t> bit_offsets;
for (size_t j = 0; j < nullmap_columns.size(); j++) {
if (!nullmap_columns[j]) {
continue;
}
- size_t bucket = j / BITSIZE;
- size_t local_offset = j % BITSIZE;
- const auto& data =
+ const uint8_t* __restrict data =
assert_cast<const
ColumnUInt8&>(*nullmap_columns[j]).get_data().data();
- for (size_t i = 0; i < row_numbers; ++i) {
- *((char*)(&result[i]) + bucket) |= data[i] << local_offset;
+
+ has_null_column[j] = simd::contain_one(data, row_numbers);
+ if (has_null_column[j]) {
+ nullmap_datas.emplace_back(data);
+ bit_offsets.emplace_back(j);
}
}
+ constexpr_int_match<1, BITSIZE, PackNullmapsReducer>::run(
+ int(nullmap_datas.size()), nullmap_datas.data(),
bit_offsets.data(),
+ row_numbers, sizeof(T),
reinterpret_cast<uint8_t*>(result_data));
offset += bitmap_size;
}
for (size_t j = 0; j < key_columns.size(); ++j) {
- const char* data = key_columns[j]->get_raw_data().data;
+ const char* __restrict data = key_columns[j]->get_raw_data().data;
auto foo = [&]<typename Fixed>(Fixed zero) {
CHECK_EQ(sizeof(Fixed), key_sizes[j]);
- if (!nullmap_columns.empty() && nullmap_columns[j]) {
- const auto& nullmap =
+ if (has_null_column.size() && has_null_column[j]) {
+ const auto* nullmap =
assert_cast<const
ColumnUInt8&>(*nullmap_columns[j]).get_data().data();
- for (size_t i = 0; i < row_numbers; ++i) {
- // make sure null cell is filled by 0x0
- memcpy_fixed<Fixed, true>(
- (char*)(&result[i]) + offset,
- nullmap[i] ? (char*)&zero : data + i *
sizeof(Fixed));
- }
- } else {
- for (size_t i = 0; i < row_numbers; ++i) {
- memcpy_fixed<Fixed, true>((char*)(&result[i]) + offset,
- data + i * sizeof(Fixed));
- }
+ // make sure null cell is filled by 0x0
+
key_columns[j]->assume_mutable()->replace_column_null_data(nullmap);
+ }
+ auto* __restrict current = result_data + offset;
+ for (size_t i = 0; i < row_numbers; ++i) {
+ memcpy_fixed<Fixed, true>(current, data);
+ current += sizeof(T);
+ data += sizeof(Fixed);
Review Comment:
This introduces two correctness risks: (1) `assume_mutable()` can create a
new column instance when the original is shared; the returned mutable is not
stored back into `key_columns`, so the null replacement may have no lasting
effect. (2) Even if it mutates in-place, you captured `data` before the
mutation; if `assume_mutable()` performs a clone/reallocation, `data` can
become stale. Safer options: avoid mutating `key_columns` here (restore the
previous per-row conditional copy of zero/default for nulls), or
store/propagate the mutable column back and re-fetch `data` after mutation.
--
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]
