github-actions[bot] commented on code in PR #68308: URL: https://github.com/apache/doris/pull/68308#discussion_r4059305566
########## be/test/exprs/function/functions_logical_test.cpp: ########## @@ -0,0 +1,141 @@ +// 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 "exprs/function/functions_logical.h" + +#include <gen_cpp/Exprs_types.h> +#include <gtest/gtest.h> + +#include <string> +#include <utility> +#include <vector> + +#include "core/block/block.h" +#include "core/column/column_const.h" +#include "core/column/column_nullable.h" +#include "core/data_type/data_type_nullable.h" +#include "core/data_type/data_type_number.h" +#include "core/data_type/primitive_type.h" +#include "exprs/vcompound_pred.h" +#include "exprs/vexpr_context.h" +#include "testutil/column_helper.h" + +namespace doris { + +namespace { + +ColumnWithTypeAndName nullable_boolean_column(std::vector<UInt8> data, std::vector<UInt8> null_map, + std::string name) { + return {ColumnHelper::create_nullable_column<DataTypeUInt8>(data, null_map), + make_nullable(std::make_shared<DataTypeUInt8>()), std::move(name)}; +} + +ColumnPtr execute_or(ColumnWithTypeAndName left, ColumnWithTypeAndName right, size_t rows) { + auto result_type = make_nullable(std::make_shared<DataTypeUInt8>()); + Block block({std::move(left), std::move(right), {nullptr, result_type, "result"}}); + auto status = FunctionOr::create()->execute_impl(nullptr, block, {0, 1}, 2, rows); + EXPECT_TRUE(status.ok()) << status.to_string(); + return block.get_by_position(2).column; +} + +class ColumnExpr final : public VExpr { +public: + ColumnExpr(ColumnPtr column, DataTypePtr type) + : VExpr(std::move(type), false), _column(std::move(column)) {} Review Comment: [P1] Keep this synthetic column expression off the constant path. Because it has no children, inherited `VExpr::is_constant()` returns true, so `VCompoundPred::_has_const_child()` delegates this test to `VectorizedFnCall` instead of reaching `do_null_pred<false>`. This helper never prepares/opens the expression, leaving `_function` unset; the fallback fails in `_do_execute()`, and the test provides no coverage for the compound change. ```suggestion : VExpr(std::move(type), false), _column(std::move(column)) {} bool is_constant() const override { return false; } ``` ########## be/src/exprs/vcompound_pred.h: ########## @@ -625,7 +625,8 @@ class VCompoundPred : public VectorizedFnCall { if constexpr (is_and) { lhs[i] &= rhs[i]; } else { - lhs[i] |= rhs[i]; + // Logical OR must produce a canonical Boolean instead of preserving input bits. + lhs[i] = (lhs[i] != 0) || (rhs[i] != 0); Review Comment: [P1] Preserve compatibility for OR results that bypass these loops. The block-wide shortcuts at lines 521-536 return an operand unchanged, so during a rolling upgrade an old BE can emit byte 65 for `NULL(payload=65) OR TRUE`, PBlock exchange preserves it, and a new BE can pass 65 into regular CASE branch indexing. Canonicalizing those returned operands fixes this route, but an exchanged legacy OR result can also feed CASE directly; please additionally interpret CASE conditions by truthiness (or canonicalize at a defined compatibility boundary) and cover the mixed-version shape. ########## be/src/exprs/function/functions_logical.h: ########## @@ -84,7 +84,9 @@ struct AndImpl { struct OrImpl { using ResultType = UInt8; - static inline constexpr ResultType apply(UInt8 a, UInt8 b) { return a | b; } + // A NULL row may carry an arbitrary nested byte. Canonicalize logical OR to 0 or 1 so that + // such a byte cannot become visible when the other operand makes the result non-NULL. + static inline constexpr ResultType apply(UInt8 a, UInt8 b) { return (a != 0) || (b != 0); } Review Comment: [P1] This normalization leaves another rolling-upgrade consumer incorrect. An old BE can send the semantically true byte 65 produced by pre-fix OR; PBlock preserves it, and the new BE's registered `XorImpl` still computes `65 ^ 1 = 64`. Doris interprets 64 as true, so `TRUE XOR TRUE` returns TRUE before any later OR or CASE. Please normalize XOR by operand truthiness (`(a != 0) != (b != 0)`) or define one Boolean canonicalization boundary, and add a mixed-version case. ########## be/src/exprs/function/functions_logical.h: ########## @@ -84,7 +84,9 @@ struct AndImpl { struct OrImpl { using ResultType = UInt8; - static inline constexpr ResultType apply(UInt8 a, UInt8 b) { return a | b; } + // A NULL row may carry an arbitrary nested byte. Canonicalize logical OR to 0 or 1 so that + // such a byte cannot become visible when the other operand makes the result non-NULL. + static inline constexpr ResultType apply(UInt8 a, UInt8 b) { return (a != 0) || (b != 0); } Review Comment: [P1] Normalize the null decision as well as the data byte. The unchanged `apply_null` below tests the low bit via `1 ^ a`, not truthiness. A supported rolling-upgrade path can produce an even true byte: old OR emits `{65,1}`, XOR with TRUE yields `{64,0}`, and PBlock preserves it; on the new BE, `64 OR NULL` reaches this formula and is marked NULL instead of TRUE. Please derive nullness with `a == 0` / `b == 0` in both `OrImpl` and `VCompoundPred`, and test both operand orders. -- 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]
