924060929 commented on code in PR #67238: URL: https://github.com/apache/doris/pull/67238#discussion_r4441958425
########## fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/SearchSignatureForDateDiff.java: ########## @@ -0,0 +1,79 @@ +// 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; + +import org.apache.doris.catalog.FunctionSignature; +import org.apache.doris.nereids.trees.expressions.Expression; +import org.apache.doris.nereids.types.DataType; +import org.apache.doris.nereids.util.ExpressionUtils; + +import com.google.common.collect.ImmutableList; + +import java.util.List; + +/** + * Signature search for the {@code *_diff} functions, which list {@code (TIMESTAMPTZ, TIMESTAMPTZ)} first. + * + * <p>{@link SearchSignature} scores a string literal against the TIMESTAMPTZ overload by whether it + * carries a time zone, and breaks a remaining tie between the temporal overloads by list order. A + * string-typed argument that is not a literal, such as a VARCHAR column or a subquery or UNION + * output slot, gives it nothing to score, so the call bound TIMESTAMPTZ by list order and the value + * was cast through the session time zone (apache/doris#66120). + * + * <p>For that case only, when the call has a string-typed argument and no string literal to inspect, + * the TIMESTAMPTZ overload is tried after the others, so the call binds DATETIMEV2 as it does for + * zone-less literals. A call with an inspectable string literal is searched in list order with the + * usual score, so results for zoned, zone-less and mixed literals do not change. + */ +public interface SearchSignatureForDateDiff extends ExplicitlyCastableSignature { + + @Override + default FunctionSignature searchSignature(List<FunctionSignature> signatures) { + List<Expression> arguments = getArguments(); + if (hasStringArgument(arguments) && !hasInspectableStringLiteral(arguments)) { + return ExplicitlyCastableSignature.super.searchSignature(withTimeStampTzLast(signatures)); Review Comment: Runtime follow-up: I have now executed the VARCHAR-column reproducer on current head `7056a0d2a7` and compared it with a FE built from the PR base `f1fd1b432f`. Both used the same local ASAN BE; there is no BE source diff between these commits. Constant folding was disabled, so these results come from BE execution over stored VARCHAR values. ```sql SET time_zone = '+08:00'; SET debug_skip_fold_constant = true; DROP TABLE IF EXISTS diff_inputs; CREATE TABLE diff_inputs ( id INT, a VARCHAR(64), b VARCHAR(64) ) DUPLICATE KEY(id) DISTRIBUTED BY HASH(id) BUCKETS 1 PROPERTIES ("replication_num" = "1"); INSERT INTO diff_inputs VALUES (1, '1800-01-01 01:00:00-14:21', '1800-01-01 00:00:00-14:21'); SET enable_strict_cast = false; SELECT hours_diff(a, b) FROM diff_inputs WHERE id = 1; SET enable_strict_cast = true; SELECT hours_diff(a, b) FROM diff_inputs WHERE id = 1; ``` | Query / mode | Base FE | PR FE | | --- | --- | --- | | `hours_diff(a,b)`, strict cast disabled | 1 | NULL | | `hours_diff(a,b)`, strict cast enabled | 1 | Query fails | | Explicitly cast both columns to `TIMESTAMPTZ(6)`, either mode | 1 | 1 | | Pass the same values directly as string literals, either mode | 1 | 1 | The PR's strict-mode error is: ```text [INVALID_ARGUMENT]parse 1800-01-01 01:00:00-14:21 to datetime failed: invalid minute offset '21' ``` `EXPLAIN VERBOSE` independently confirms `TIMESTAMPTZ(6)` argument casts on the base versus `DATETIMEV2(6)` on the PR. Replacing `-14:21` with `-13:21` produces the same regression, so this is not just the 14-hour boundary. Ordinary `+08:00` column inputs still return 1 on both. Thus this is a regression introduced by the new overload selection: the DATETIMEV2 parsing restriction already existed, but this PR routes previously accepted VARCHAR inputs into it. The observed failure is a valid result becoming NULL or a query error; I have not observed an incorrect non-NULL hour count in this reproducer. Please preserve the accepted offset formats while retaining the intended zone-less-string fix, and add a stored-VARCHAR regression covering both cast modes. -- 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]
