github-actions[bot] commented on code in PR #68297:
URL: https://github.com/apache/doris/pull/68297#discussion_r4059796866
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/agg/CollectSet.java:
##########
@@ -108,4 +109,14 @@ public List<FunctionSignature> getSignatures() {
public Expression resultForEmptyInput() {
return new ArrayLiteral(new ArrayList<>(), this.getDataType());
}
+
+ @Override
+ public void checkLegalityBeforeTypeCoercion() {
+ // The BE set kernel cannot hash raw VARBINARY; reject it before
implicit casts change its type.
+ for (Expression argument : getArguments()) {
+ if (argument.getDataType().isVarBinaryType()) {
Review Comment:
[P2] Complete the aggregate-side VARBINARY fence
This check only sees a direct VARBINARY. ARRAY/STRUCT/MAP values containing
a binary leaf therefore pass collect_set analysis and reach its
INVALID_TYPE/INTERNAL_ERROR factory path. The same function-local gap remains
for direct VARBINARY in ndv/approx_count_distinct, histogram, and
linear_histogram: FE accepts them, but each BE creator list omits
TYPE_VARBINARY. Please apply one recursive pre-coercion check to every
aggregate whose factory does not support binary, including aliases/optional
forms, while preserving intentional byte-carrying or explicitly supported cases
such as collect_list, count, any_value, multi_distinct_count, and DataSketches
HLL. Add direct and nested analysis tests.
##########
be/src/core/value/timestamptz_value.cpp:
##########
@@ -38,6 +39,13 @@ std::string TimestampTzValue::to_string(const
cctz::time_zone& tz, int scale) co
auto lookup_result = tz.lookup(cur_tz_time);
cctz::civil_second civ = lookup_result.cs;
+ // UTC storage bounds do not guarantee a representable session-local year.
Reject
+ // overflow before DateTimeV2 formatting could produce an offset-only wire
value.
+ if (civ.year() < 0 || civ.year() > 9999) {
Review Comment:
[P2] Apply this boundary to FE-folded casts too
This guard runs only in the BE formatter. A constant TIMESTAMPTZ-to-string
cast is folded through TimestampTzLiteral.getStringValueInSessionTimeZone(),
which performs the same session-zone conversion but formats Java's year without
the [0,9999] check. For example, under +08:00 the folded maximum UTC literal
can emit year 10000, while the equivalent nonconstant expression throws here
(the minimum endpoint is symmetric). The new regression intentionally makes its
input nonconstant, so it misses this fold-on/fold-off split. Please enforce the
same bound in FE or defer these folds, and cover both modes/endpoints.
##########
be/src/util/raw_value.h:
##########
@@ -45,6 +46,11 @@ class RawValue {
// Because crc32 hardware is not equal with zlib crc32
inline uint32_t RawValue::zlib_crc32(const void* v, size_t len, const
PrimitiveType& type,
uint32_t seed) {
+ // Reject binary even for NULL instead of reaching the default-type
assertion or hash path.
+ if (type == TYPE_VARBINARY) {
Review Comment:
[P2] Keep crc32_internal from failing after analysis
This new guard is also reached by the separately registered crc32_internal
scalar. Its FE class still advertises variadic AnyDataType with no legality
hook, and FunctionCrc32Internal passes every non-null argument to
RawValue::zlib_crc32, so crc32_internal(VARBINARY) analyzes successfully and
throws here during execution (while a NULL binary takes a different branch).
Please either reject binary arguments before coercion in Crc32Internal, using
getArguments(), or define and implement their routing-hash contract, with
non-null and NULL coverage.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/scalar/ArrayFunctionUtils.java:
##########
@@ -0,0 +1,42 @@
+// 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.nereids.exceptions.AnalysisException;
+import org.apache.doris.nereids.trees.expressions.Expression;
+import org.apache.doris.nereids.types.ArrayType;
+import org.apache.doris.nereids.types.DataType;
+
+/** Argument validation shared by array functions. */
+final class ArrayFunctionUtils {
+ private ArrayFunctionUtils() {
+ }
+
+ static void checkNoVarBinaryArguments(ScalarFunction function) {
+ // Inspect original arguments before coercion can hide unsupported
binary comparison/hash inputs.
Review Comment:
[P2] Make the collection fence recursive and complete
This helper follows only ArrayType links and is called by only the eleven
changed functions. Consequently Struct/Map binary leaves escape those callers;
sibling array_sort/array_compact/array_min-max paths either compute via
compare_at or fail in BE; and MAP construction, lookup, contains, and
aggregation still compare/deduplicate VARBINARY keys or fail after analysis.
Please factor a recursive Array/Struct/Map leaf check and apply it to every
collection operation that compares, hashes, orders, deduplicates, or aggregates
(including both array_sortby inputs), while preserving byte-carrying operations
such as collect_list and map projection. Add direct and nested Array/Struct/Map
cases.
##########
be/src/exprs/create_predicate_function.h:
##########
@@ -106,6 +106,9 @@ typename Traits::BasePtr
create_predicate_function(PrimitiveType type, bool null
using Creator = PredicateFunctionCreator<Traits>;
switch (type) {
+ case TYPE_VARBINARY:
Review Comment:
[P2] Close comparison bypasses outside direct VARBINARY predicates
The FE restriction is not preserved across sibling comparison paths. Equal
ARRAY<VARBINARY> operands pass supportCompare() and execute recursively through
ColumnArray::compare_at; greatest/least admit a VARBINARY common type and fail
only in BE scalar dispatch; and nullif invokes generic eq directly, computing
ColumnVarbinary::compare_at without ComparisonPredicate analysis. Please use
one recursive binary-comparability check before coercion for ordinary
predicates and comparison-bearing scalar helpers, and add direct/nested
equality, range, greatest/least, and nullif cases.
--
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]
