github-actions[bot] commented on code in PR #63143: URL: https://github.com/apache/doris/pull/63143#discussion_r3302148081
########## be/src/exprs/aggregate/aggregate_function_datasketches_hll_union_agg.cpp: ########## @@ -0,0 +1,44 @@ +// 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/aggregate/aggregate_function_datasketches_hll_union_agg.h" + +#include <string> + +#include "core/data_type/data_type.h" +#include "core/data_type/define_primitive_type.h" +#include "exec/common/hash_table/hash.h" // IWYU pragma: keep +#include "exprs/aggregate/aggregate_function_simple_factory.h" +#include "exprs/aggregate/helpers.h" +namespace doris { +template <template <PrimitiveType> class Data> +AggregateFunctionPtr create_aggregate_function_datasketches_hll_union_agg( + const std::string& name, const DataTypes& argument_types, const DataTypePtr& result_type, + const bool result_is_nullable, const AggregateFunctionAttr& attr) { + return creator_with_type_list<TYPE_STRING, TYPE_VARCHAR, TYPE_BINARY, TYPE_VARBINARY>::create< Review Comment: `TYPE_BINARY` is registered here, but the implementation only does work when `is_string_type(T) || is_varbinary(T)` is true. `TYPE_BINARY` satisfies neither condition, so the instantiated `add_one()` body becomes a no-op: a BE caller resolving this aggregate for a `TYPE_BINARY` argument will silently ignore every non-null sketch and return `0.0`. Please either remove `TYPE_BINARY` from the factory list or handle it explicitly, and add a test for that registered path. ########## build.sh: ########## @@ -662,6 +663,14 @@ FE_MODULES="$( # Clean and build Backend if [[ "${BUILD_BE}" -eq 1 ]]; then + + echo "install datasketches-cpp to thirdparty path before build be" + update_submodule "contrib/datasketches-cpp" "datasketches-cpp" "https://github.com/apache/datasketches-cpp/archive/refs/tags/5.2.0.tar.gz"; Review Comment: The fallback path in `update_submodule()` only rewrites archive URLs containing `refs/heads`, but this new submodule passes a tag archive (`refs/tags/5.2.0.tar.gz`). If `git submodule update` fails, the build downloads the tag archive rather than the exact gitlink commit recorded in `contrib/datasketches-cpp` (`de8553b...`). That makes restricted/offline fallback builds use source that can differ from the reviewed superproject commit, and future gitlink updates would also be ignored. Please make the fallback archive URL commit-specific for this submodule (and mirror the fix in `run-be-ut.sh`, which uses the same tag URL). ########## fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/agg/DataSketchesHllUnionAgg.java: ########## @@ -0,0 +1,111 @@ +// 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.agg; + +import org.apache.doris.catalog.FunctionSignature; +import org.apache.doris.catalog.Type; +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.Function; +import org.apache.doris.nereids.trees.expressions.functions.FunctionTrait; +import org.apache.doris.nereids.trees.expressions.literal.DoubleLiteral; +import org.apache.doris.nereids.trees.expressions.shape.UnaryExpression; +import org.apache.doris.nereids.trees.expressions.visitor.ExpressionVisitor; +import org.apache.doris.nereids.types.DataType; +import org.apache.doris.nereids.types.DoubleType; +import org.apache.doris.nereids.types.StringType; +import org.apache.doris.nereids.types.VarBinaryType; +import org.apache.doris.nereids.types.VarcharType; + +import com.google.common.base.Preconditions; +import com.google.common.collect.ImmutableList; + +import java.util.List; + +/** datasketches_hll_union_agg agg function. */ +public class DataSketchesHllUnionAgg extends NotNullableAggregateFunction + implements UnaryExpression, ExplicitlyCastableSignature, FunctionTrait, RollUpTrait { Review Comment: Because this function implements `ExplicitlyCastableSignature` and the pre-coercion check only rejects metric types, non-sketch input types such as numeric columns can match the `StringType` signature by inserting a cast to string. Those values are not serialized DataSketches HLL bytes, so the query is accepted by analysis and then fails later in BE deserialization with `CORRUPTION` (or can reach a BE type that is not part of the real contract). This aggregate should reject unsupported argument types before coercion and only permit the serialized byte/string types that BE actually supports. -- 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]
