lets-order-some-fries commented on code in PR #67238: URL: https://github.com/apache/doris/pull/67238#discussion_r4567670553
########## 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: Pushed. The fix is the last commit; under it is a merge of master `e82d2650e`, because #68979 and #69833 changed the offset parser and the coercion code this PR sits on, and I wanted the tests run against them. `SearchSignatureForDateDiff` now also overrides `prepareBeforeTypeCoercion()`. When trying TIMESTAMPTZ last is what moves the overload, each string argument is rewritten to `cast(cast(s as TIMESTAMPTZ(6)) as DATETIMEV2(6))`. The call still binds `(DATETIMEV2, DATETIMEV2)`. A call that keeps its overload (a typed temporal argument or an inspectable string literal beside the slot) is coerced exactly as before. No BE change. Your reproducer is in the suite as stored VARCHAR rows, in both strict-cast modes: `-14:21`, `-13:21`, `+08:00`, a zone-less row and a row mixing the two, plus the #66120 day counts in both modes. I have no BE here, so those rows are verified by CI. What I could run is FE constant folding of the same values as subquery outputs: the `-14:21` pair folds to 1, and with the conversion disabled it folds to NULL, which is the regression you measured. Where results differ from the previous head, which cast the slot straight to DATETIMEV2: 1. Offsets only the TIMESTAMPTZ branch accepts are read: minutes other than 00, 30 or 45, a seconds part, or more than 14 hours (up to 18:00). 2. A zone-less time inside a DST gap of the session zone is read as the first instant after the gap. Under `America/Los_Angeles`, `2021-03-14 02:30:00` is read as `03:00:00`, so `minutes_diff` from `2021-03-14 00:30:00` is 150 where plain wall-clock arithmetic gives 120. A unit test pins this so it is visible. 3. A zone-less value whose UTC instant falls outside the TIMESTAMPTZ range, within the session offset of year 0000 or 9999, fails the TIMESTAMPTZ cast as it does on master. Not changed: a varchar slot beside a DATETIMEV2 argument binds DATETIMEV2 on master as well and is cast straight to it, so `-14:21` rows are rejected there on master too. Beside a DATEV2 or TIMESTAMP_NS argument it likewise binds that argument's overload, as on master. One thing to flag. The javadoc of `prepareBeforeTypeCoercion()` describes it for arguments whose value the signature depends on, and here it rewrites arguments. If you would rather have the conversion somewhere else, I will move 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]
