lets-order-some-fries commented on code in PR #67238: URL: https://github.com/apache/doris/pull/67238#discussion_r4565456848
########## 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: Thank you for running this on both FEs, and sorry for the slow reply. Confirmed against the source. With TIMESTAMPTZ moved last, a VARCHAR slot is cast by the DATETIME branch of the offset parser in `cast_to_datetimev2_impl.hpp`, which accepts only 00, 30 or 45 offset minutes in both of its parser paths. The TIMESTAMPTZ branch of the same code accepts any minute below 60, and its comment reads "Keep the legacy DATETIME parser unchanged". So rows such as `-14:21` and `-13:21`, which master converts, become NULL or fail the query on this head. That is a regression from this PR, and I will not touch BE to fix it. A VARCHAR slot can hold zoned and zone-less rows together, so no choice of overload at bind time is right for every row; the conversion of the argument has to carry it. What I intend to push: an uninspectable string slot in the 12 diff functions still binds DATETIMEV2, but is converted through TIMESTAMPTZ(6) and then to DATETIMEV2(6), not straight to DATETIMEV2. The first cast is the one master applies to these slots today, so the inputs master parses are parsed the same way in both strict-cast modes. The second reads the instant back in the session zone, so a zone-less row keeps its wall-clock time and the #66120 results hold. Results would differ from this head only where the TIMESTAMPTZ cast itself differs from the DATETIME one, for example a zone-less time inside a DST gap of the session zone; I will list those cases with the push. It will come with the stored-VARCHAR regression in both strict-cast modes: your `-14:21` reproducer, `-13:21`, `+08:00` and a zone-less row. If you would prefer a different shape for the conversion, tell me and I will follow it. -- 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]
