cloud-fan commented on code in PR #59545:
URL: https://github.com/apache/spark/pull/59545#discussion_r4323384269
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/constraints.scala:
##########
@@ -142,6 +160,17 @@ case class CheckConstraint(
.build()
}
+ /**
+ * Whether the rules that compute the current time, date, catalog and the
like change part of `e`,
+ * applied to `e` as the optimizer has it, with its `RuntimeReplaceable`s
replaced.
+ */
+ private def readsCurrentContext(e: Expression): Boolean = {
+ val replaced = ReplaceExpressions.replace(e)
+ replaced.containsPattern(CURRENT_LIKE) ||
+ (ComputeCurrentTime.applyForExpression(replaced) ne replaced) ||
+ (SpecialDatetimeValues.applyForExpression(replaced) ne replaced)
Review Comment:
**Nit (P3):** `SpecialDatetimeValues` also folds `'epoch'`, and
`DateTimeUtils.convertSpecialDate` / `convertSpecialTimestamp` /
`convertSpecialTimestampNTZ` map it to a fixed 1970-01-01 value that doesn't
depend on the current time. So a constant condition like `created >=
CAST('epoch' AS TIMESTAMP)`, or just `created >= 'epoch'` after implicit
casting, counts as reading the current context here and is stored with
`predicate = null`. But the predicate built from the optimized condition is
faithful, and it is what the merge base stored.
Connectors then lose a predicate that could be expressed. And if the
condition also uses a table-qualified column (`tbl.created >= 'epoch'`), every
later write fails with UNRESOLVED_COLUMN. The PR description accepts that
outcome only for conditions that really read the current context.
Could this check count only the special values that depend on the current
time (`now`, `today`, `tomorrow`, `yesterday`)? One way is to reuse the
special-value extraction in `DateTimeUtils` (`extractSpecialValue` is private
there now) instead of checking whether the rewrite changed the tree. A test
with an ALTER-added `'epoch'` condition would cover it.
**Verification:**
- **Regression:** An ALTER TABLE CHECK constraint comparing a timestamp
column with CAST('epoch' AS TIMESTAMP), and one using a table-qualified column
reference, is stored with a non-null predicate. Later writes accept a row at or
after the epoch and reject a row before it with a CHECK constraint violation,
instead of failing to resolve the column.
- **Behavior:** ALTER TABLE CHECK constraints casting 'now' or 'today' to
TIMESTAMP, TIMESTAMP_NTZ or DATE are still stored without a predicate and keep
accepting a row written with the current value and rejecting one written a day
ahead.
- **Compatibility:** An ALTER TABLE CHECK constraint comparing with an
ordinary, non-special datetime string keeps the predicate built from the
optimized condition, so constraints that read no current context store exactly
what they did before this change.
##########
sql/core/src/test/scala/org/apache/spark/sql/execution/command/v2/CheckConstraintSuite.scala:
##########
@@ -170,6 +170,60 @@ class CheckConstraintSuite extends QueryTest with
CommandSuiteBase with DDLComma
}
}
+ test("SPARK-60032: Check constraint added by ALTER TABLE reading the current
time") {
+ // The check compares with the time of the write, not the time of the
ALTER TABLE.
+ Seq(
+ ("TIMESTAMP", "current_timestamp()", "current_timestamp()", "INTERVAL 1
DAY"),
+ ("TIMESTAMP", "CAST('now' AS TIMESTAMP)", "current_timestamp()",
"INTERVAL 1 DAY"),
+ ("TIMESTAMP", "to_timestamp('now')", "current_timestamp()", "INTERVAL 1
DAY"),
+ ("TIMESTAMP_NTZ", "CAST('now' AS TIMESTAMP_NTZ)", "localtimestamp()",
"INTERVAL 1 DAY"),
+ ("DATE", "current_date()", "current_date()", "1"),
Review Comment:
**Nit (P3):** For the two DATE rows, the ALTER TABLE and both INSERTs run on
the same day. So a predicate frozen to the ALTER-time date (`created <= DATE
'<today>'`, which is what the merge base stores) gives the same accept/reject
results and the same error parameters, since `expression` comes from
`predicateSql`. These four cases (2 shapes x ANSI on/off) pass with or without
the fix, and only the TIMESTAMP / TIMESTAMP_NTZ rows catch the regression.
Also, no row casts a TIME value to a timestamp type, so nothing covers the
`ComputeCurrentTime` branch of `readsCurrentContext`, the only check that
catches those casts.
After each ALTER TABLE, could the test assert
`getCheckConstraint(table).predicate() == null` and that `predicateSql` is the
condition text, as 'Predicate should be null if it can't be converted to V2
predicate' already does? That gives every row a failure signal that doesn't
depend on the date. A row such as `created <= CAST(TIME '12:00:00' AS
TIMESTAMP)` that asserts its stored predicate is null would cover the TIME
branch.
**Verification:**
- **Regression:** With current-context detection disabled so that every
ALTER-added constraint stores a predicate built from the optimized condition,
the current_date() and CAST('today' AS DATE) cases fail on the stored-predicate
assertion under both ANSI modes, as the timestamp cases already do.
- **Regression:** With only the TIME-to-timestamp part of current-context
detection removed, the new TIME cast case fails on its stored-predicate
assertion.
- **Behavior:** With the change as implemented, every case in the ALTER
TABLE current-time test passes under ANSI mode on and off, including the accept
and reject writes and the new stored-predicate assertions.
--
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]
