dongjoon-hyun commented on code in PR #59545:
URL: https://github.com/apache/spark/pull/59545#discussion_r4327531723
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/constraints.scala:
##########
@@ -120,16 +122,32 @@ case class CheckConstraint(
condition: String,
override val userProvidedName: String = null,
override val tableName: String = null,
- override val userProvidedCharacteristic: ConstraintCharacteristic =
ConstraintCharacteristic.empty)
+ override val userProvidedCharacteristic: ConstraintCharacteristic =
ConstraintCharacteristic.empty,
+ analyzedChild: Option[Expression] = None)
extends UnaryExpression
with TableConstraint
- with ImplicitCastInputTypes {
+ with ImplicitCastInputTypes
+ with AnalysisAwareExpression[CheckConstraint] {
// scalastyle:on line.size.limit
+ final override val nodePatterns: Seq[TreePattern] =
Seq(ANALYSIS_AWARE_EXPRESSION)
+
override def inputTypes: Seq[AbstractDataType] = Seq(BooleanType)
+ override def stringArgs: Iterator[Any] =
+ Iterator(child, condition, userProvidedName, tableName,
userProvidedCharacteristic)
+
+ override def markAsAnalyzed(): CheckConstraint = copy(analyzedChild =
Some(child))
+
+ // ALTER TABLE builds the constraint from the optimized plan, where the
current time, date,
+ // catalog, user and the like are replaced by their values at the time of
the ALTER TABLE. A
+ // condition that reads one is stored without a predicate, so that writes
check `predicateSql`.
def toV2Constraint: Constraint = {
- val predicate = new V2ExpressionBuilder(child,
true).buildPredicate().orNull
+ val predicate = if (analyzedChild.exists(readsCurrentContext)) {
Review Comment:
**This guard never runs on CREATE TABLE or REPLACE TABLE, so they still
build the predicate from the analyzed condition, the path that the PR
description rules out because of `CAST('today' AS DATE)`.**
`ResolveTableSpec` converts the constraints during analysis
(ResolveTableSpec.scala:110), before `HandleSpecialCommand` marks them, so
`analyzedChild` is `None` there and L149 hands the analyzed condition to
`V2ExpressionBuilder`. The builder folds every `contextIndependentFoldable`
sub-expression (V2ExpressionBuilder.scala:108-114), which is true for a string
cast to DATE or TIMESTAMP_NTZ because `needsTimeZone` has no case for them
(Cast.scala:716-718). An ordinary `Cast.eval` does not know special values, and
`tryFold` rethrows outside a conditional branch. So, from reading the code:
```sql
CREATE TABLE cat.ns.t (id INT, d DATE, CONSTRAINT c CHECK (d <= CAST('today'
AS DATE)));
-- ANSI on: fails with CAST_INVALID_INPUT
-- ANSI off: stores the predicate d <= NULL, so INSERT ... VALUES (1,
DATE'9999-12-31') passes
```
`CAST('now' AS TIMESTAMP_NTZ)` and the implicit cast in `d <= 'today'`
behave the same, while `CAST('now' AS TIMESTAMP)` is fine because it needs a
time zone. This is already in 4.1.0, but after this PR the same condition
behaves differently depending on whether CREATE TABLE or ALTER TABLE added it.
It also qualifies my [earlier comment on
#59217](https://github.com/apache/spark/pull/59217#discussion_r4199939321):
building from the analyzed expression fixes `current_*()`, but CREATE TABLE
already shows that it breaks special datetime strings.
The obvious fix, `readsCurrentContext(analyzedChild.getOrElse(child))`, has
a trap. `ResolveTableSpec` (Analyzer.scala:697) can run before
`ResolveTimeZone` (Analyzer.scala:722) in the same iteration, a string-to-DATE
cast counts as resolved without a time zone, and `SpecialDatetimeValues` reads
`cast.zoneId` (finishAnalysis.scala:320), so it could throw even on
`CAST('2020-01-01' AS DATE)`. Could CREATE and REPLACE TABLE get the same
treatment, e.g. after `ResolveTimeZone.resolveTimeZones(child)`, with CREATE
TABLE rows in the new test, or could this go to a separate JIRA that the
description mentions?
##########
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) ||
Review Comment:
Minor: time-only timestamp strings read the current date too, and nothing
here detects them. For a string with only a time part, `stringToTimestamp`
takes the date from `LocalDate.now(zoneId)` (SparkDateTimeUtils.scala:907-908),
so `ConstantFolding` freezes `CAST('12:00:00' AS TIMESTAMP)`, or the implicit
cast in `ts >= '08:00'`, at the ALTER TABLE date. This line does not catch it
because `Cast.isTimeToTimestampLTZ` is false for a string source, and L171 does
not because '12:00:00' is not a special value:
```sql
-- on 2026-10-20
ALTER TABLE t ADD CONSTRAINT c CHECK (ts <= CAST('23:59:59' AS TIMESTAMP));
-- the stored predicate compares ts with 2026-10-20 23:59:59
-- on 2026-10-21
INSERT INTO t VALUES (1, current_timestamp()); -- CHECK_CONSTRAINT_VIOLATION
```
CREATE TABLE does not fold this cast because it needs a time zone, so there
the check stays live. It is the string form of the TIME-to-TIMESTAMP cast this
line handles. Could it be detected the same way, or listed in the description
as not covered?
##########
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 = {
Review Comment:
**Typed literals such as `TIMESTAMP 'now'` and `DATE 'today'` are still
frozen at the ALTER TABLE time, because the parser has already turned them into
plain literals.**
`AstBuilder.visitTypeConstructor` converts special values while parsing
(AstBuilder.scala:4777, 4817, 4834), so `analyzedChild` holds only a `Literal`,
none of the three checks here fires, and L149 stores the ALTER-time value,
while `predicateSql`, and so the error message, still says `'today'`:
```sql
ALTER TABLE t ADD CONSTRAINT c CHECK (d <= DATE 'today');
-- the next day:
INSERT INTO t VALUES (1, current_date()); -- CHECK_CONSTRAINT_VIOLATION on
d <= DATE 'today'
```
Likewise `created >= TIMESTAMP 'yesterday'` keeps accepting stale rows. The
migration guide names the typed literal as one of the two spellings of special
values, next to the cast this PR handles (sql-migration-guide.md:318). This PR
also makes the result depend on the rest of the condition: `created >=
TIMESTAMP 'yesterday' AND created <= current_timestamp()` gets no predicate
because of `current_timestamp()`, so its typed literal is re-parsed on every
write, while `created >= TIMESTAMP 'yesterday'` alone stays frozen. The
analyzed tree cannot tell these apart, so a fix would need the parser to record
it, e.g. in `visitCheckConstraint`. Note that a test row comparing `TIMESTAMP
'now'` with a `current_timestamp()` value cannot pass under any design, since
the write parses `predicateSql` before `ComputeCurrentTime` takes its snapshot;
a row that writes `TIMESTAMP 'now'`, or an assertion on `predicate()`, would
work. Could the description at least say that typed literals are still f
rozen, with a follow-up JIRA?
##########
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.
Review Comment:
Minor: a check that a row can satisfy today and fail later now also fails
row-level operations on rows they do not change. This test covers INSERT, but
group-based DELETE, UPDATE and MERGE copy the unchanged rows of each rewritten
group through the same `CheckInvariant` (RewriteDeleteFromTable.scala:85-90;
only a delta DELETE is skipped, ResolveTableConstraints.scala:35-38):
```sql
-- day D: rows (1, D) and (2, D + 1) in one group of a copy-on-write table
ALTER TABLE t ADD CONSTRAINT not_past CHECK (d >= current_date());
-- day D + 1:
DELETE FROM t WHERE id > 1; -- copies row 1 and fails with
CHECK_CONSTRAINT_VIOLATION
```
Before this PR the frozen `d >= <day D>` let it succeed. The constraint is
also stored as VALID (constraints.scala:159), which Check.java:39-40 defines as
"the existing data is guaranteed to satisfy the constraint", and that stops
being true over time. CREATE TABLE constraints already behave this way. Could
the user-facing section mention it, and could the questions of whether such a
constraint should be VALID and whether copied rows need checking go to a
separate JIRA?
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/constraints.scala:
##########
@@ -120,16 +122,32 @@ case class CheckConstraint(
condition: String,
override val userProvidedName: String = null,
override val tableName: String = null,
- override val userProvidedCharacteristic: ConstraintCharacteristic =
ConstraintCharacteristic.empty)
+ override val userProvidedCharacteristic: ConstraintCharacteristic =
ConstraintCharacteristic.empty,
+ analyzedChild: Option[Expression] = None)
Review Comment:
Nit: only a Boolean derived from this field is ever read,
`analyzedChild.exists(readsCurrentContext)` at L146. Computing it once in
`markAsAnalyzed` and storing `readsCurrentContext: Boolean = false` would give
the same result on every path, make the `stringArgs` override optional, and
keep the next reader from building the predicate from `analyzedChild` as the
sibling expressions do, which is what c61d6028b87 did and what the description
now rules out. Keeping the tree matches the siblings, so this is a judgment
call. Could this field be a Boolean, or could L142-144 at least say why the
predicate is not built from it?
##########
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`,
Review Comment:
Minor: these comments do not say how the check works or why the predicate is
not built from `analyzedChild`, which is what would stop the next change from
undoing this one.
- This scaladoc says the rules are "applied to `e`", but the catalog,
database, user and path functions are caught by the CURRENT_LIKE pattern at
L169, not by applying `ReplaceCurrentLike`, which needs a `CatalogManager`.
Nothing links this list to `Optimizer.FinishAnalysis`.
- After L169, the term at L170 only matters for TIME-to-TIMESTAMP casts,
which `ComputeCurrentTime` deliberately leaves without CURRENT_LIKE
(finishAnalysis.scala:145-147).
- L142-144 say why the predicate is null, but not why it is not built from
`analyzedChild` as the sibling expressions do (the `CAST('today' AS DATE)`
folding), nor that CREATE and REPLACE TABLE reach `toV2Constraint` with
`analyzedChild = None`.
- The comment of the second test (CheckConstraintSuite.scala:205-207) does
not say why it uses `nullif`: `V2ExpressionBuilder` has no `NullIf` case, so
the test also fails if the predicate is built from the analyzed condition,
which `tbl.id > 0` would not catch.
Could the scaladoc say, for example, "The CURRENT_LIKE pattern stands in for
ReplaceCurrentLike, which needs a CatalogManager, and for the functions
ComputeCurrentTime replaces. ComputeCurrentTime is still applied for the TIME
-> TIMESTAMP casts it rewrites, which carry no CURRENT_LIKE bit.", and could
L144 add why `analyzedChild` is not used?
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/constraints.scala:
##########
@@ -120,16 +122,32 @@ case class CheckConstraint(
condition: String,
override val userProvidedName: String = null,
override val tableName: String = null,
- override val userProvidedCharacteristic: ConstraintCharacteristic =
ConstraintCharacteristic.empty)
+ override val userProvidedCharacteristic: ConstraintCharacteristic =
ConstraintCharacteristic.empty,
+ analyzedChild: Option[Expression] = None)
extends UnaryExpression
with TableConstraint
- with ImplicitCastInputTypes {
+ with ImplicitCastInputTypes
+ with AnalysisAwareExpression[CheckConstraint] {
// scalastyle:on line.size.limit
+ final override val nodePatterns: Seq[TreePattern] =
Seq(ANALYSIS_AWARE_EXPRESSION)
+
override def inputTypes: Seq[AbstractDataType] = Seq(BooleanType)
+ override def stringArgs: Iterator[Any] =
+ Iterator(child, condition, userProvidedName, tableName,
userProvidedCharacteristic)
+
+ override def markAsAnalyzed(): CheckConstraint = copy(analyzedChild =
Some(child))
Review Comment:
Nit: under the legacy `spark.sql.legacy.earlyEvalCurrentTime=true`, `child`
is already folded when this captures it. ResolveTableSpec.scala:39-45 runs
`ComputeCurrentTime(ResolveTimeZone(plan))` during analysis on any plan that
contains a command, `AddCheckConstraint` included, so with that flag
`current_timestamp()`, `now()`, `current_date()` and `localtimestamp()` stay
frozen while `CAST('now' AS TIMESTAMP)` and `current_user()` do not. The doc of
the flag says it sacrifices correctness, so this may be acceptable. Could
L142-144 or the description mention it, with narrowing that branch to the
OPTIONS values it exists for as a separate JIRA?
##########
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) ||
Review Comment:
Minor: no test pins this term, and it is the only one that catches
`current_database()`, `current_catalog()`, `current_user()` and
`current_path()`. The new test's `current_timestamp()` and `current_date()`
rows are also caught by `ComputeCurrentTime` at L170, and its 'now' and 'today'
rows by L171, so deleting this term leaves both new tests green while `ALTER
TABLE ... CHECK (db = current_database())` goes back to freezing the ALTER-time
namespace through `ReplaceCurrentLike`. The description lists
`current_database()` and `current_user()`, but the existing `current_database`
tests (CheckConstraintSuite.scala:609-712) use CREATE TABLE only. Could a test
run `USE ns1`, add `CHECK (db = current_database())` with ALTER TABLE, assert
that `predicate()` is null, then `USE ns2` and insert 'ns2', restoring the
namespace afterwards?
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/constraints.scala:
##########
@@ -120,16 +122,32 @@ case class CheckConstraint(
condition: String,
override val userProvidedName: String = null,
override val tableName: String = null,
- override val userProvidedCharacteristic: ConstraintCharacteristic =
ConstraintCharacteristic.empty)
+ override val userProvidedCharacteristic: ConstraintCharacteristic =
ConstraintCharacteristic.empty,
+ analyzedChild: Option[Expression] = None)
extends UnaryExpression
with TableConstraint
- with ImplicitCastInputTypes {
+ with ImplicitCastInputTypes
+ with AnalysisAwareExpression[CheckConstraint] {
// scalastyle:on line.size.limit
+ final override val nodePatterns: Seq[TreePattern] =
Seq(ANALYSIS_AWARE_EXPRESSION)
+
override def inputTypes: Seq[AbstractDataType] = Seq(BooleanType)
+ override def stringArgs: Iterator[Any] =
+ Iterator(child, condition, userProvidedName, tableName,
userProvidedCharacteristic)
+
+ override def markAsAnalyzed(): CheckConstraint = copy(analyzedChild =
Some(child))
+
+ // ALTER TABLE builds the constraint from the optimized plan, where the
current time, date,
+ // catalog, user and the like are replaced by their values at the time of
the ALTER TABLE. A
+ // condition that reads one is stored without a predicate, so that writes
check `predicateSql`.
Review Comment:
Minor: under the legacy
`spark.sql.legacy.parameterSubstitution.constantsOnly=true`, a parameter marker
stays in `predicateSql`, so a current-context condition with a marker now makes
every write fail. In that mode the parser leaves the markers in the text and
`BindParameters` binds them during analysis (SparkSqlParser.scala:158-161), so
`condition` keeps `:d`. Before this PR the stored predicate held the bound
value; now the predicate is null and each write parses `predicateSql` with `:d`
unbound:
```scala
spark.sql(
"ALTER TABLE t ADD CONSTRAINT c CHECK (ts >= current_timestamp() -
make_interval(0, 0, 0, :d))",
Map("d" -> 1))
// every later INSERT, UPDATE and MERGE fails with UNBOUND_SQL_PARAMETER
```
I have not run it, and CREATE TABLE has the same problem when its predicate
cannot be translated. DEFAULT and GENERATED expressions reject markers for this
reason (AstBuilder.scala:5567-5569, 5596-5598), but `visitCheckConstraint` does
not. Could it reject them the same way?
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/constraints.scala:
##########
@@ -120,16 +122,32 @@ case class CheckConstraint(
condition: String,
override val userProvidedName: String = null,
override val tableName: String = null,
- override val userProvidedCharacteristic: ConstraintCharacteristic =
ConstraintCharacteristic.empty)
+ override val userProvidedCharacteristic: ConstraintCharacteristic =
ConstraintCharacteristic.empty,
+ analyzedChild: Option[Expression] = None)
extends UnaryExpression
with TableConstraint
- with ImplicitCastInputTypes {
+ with ImplicitCastInputTypes
+ with AnalysisAwareExpression[CheckConstraint] {
// scalastyle:on line.size.limit
+ final override val nodePatterns: Seq[TreePattern] =
Seq(ANALYSIS_AWARE_EXPRESSION)
+
override def inputTypes: Seq[AbstractDataType] = Seq(BooleanType)
+ override def stringArgs: Iterator[Any] =
+ Iterator(child, condition, userProvidedName, tableName,
userProvidedCharacteristic)
+
+ override def markAsAnalyzed(): CheckConstraint = copy(analyzedChild =
Some(child))
+
+ // ALTER TABLE builds the constraint from the optimized plan, where the
current time, date,
+ // catalog, user and the like are replaced by their values at the time of
the ALTER TABLE. A
+ // condition that reads one is stored without a predicate, so that writes
check `predicateSql`.
def toV2Constraint: Constraint = {
- val predicate = new V2ExpressionBuilder(child,
true).buildPredicate().orNull
+ val predicate = if (analyzedChild.exists(readsCurrentContext)) {
+ null
Review Comment:
Minor: this adds a second reason for a null predicate, which the public
`Check.predicate()` does not document. Check.java:77-78 says it returns null
"if the condition cannot be expressed using supported predicate expressions",
and the class doc says the predicate is exposed "whenever it can be expressed
using supported expressions" (Check.java:32-33). Under ANSI, `ts <= CAST('now'
AS TIMESTAMP)` gets a V2 cast predicate from CREATE TABLE
(V2ExpressionBuilder.scala:163-165) but null from ALTER TABLE after this PR, so
a connector that reads null as "cannot be enforced natively" behaves
differently depending on which DDL added the constraint. Spark itself falls
back to `predicateSql`, so its own enforcement is fine. Could the Javadoc of
`predicate()` add a sentence such as "Spark may also leave it null for a
condition that reads the current time, date, catalog, user and the like, so
that each write evaluates it from predicateSql()."?
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/constraints.scala:
##########
@@ -120,16 +122,32 @@ case class CheckConstraint(
condition: String,
override val userProvidedName: String = null,
override val tableName: String = null,
- override val userProvidedCharacteristic: ConstraintCharacteristic =
ConstraintCharacteristic.empty)
+ override val userProvidedCharacteristic: ConstraintCharacteristic =
ConstraintCharacteristic.empty,
+ analyzedChild: Option[Expression] = None)
extends UnaryExpression
with TableConstraint
- with ImplicitCastInputTypes {
+ with ImplicitCastInputTypes
+ with AnalysisAwareExpression[CheckConstraint] {
// scalastyle:on line.size.limit
+ final override val nodePatterns: Seq[TreePattern] =
Seq(ANALYSIS_AWARE_EXPRESSION)
+
override def inputTypes: Seq[AbstractDataType] = Seq(BooleanType)
+ override def stringArgs: Iterator[Any] =
+ Iterator(child, condition, userProvidedName, tableName,
userProvidedCharacteristic)
+
+ override def markAsAnalyzed(): CheckConstraint = copy(analyzedChild =
Some(child))
+
+ // ALTER TABLE builds the constraint from the optimized plan, where the
current time, date,
+ // catalog, user and the like are replaced by their values at the time of
the ALTER TABLE. A
+ // condition that reads one is stored without a predicate, so that writes
check `predicateSql`.
Review Comment:
Minor: with no predicate, writes re-parse and re-resolve the whole condition
under their own session settings, not only its current-context part.
ResolveTableConstraints.scala:79 parses `predicateSql` with the writer's parser
and conf, while the predicate stored before this PR carried resolved column
names and already converted literals, and its V2 round trip uses ANSI casts
(V2ExpressionUtils.scala:268). From reading the code:
- With `spark.sql.ansi.doubleQuotedIdentifiers=true` under ANSI, `CHECK
("created" <= current_timestamp())` refers to the column, but a writer with the
default settings parses `"created"` as a string, so every write fails with
CAST_INVALID_INPUT under ANSI, and with ANSI off the check is NULL and accepts
every row.
- `CHECK (created >= TIMESTAMP '2024-01-01 00:00:00' AND created <=
current_timestamp())` added in UTC gets a lower bound 8 hours later for a
writer in America/Los_Angeles.
- `CHECK (CAST(code AS INT) > 0 AND ts <= current_timestamp())` silently
accepts bad codes from a writer with ANSI off.
CREATE TABLE constraints without a predicate already behave this way, and
views avoid it by storing the confs they were created with. Could the
user-facing section say that such constraints are re-parsed with each writer's
session settings, and could storing those confs be a follow-up JIRA?
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/constraints.scala:
##########
@@ -120,16 +122,32 @@ case class CheckConstraint(
condition: String,
override val userProvidedName: String = null,
override val tableName: String = null,
- override val userProvidedCharacteristic: ConstraintCharacteristic =
ConstraintCharacteristic.empty)
+ override val userProvidedCharacteristic: ConstraintCharacteristic =
ConstraintCharacteristic.empty,
+ analyzedChild: Option[Expression] = None)
extends UnaryExpression
with TableConstraint
- with ImplicitCastInputTypes {
+ with ImplicitCastInputTypes
+ with AnalysisAwareExpression[CheckConstraint] {
// scalastyle:on line.size.limit
+ final override val nodePatterns: Seq[TreePattern] =
Seq(ANALYSIS_AWARE_EXPRESSION)
+
override def inputTypes: Seq[AbstractDataType] = Seq(BooleanType)
+ override def stringArgs: Iterator[Any] =
+ Iterator(child, condition, userProvidedName, tableName,
userProvidedCharacteristic)
+
+ override def markAsAnalyzed(): CheckConstraint = copy(analyzedChild =
Some(child))
+
+ // ALTER TABLE builds the constraint from the optimized plan, where the
current time, date,
+ // catalog, user and the like are replaced by their values at the time of
the ALTER TABLE. A
+ // condition that reads one is stored without a predicate, so that writes
check `predicateSql`.
def toV2Constraint: Constraint = {
- val predicate = new V2ExpressionBuilder(child,
true).buildPredicate().orNull
+ val predicate = if (analyzedChild.exists(readsCurrentContext)) {
+ null
+ } else {
+ new V2ExpressionBuilder(child, true).buildPredicate().orNull
Review Comment:
**The second reason in the description, that `V2ExpressionBuilder` evaluates
`CAST('today' AS DATE)` as an ordinary cast, is a bug in
`Cast.contextIndependentFoldable`, and it already breaks DEFAULT values and
generated columns as well.**
`contextIndependentFoldable` promises folding "without relying on external
context, such as current time zone", so that the expression "can be safely
evaluated during DDL operations like creating tables, views, or constraints"
(Expression.scala:106-116). Cast.scala:716-718 decides it from the types alone,
but a special string cast to DATE or TIMESTAMP_NTZ reads the clock and the
session time zone. `DefaultValueExpression.toV2` and `toV2CurrentDefault`
(ColumnDefinition.scala:284, 294) and `GeneratedColumnExpression.toV2`
(GeneratedColumnExpression.scala:135) run the same builder on their
`analyzedChild`. From reading the code, on a V2 catalog:
```sql
CREATE TABLE testcat.t (id INT, d DATE DEFAULT CAST('today' AS DATE));
-- ANSI on: fails with CAST_INVALID_INPUT, and so do ADD COLUMN and SET
DEFAULT
-- ANSI off: stores a NULL literal as the V2 current default, which
-- ResolveDefaultColumnsUtil.scala:219-221 prefers over the SQL text,
-- so INSERT INTO testcat.t(id) VALUES (1) writes NULL instead of today
```
If `contextIndependentFoldable` were false for a foldable string child that
is a special value, the second reason would go away, and so would the CREATE
TABLE problem at L146. Its only reader in the main code is
V2ExpressionBuilder.scala:108, and query pushdown is not affected because the
optimizer has already replaced these casts with literals.
`DateTimeUtils.extractSpecialValue` is private now; a `private[sql]` helper
around it could serve both this and cloud-fan's 'epoch' point at L171. Could
this be filed as a separate JIRA and referenced from the description?
##########
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"),
Review Comment:
Minor: at write time `CAST('now' AS TIMESTAMP)` and `current_timestamp()`
are different instants, so only `<=` holds at the boundary.
`ComputeCurrentTime` folds `current_timestamp()` from one snapshot
(finishAnalysis.scala:116), but `SpecialDatetimeValues` runs after it and folds
'now' with its own `Instant.now()` (DateTimeUtils.scala:807). So `CHECK
(expires_at >= CAST('now' AS TIMESTAMP))` rejects `INSERT INTO t VALUES (1,
current_timestamp())` unless both land in the same microsecond, while the same
check written with `current_timestamp()` accepts it, and so did the frozen
value before this PR. This row passes because it uses `<=` and the two rules
run in that order. The mechanism is not new, as `SELECT current_timestamp() >=
CAST('now' AS TIMESTAMP)` is false as well. Could the comment at L174 or the
description say that 'now' is read after the `current_timestamp()` snapshot?
##########
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"),
+ ("DATE", "CAST('today' AS DATE)", "current_date()", "1")
+ ).foreach { case (dataType, now, current, step) =>
+ Seq("true", "false").foreach { ansi =>
+ withSQLConf(SQLConf.ANSI_ENABLED.key -> ansi) {
+ withNamespaceAndTable("ns", "tbl", nonPartitionCatalog) { t =>
+ sql(s"CREATE TABLE $t (id INT, created $dataType) $defaultUsing")
+ sql(s"ALTER TABLE $t ADD CONSTRAINT valid_time CHECK (created <=
$now)")
+
+ sql(s"INSERT INTO $t VALUES (1, $current)")
+ checkAnswer(spark.table(t).select("id"), Seq(Row(1)))
+
+ val error = intercept[SparkRuntimeException] {
+ sql(s"INSERT INTO $t VALUES (2, $current + $step)")
+ }
+ assert(error.getCondition == "CHECK_CONSTRAINT_VIOLATION")
+ assert(error.getMessageParameters.get("constraintName") ==
"valid_time")
+ assert(error.getMessageParameters.get("expression") == s"created
<= $now")
+ }
+ }
+ }
+ }
+ }
+
+ test("SPARK-60032: Check constraint added by ALTER TABLE on a column
qualified by the table") {
+ // ALTER TABLE resolves `tbl.id` as the column `id` of the table `tbl`,
not as the field `id` of
+ // its struct column `tbl`. A write that parses `predicateSql` reads
`tbl.id` as the field, so
+ // the test fails if this constraint is stored without a predicate.
+ withNamespaceAndTable("ns", "tbl", nonPartitionCatalog) { t =>
+ sql(s"CREATE TABLE $t (id BIGINT, tbl STRUCT<id: BIGINT>) $defaultUsing")
+ sql(s"ALTER TABLE $t ADD CONSTRAINT c1 CHECK (nullif(tbl.id, 3) > 0)")
Review Comment:
Minor: this is pre-existing and outside this PR, but `ALTER TABLE ... ADD
CONSTRAINT ... CHECK` accepts a view and stores the constraint on its base
table after validating only the rows the view returns. Only the CHECK branch of
`visitAddTableConstraint` resolves the target with `createUnresolvedRelation`
(AstBuilder.scala:7067), which follows views (Analyzer.scala:1297-1298); the
other constraints use `UnresolvedTable` (AstBuilder.scala:7071), which fails
with EXPECT_TABLE_NOT_VIEW (Analyzer.scala:1311-1319). From reading the code:
```sql
-- cat.ns.t has a row with id = 5
CREATE TEMPORARY VIEW v AS SELECT * FROM cat.ns.t WHERE id > 50;
ALTER TABLE v ADD CONSTRAINT c CHECK (id > 10);
-- validates NOT(id > 10) AND id > 50, finds no row, and adds c to cat.ns.t
as VALID
```
With a view `SELECT id * 100 AS id FROM t`, the validation checks `id * 100
> 10` but the stored predicate is `id > 10`. Could this be a separate JIRA?
##########
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"),
+ ("DATE", "CAST('today' AS DATE)", "current_date()", "1")
+ ).foreach { case (dataType, now, current, step) =>
+ Seq("true", "false").foreach { ansi =>
+ withSQLConf(SQLConf.ANSI_ENABLED.key -> ansi) {
+ withNamespaceAndTable("ns", "tbl", nonPartitionCatalog) { t =>
+ sql(s"CREATE TABLE $t (id INT, created $dataType) $defaultUsing")
+ sql(s"ALTER TABLE $t ADD CONSTRAINT valid_time CHECK (created <=
$now)")
Review Comment:
Minor: this is pre-existing and outside this PR, but an ALTER TABLE CHECK
whose condition folds to TRUE or NULL fails with an internal error, and a
condition on the current time is a natural way to hit it. The child
`Filter(Not(cond), relation)` (AstBuilder.scala:7068) folds to `Filter(false)`,
`PruneFilters` turns it into an empty `LocalRelation` before
`V2ScanRelationPushDown` runs, and the only strategy for `AddCheckConstraint`
needs a `DataSourceV2ScanRelation` (DataSourceV2Strategy.scala:810), so
planning fails with "No plan for AddCheckConstraint":
```sql
ALTER TABLE t ADD CONSTRAINT c CHECK (current_date() > DATE'2023-01-01');
-- INTERNAL_ERROR
-- the same for CHECK (1 > 0), CHECK (NULL), and CHECK (id IS NOT NULL) on a
NOT NULL column
```
CREATE TABLE accepts these conditions (L1252-1270). Could this be a separate
JIRA, with an ALTER TABLE version of that 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]
