github-actions[bot] commented on code in PR #65674: URL: https://github.com/apache/doris/pull/65674#discussion_r3613189057
########## be/src/format_v2/parquet/reader/plain_fixed_predicate.h: ########## @@ -0,0 +1,125 @@ +// 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. + +#pragma once + +#include <array> +#include <cstddef> +#include <cstdint> +#include <cstring> + +#include "common/status.h" +#include "util/unaligned.h" + +namespace doris::format::parquet { + +enum class PlainFixedPredicateOp : uint8_t { EQ, NE, LT, LE, GT, GE }; +enum class PlainFixedPredicateType : uint8_t { INT32, INT64, FLOAT, DOUBLE }; + +// A compiled predicate over the physical little-endian values of a fixed-width PLAIN page. +// Compilation is deliberately restricted to expressions whose Doris value and Parquet physical +// value have identical comparison semantics; casts and logical-type conversions stay on the +// ordinary materialization path. +class PlainFixedPredicate { +public: + template <typename T> + static PlainFixedPredicate create(PlainFixedPredicateType type, PlainFixedPredicateOp op, + T literal) { + PlainFixedPredicate predicate; + predicate._type = type; + predicate._op = op; + static_assert(sizeof(T) <= sizeof(predicate._literal)); + memcpy(predicate._literal.data(), &literal, sizeof(T)); + return predicate; + } + + size_t value_width() const { + switch (_type) { + case PlainFixedPredicateType::INT32: + case PlainFixedPredicateType::FLOAT: + return sizeof(uint32_t); + case PlainFixedPredicateType::INT64: + case PlainFixedPredicateType::DOUBLE: + return sizeof(uint64_t); + } + __builtin_unreachable(); + } + + PlainFixedPredicateType type() const { return _type; } + + // AND this predicate into matches. The caller owns NULL handling because Parquet omits NULLs + // from the physical value stream. + Status evaluate(const uint8_t* values, size_t num_values, size_t value_width, + uint8_t* matches) const { + if (UNLIKELY(value_width != this->value_width())) { + return Status::Corruption("PLAIN predicate width {} does not match expected {}", + value_width, this->value_width()); + } + switch (_type) { + case PlainFixedPredicateType::INT32: + return _evaluate<int32_t>(values, num_values, matches); + case PlainFixedPredicateType::INT64: + return _evaluate<int64_t>(values, num_values, matches); + case PlainFixedPredicateType::FLOAT: + return _evaluate<float>(values, num_values, matches); + case PlainFixedPredicateType::DOUBLE: + return _evaluate<double>(values, num_values, matches); + } + __builtin_unreachable(); + } + +private: + template <typename T> + Status _evaluate(const uint8_t* values, size_t num_values, uint8_t* matches) const { + const T literal = unaligned_load<T>(_literal.data()); + for (size_t row = 0; row < num_values; ++row) { + if (matches[row] == 0) { + continue; + } + const T value = unaligned_load<T>(values + row * sizeof(T)); + bool keep = false; + switch (_op) { + case PlainFixedPredicateOp::EQ: + keep = value == literal; + break; + case PlainFixedPredicateOp::NE: + keep = value != literal; + break; + case PlainFixedPredicateOp::LT: + keep = value < literal; + break; + case PlainFixedPredicateOp::LE: + keep = value <= literal; + break; + case PlainFixedPredicateOp::GT: Review Comment: [P1] Preserve Doris NaN semantics in the direct PLAIN filter This switch uses raw C++ comparisons for `float`/`double`, but the normal Doris expression path goes through `Compare`, where NaN is equal to NaN and ordered above finite values. For example, a hidden predicate-only PLAIN FLOAT column containing NaN satisfies `col > 0` in `GreaterOp`, while this fast path evaluates `NaN > 0` as false and drops the row. Please use the shared Doris comparison helpers (or disable direct FLOAT/DOUBLE filtering) and add NaN coverage for the direct predicate path. ########## be/src/format_v2/parquet/parquet_scan.cpp: ########## @@ -1130,22 +1649,61 @@ Status ParquetScanScheduler::read_filter_columns(int64_t batch_rows, update_counter_if_not_null(_scan_profile.rows_filtered_by_dict_filter, filtered_rows); if (new_selected_rows != selected_rows_before) { - // The dictionary reader has already appended only surviving values for the - // current column. Apply the compact row filter only to columns read before this - // one, then update the shared selection for later predicate/lazy columns. - RETURN_IF_ERROR(filter_read_predicate_columns(file_block, read_column_positions, - compact_filter)); + // The dictionary reader already appended only survivors for this column. Keep + // older predicate columns in their original coordinate spaces and compact all + // of them once at the expression/output boundary below. *selected_rows = apply_compact_filter_to_selection(compact_filter, selection, selected_rows_before); - *predicate_columns_filtered = true; } file_block->replace_by_position(block_position, std::move(column)); read_column_positions.push_back(cast_set<uint32_t>(block_position)); + remember_column_selection(cast_set<uint32_t>(block_position)); *used_dictionary_filter = true; return Status::OK(); } } + if (single_column_conjuncts != nullptr && + request.is_predicate_only(format::LocalColumnId(cast_set<int32_t>(local_id)))) { + auto predicates = compile_plain_fixed_predicates( + *single_column_conjuncts, file_block->get_by_position(block_position).type, + block_position); + if (predicates.has_value()) { + const uint16_t selected_rows_before = *selected_rows; + IColumn::Filter compact_filter; + bool used_filter = false; + RETURN_IF_ERROR(column_reader->select_with_plain_filter( + *selection, *selected_rows, batch_rows, *predicates, &compact_filter, + &used_filter)); + if (used_filter) { + DORIS_CHECK_EQ(compact_filter.size(), selected_rows_before); + update_counter_if_not_null(_scan_profile.plain_predicate_direct_batches, 1); + update_counter_if_not_null(_scan_profile.plain_predicate_direct_rows, + selected_rows_before); + const uint16_t new_selected_rows = count_selected_rows(compact_filter); + const auto filtered_rows = static_cast<int64_t>(selected_rows_before) - + static_cast<int64_t>(new_selected_rows); + if (conjunct_filtered_rows != nullptr) { + *conjunct_filtered_rows += filtered_rows; + } + if (new_selected_rows != selected_rows_before) { + *selected_rows = apply_compact_filter_to_selection( + compact_filter, selection, selected_rows_before); + } + // Predicate-only values are dead after this exact comparison. Preserve only a + // row-shaped placeholder so later block positions keep their stable identity. Review Comment: [P1] Keep hidden predicate data until residual conjuncts finish This replaces predicate-only payload with defaults immediately, but `predicate_only` only means absent from final projection; the same hidden slot may still be referenced by a multi-column `remaining_conjunct`. For example, with hidden `id`, visible `score`, and `id > 2 AND id + score < 42`, the direct path evaluates `id > 2`, then the residual sees default `id` values and can retain wrong rows. `compact_predicate_columns(false)` cannot restore the discarded payload. Please defer replacement until remaining/delete conjuncts finish (or disable this path when they consume the slot), and add a combined single-plus-multi-column predicate test. -- 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]
