uros-b commented on code in PR #59785: URL: https://github.com/apache/spark/pull/59785#discussion_r4376850935
########## sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/NanosTimestampSubtractionCheck.scala: ########## @@ -0,0 +1,65 @@ +/* + * 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.spark.sql.catalyst.analysis + +import org.apache.spark.sql.catalyst.expressions.{Expression, ExprUtils, If, SubtractTimestamps} +import org.apache.spark.sql.catalyst.plans.logical.{AsOfJoin, LogicalPlan} +import org.apache.spark.sql.catalyst.trees.TreePattern.SUBTRACT_TIMESTAMPS +import org.apache.spark.sql.types.AnyTimestampNanoType + +/** + * Rejects subtraction with a nanosecond-precision timestamp operand. The interval result only has + * microsecond precision, and adding fractional-seconds precision to it later would change the + * result type. The as-of join ordering distance is internal, so its type never reaches users; it + * still compares only `epochMicros`, so the non-sort-merge rewrite may tie or misrank right rows + * whose keys differ from the left key only below a microsecond. + */ +object NanosTimestampSubtractionCheck extends (LogicalPlan => Unit) { Review Comment: There might be a new false positive for SQL `ASOF JOIN` with a tuple or struct `MATCH_CONDITION`. - `checkAsOfDistance` only knows the DataFrame shapes: a top-level `SubtractTimestamps`, or an `If` over two of them. - The SQL builder splits struct operands per field with `buildLeafOrderExpression`. It emits `Subtract(l, r)` whenever `supportsSubtract` accepts the left type (micro TIMESTAMP, TIMESTAMP_NTZ or DATE). - Struct operands are left uncoerced, and the comparison accepts a micro vs nanos field pair, because `findWiderDateTimeType` widens it to the nanos type. - Example: `MATCH_CONDITION ((l.id, l.ts) >= (r.id, r.ts))` with `l.ts TIMESTAMP_NTZ` and `r.ts TIMESTAMP_NTZ(9)` gets the order expression `CreateStruct(l.id - r.id, l.ts - r.ts)`, which resolves to a nested nanos `SubtractTimestamps`. - Master accepts this query. With this PR it fails with "Subtracting nanosecond-precision timestamps ... in (ts - ts)", although the user wrote no subtraction. - The SQL path never executes `orderExpression` at all: SparkStrategies.scala:452-467 plans from `leftSortExprs` / `rightSortExprs` and `asOfCondition`. - The scalar micro-left case passes only because its distance is top-level. Neither case is tested. This is a static trace, and both features are internal previews, hence minor. Fix, either of: - skip `orderExpression` when `j.requiresSortMergeAsOfJoin`, since the SQL operands also sit in `asOfCondition` and the sort exprs, which stay checked; - add a `CreateNamedStruct` arm that recurses into `valExprs`. And add SQL ASOF tests over mixed micro/nanos keys, scalar and tuple. ########## sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/TypeCoercionHelper.scala: ########## @@ -429,7 +429,9 @@ abstract class TypeCoercionHelper { * microsecond grid (a DayTimeIntervalType in the default mode, a CalendarIntervalType when * spark.sql.legacy.interval.enabled is set), so widening the precision never changes the numeric * result -- it only keeps the two operands the same concrete type. For a pure-micro cross-family - * pair this returns TimestampNTZType, identical to the pre-nanos behavior. + * pair this returns TimestampNTZType, identical to the pre-nanos behavior. A nanos operand only + * survives analysis in the as-of join distance; [[NanosTimestampSubtractionCheck]] rejects it + * anywhere else. Review Comment: Analysis-time folding still bypasses the check, so the new comments' "rejects it anywhere else" is not true. - Example: `EXECUTE IMMEDIATE 'SELECT ?' USING TIMESTAMP_NTZ '2020-01-02 03:04:05.123456789' - TIMESTAMP_NTZ '2020-01-01 03:04:05'`. - USING takes any `namedExpressionSeq`. - Once the arg resolves to a foldable `SubtractTimestamps`, `evaluateParameterExpression` evaluates it into a Literal. - The node is replaced by the inner plan before CheckAnalysis runs, so the query still returns the microsecond interval. - PIVOT values are folded the same way on the `PivotFirst` path (PivotTransformer.scala:100-108; conf med). - The second commit already patched this class of hole for VALUES. Fix, either of: - run `NanosTimestampSubtractionCheck.checkExpression` on the USING args (at the top of `buildUnifiedParameters`) and on pivot values, and add a golden case; - reject inside `SubtractTimestamps` with an explicit as-of opt-in, which closes every fold path by construction. That is a larger change, because the distance is built as `Subtract` before resolution. ########## common/utils/src/main/resources/error/error-conditions.json: ########## @@ -9478,6 +9478,11 @@ "Temporary views cannot be created with the WITH SCHEMA clause. Recreate the temporary view when the underlying schema changes, or use a persisted view." ] }, + "TIMESTAMP_NANOS_SUBTRACTION" : { Review Comment: The `unix_nanos(end) - unix_nanos(start)` hint fails for 2 of the 3 operand mixes that raise this error. - `UnixNanos` is `ExpectsInputTypes` with `inputTypes = Seq(AnyTimestampNanoType)` (datetimeExpressions.scala:1334-1340, where micro support is "deferred to a follow-up"). `ExpectsInputTypes` gets no implicit cast. - So for nanos vs micro (the PR's own LTZ golden `... - TIMESTAMP_LTZ '2020-01-02 03:04:05 UTC'`) and for nanos vs DATE, following the hint fails with `DATATYPE_MISMATCH.UNEXPECTED_INPUT_TYPE`. - The new goldens only exercise the hint with two nanos operands. Fix, either of: - tell users to cast a microsecond or DATE operand to a nanos type first, e.g. `unix_nanos(CAST(start AS TIMESTAMP_NTZ(9)))`; - make `unix_nanos` accept the micro types, as `unix_micros` already accepts the nanos ones. - Add a mixed-operand workaround case. ########## sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/NanosTimestampSubtractionCheck.scala: ########## @@ -0,0 +1,65 @@ +/* + * 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.spark.sql.catalyst.analysis + +import org.apache.spark.sql.catalyst.expressions.{Expression, ExprUtils, If, SubtractTimestamps} +import org.apache.spark.sql.catalyst.plans.logical.{AsOfJoin, LogicalPlan} +import org.apache.spark.sql.catalyst.trees.TreePattern.SUBTRACT_TIMESTAMPS +import org.apache.spark.sql.types.AnyTimestampNanoType + +/** + * Rejects subtraction with a nanosecond-precision timestamp operand. The interval result only has + * microsecond precision, and adding fractional-seconds precision to it later would change the + * result type. The as-of join ordering distance is internal, so its type never reaches users; it + * still compares only `epochMicros`, so the non-sort-merge rewrite may tie or misrank right rows + * whose keys differ from the left key only below a microsecond. + */ Review Comment: The exemption keeps a known wrong-row path on the default DataFrame `joinAsOf`, and nothing tracks it. This is pre-existing since SPARK-57832. - `spark.sql.join.sortMergeAsOfJoin.enabled` defaults to false, and mixed LTZ/NTZ keys always take the rewrite. There, `MIN_BY(struct, orderExpression)` ranks candidates by the per-operand-floored microsecond distance. pandas-on-Spark `merge_asof` takes the same path. - Two concrete failures: - **nearest:** L=`.000000999`, A=`.000000000`, B=`.000001001` score 0 us vs 1 us, so A is returned. B is the true nearest (2 ns away, against 999 ns). - **backward:** two right keys in the same microsecond tie, so an arbitrary one is returned instead of the latest. That is the common case for nanosecond event data. - The scaladoc states the precision loss. The new test (TimestampNanosJoinSuiteBase.scala:405) only uses candidates seconds apart. Ask: - file a JIRA and reference it in the scaladoc; - preferably rank by the `unix_nanos` difference in the rewrite, or route nanos keys to the sort-merge operator, which SortMergeAsOfJoinSuite's "compares nanosecond gaps" tests show is exact; - add a sub-microsecond-neighbour test. -- 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]
