github-actions[bot] commented on code in PR #65972:
URL: https://github.com/apache/doris/pull/65972#discussion_r3642861467
##########
be/src/format_v2/parquet/reader/native/column_chunk_reader.cpp:
##########
@@ -603,6 +604,13 @@ void expand_nullable_pod_values(ColumnType& column, size_t
old_size, size_t comp
auto& data = column.get_data();
DORIS_CHECK_EQ(data.size(), old_size + compact_values);
data.resize(old_size + selected_nulls.size());
+ if constexpr (sizeof(typename ColumnType::value_type) == 4 ||
+ sizeof(typename ColumnType::value_type) == 8) {
+ simd::expand_nullable_values(reinterpret_cast<uint8_t*>(data.data() +
old_size),
Review Comment:
[P2] Keep the typed nullable fallback when SIMD is unavailable
This branch unconditionally routes every 4/8-byte nullable expansion through
the new helper. On ARM or baseline x86—and for fewer than 8/4 output rows even
on AVX2—the helper falls through to a per-row runtime-width `memset`/`memmove`
loop, so the pre-existing typed backwards assignment below is never used for
these columns. This is separate from the fixed BYTE_STREAM_SPLIT fallback:
sparse nullable batches still lose the generic path that the PR promises to
preserve, and the 17-row unit cases do not force the short scalar path on AVX2.
Please use a `try_` dispatch and run the typed loop when SIMD is declined (or
provide an equivalently optimized width-specialized scalar path), and add
forced-scalar before/after coverage for representative sub-vector batches.
##########
be/src/core/data_type_serde/parquet_decode_source.cpp:
##########
@@ -0,0 +1,78 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+#include "core/data_type_serde/parquet_decode_source.h"
+
+#include "core/column/column_vector.h"
+#include "util/simd/parquet_kernels.h"
+
+namespace doris {
+namespace {
+
+template <PrimitiveType TYPE>
+bool try_gather_vector(IColumn& destination, const IColumn& dictionary, const
uint32_t* indices,
+ size_t num_values) {
+ using ColumnType = ColumnVector<TYPE>;
+ using ValueType = typename ColumnType::value_type;
+ if constexpr (sizeof(ValueType) != 4 && sizeof(ValueType) != 8) {
+ return false;
+ } else {
+ auto* destination_vector = dynamic_cast<ColumnType*>(&destination);
+ const auto* dictionary_vector = dynamic_cast<const
ColumnType*>(&dictionary);
+ if (destination_vector == nullptr || dictionary_vector == nullptr) {
+ return false;
+ }
+ // The direct strategy is chosen only while the typed dictionary is
cache-resident. Keep
+ // the existing generic insertion path for tiny batches where gather
setup cannot amortize.
+ constexpr size_t SIMD_LANES = sizeof(ValueType) == 4 ? 8 : 4;
+ if (num_values < SIMD_LANES) {
+ return false;
+ }
+ auto& destination_data = destination_vector->get_data();
+ const auto& dictionary_data = dictionary_vector->get_data();
+ const size_t old_size = destination_data.size();
+ destination_data.resize(old_size + num_values);
+ simd::dictionary_gather(reinterpret_cast<const
uint8_t*>(dictionary_data.data()), indices,
Review Comment:
[P2] Preserve the typed dictionary fallback off AVX2
`try_gather_vector` returns `true` for every 4/8-byte `ColumnVector` batch
of at least 8/4 rows, regardless of whether AVX2 is available. On ARM or
baseline x86, `dictionary_gather()` then falls through to its runtime-width
per-row `memcpy` loop, and this `true` return prevents the existing
`ColumnVector::insert_indices_from()` typed copy loop from running. This is a
distinct remaining instance of the fallback regression from the
BYTE_STREAM_SPLIT thread: every eligible direct-strategy numeric dictionary
literal batch on those hosts now takes the replacement scalar path even though
the PR says non-AVX2 generic paths are preserved. Please make this a `try_`
dispatch that returns false without AVX2 (or keep an equivalently optimized
typed scalar implementation), and add forced-scalar dictionary materialization
coverage.
--
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]
