github-actions[bot] commented on code in PR #67132: URL: https://github.com/apache/doris/pull/67132#discussion_r3854042907
########## be/src/exprs/function/array/function_array_except_all.cpp: ########## @@ -0,0 +1,156 @@ +// 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 <type_traits> + +#include "core/assert_cast.h" +#include "core/call_on_type_index.h" +#include "core/column/column_array.h" +#include "core/column/column_array_view.h" +#include "core/column/column_decimal.h" +#include "core/column/column_nullable.h" +#include "core/column/column_string.h" +#include "core/column/column_vector.h" +#include "core/data_type/data_type_array.h" +#include "core/data_type/primitive_type.h" +#include "core/string_ref.h" +#include "exprs/function/function.h" +#include "exprs/function/simple_function_factory.h" + +namespace doris { + +template <PrimitiveType PType> +struct ArrayExceptAllCountMap { + using ElementType = typename ColumnElementView<PType>::ElementType; + using KeyType = typename NativeType<ElementType>::Type; + using Type = phmap::flat_hash_map<KeyType, size_t>; Review Comment: Please replace both direct phmap aliases with Doris's allocator-aware map and use float hash/equality consistent with Doris. There are two independent defects here: (1) FLOAT/DOUBLE use ordinary equality, so a right-side NaN cannot be found by the left lookup and `array_except_all(array(cast('NaN' as double)), array(cast('NaN' as double)))` incorrectly retains it; use the normalized hash plus `doris::EqualTo`. (2) The default `std::allocator` leaves this user-sized scratch table, and the string specialization below, outside query MemTracker accounting; `doris::flat_hash_map` routes allocation through tracking `Allocator::alloc`. Please fix both aliases and add NaN/accounting coverage. ########## fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/scalar/ArrayExceptAll.java: ########## @@ -0,0 +1,83 @@ +// 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. + +package org.apache.doris.nereids.trees.expressions.functions.scalar; + +import org.apache.doris.catalog.FunctionSignature; +import org.apache.doris.nereids.exceptions.AnalysisException; +import org.apache.doris.nereids.trees.expressions.Expression; +import org.apache.doris.nereids.trees.expressions.functions.ExplicitlyCastableSignature; +import org.apache.doris.nereids.trees.expressions.functions.PropagateNullable; +import org.apache.doris.nereids.trees.expressions.shape.BinaryExpression; +import org.apache.doris.nereids.trees.expressions.visitor.ExpressionVisitor; +import org.apache.doris.nereids.types.ArrayType; +import org.apache.doris.nereids.types.DataType; +import org.apache.doris.nereids.types.coercion.AnyDataType; + +import com.google.common.base.Preconditions; +import com.google.common.collect.ImmutableList; + +import java.util.List; + +/** Scalar function array_except_all. */ +public class ArrayExceptAll extends ScalarFunction implements ExplicitlyCastableSignature, + BinaryExpression, PropagateNullable { + + public static final List<FunctionSignature> SIGNATURES = ImmutableList.of( + FunctionSignature.retArgType(0) + .args(ArrayType.of(new AnyDataType(0)), ArrayType.of(new AnyDataType(0))) + ); + + public ArrayExceptAll(Expression arg0, Expression arg1) { + super("array_except_all", arg0, arg1); + } + + private ArrayExceptAll(ScalarFunctionParams functionParams) { + super(functionParams); + } + + @Override + public ArrayExceptAll withChildren(List<Expression> children) { + Preconditions.checkArgument(children.size() == 2); + return new ArrayExceptAll(getFunctionParams(children)); + } + + @Override + public void checkLegalityBeforeTypeCoercion() { + for (Expression argument : getArguments()) { + DataType argumentType = argument.getDataType(); + if (!argumentType.isArrayType()) { + continue; + } + DataType itemType = ((ArrayType) argumentType).getItemType(); + if (itemType.isComplexType() || itemType.isVariantType() || itemType.isJsonType()) { Review Comment: Please keep the FE-accepted element families aligned with the BE implementation. `VarBinaryType` is primitive, so `array_except_all(array(X'AB'), array(X'AB'))` binds through this check and the `ARRAY<AnyDataType(0)>` signature, but BE's `dispatch_switch_all` has no `TYPE_VARBINARY` case and returns `InvalidArgument` only at execution. Either reject unsupported primitive/object families here during analysis or implement the missing BE family, and add a regression for the boundary. ########## regression-test/suites/query_p0/sql_functions/array_functions/test_array_except_all.groovy: ########## @@ -0,0 +1,139 @@ +// 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. + +suite("test_array_except_all") { + order_qt_partial_cancel """ + select array_sort(array_except_all(['a', 'a', 'b'], ['a'])) Review Comment: Please add one non-monotonic assertion without `array_sort` to cover the advertised left-order guarantee, for example `array_except_all([3, 1, 2, 1], [1]) = [3, 2, 1]`. As written, every nontrivial result is sorted (and the BE unit inputs are already ordered), so an implementation that emits hash/sorted order would pass the whole new suite. ########## be/src/exprs/function/array/function_array_except_all.cpp: ########## @@ -0,0 +1,156 @@ +// 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 <type_traits> + +#include "core/assert_cast.h" +#include "core/call_on_type_index.h" +#include "core/column/column_array.h" +#include "core/column/column_array_view.h" +#include "core/column/column_decimal.h" +#include "core/column/column_nullable.h" +#include "core/column/column_string.h" +#include "core/column/column_vector.h" +#include "core/data_type/data_type_array.h" +#include "core/data_type/primitive_type.h" +#include "core/string_ref.h" +#include "exprs/function/function.h" +#include "exprs/function/simple_function_factory.h" + +namespace doris { + +template <PrimitiveType PType> +struct ArrayExceptAllCountMap { + using ElementType = typename ColumnElementView<PType>::ElementType; + using KeyType = typename NativeType<ElementType>::Type; + using Type = phmap::flat_hash_map<KeyType, size_t>; +}; + +template <> +struct ArrayExceptAllCountMap<TYPE_STRING> { + using Type = phmap::flat_hash_map<StringRef, size_t, StringRefHash>; +}; + +class FunctionArrayExceptAll : public IFunction { +public: + static constexpr auto name = "array_except_all"; + static FunctionPtr create() { return std::make_shared<FunctionArrayExceptAll>(); } + + String get_name() const override { return name; } + bool is_variadic() const override { return false; } + size_t get_number_of_arguments() const override { return 2; } + bool use_default_implementation_for_constants() const override { return false; } Review Comment: Please keep the framework's default all-constant execution path. This opt-out also makes `VectorizedFnCall::is_constant()` false, disabling open-time constant-result caching. When FE folding is skipped, two `ColumnConst` arrays retain the live block's logical row count in `ColumnArrayView`, so the loop rebuilds the same map and appends the same potentially large result for every row instead of executing once and returning a `ColumnConst`. Removing the override still leaves one-constant calls on the normal implementation because the framework unwraps only when all arguments are constant; add a multi-row, both-constant BE test that checks the nontrivial value and constant result shape. -- 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]
