LuciferYang commented on code in PR #59545:
URL: https://github.com/apache/spark/pull/59545#discussion_r4435699127
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/constraints.scala:
##########
@@ -120,16 +127,47 @@ 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,
+ keepPredicate: Boolean = false,
+ omitPredicate: Boolean = false)
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)
+
+ // A write parses `condition` again. It cannot bind a parameter marker that
the legacy parameter
+ // substitution leaves there, resolves an IDENTIFIER clause in its own
session, and does not
+ // resolve a column qualified by the table name the way ALTER TABLE does.
None of them can be
+ // told apart after analysis, so the parser sets `keepPredicate` for them,
and for any name with
+ // more than one part, since it cannot tell a table qualifier from a struct
column. Such a
+ // condition, or one that `resolvesDifferentlyOnWrite`, keeps the predicate
built from the
+ // optimized plan, as before.
+ override def markAsAnalyzed(): CheckConstraint = copy(omitPredicate =
Review Comment:
Fixed in 9941e799a93 rather than documenting it: the parser also keeps the
predicate when the condition has a name that a write could resolve to a
function, such as a paren-less `current_date` or `user`. So `owner = user`
keeps `owner = '<user>'` as on master, and adding a column named `user` changes
nothing. The new test adds the column and checks that the row is still
rejected, for `user` and `current_date`.
If the ALTER TABLE runs with `spark.sql.ansi.enforceReservedKeywords=true`,
the keyword parses as the function, so the predicate is still dropped and a
writer with the default setting can bind the name to such a column. That is the
writer's-settings case of SPARK-60453. CREATE TABLE constraints take this path
on master, and so does a condition with a lambda, such as `forall(ds, x -> x <=
current_date)`, which has no V2 predicate on master either. I filed SPARK-60787
for both.
##########
sql/core/src/test/scala/org/apache/spark/sql/execution/command/v2/CheckConstraintSuite.scala:
##########
@@ -170,6 +174,275 @@ 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. 'now' is
+ // read after the snapshot `current_timestamp()` takes, so only `<=` holds
for a row written
+ // with `current_timestamp()`.
+ 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", "CAST('23:59:59.999999' AS TIMESTAMP)",
"current_timestamp()",
Review Comment:
Done in 9941e799a93: both rows step two days ahead.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/constraints.scala:
##########
@@ -171,6 +209,65 @@ case class CheckConstraint(
}
}
+object CheckConstraint {
+ /**
+ * Whether `e`, with its `RuntimeReplaceable`s replaced, reads the current
time, date, catalog,
+ * user and the like, which the optimizer replaces with their values. The
CURRENT_LIKE pattern
+ * stands in for `ReplaceCurrentLike`, which needs a `CatalogManager`, and
for the functions
+ * `ComputeCurrentTime` replaces. `ComputeCurrentTime` is still applied for
the casts from TIME
+ * to a timestamp it rewrites, which carry no CURRENT_LIKE bit.
+ */
+ private def readsCurrentContext(e: Expression): Boolean = {
+ val replaced = ReplaceExpressions.replace(e)
+ replaced.containsPattern(CURRENT_LIKE) ||
+ (ComputeCurrentTime.applyForExpression(replaced) ne replaced) ||
+ replaced.exists {
+ case c: Cast => castReadsCurrentDate(c)
+ case _ => false
+ }
+ }
+
+ /**
+ * Whether `cast` reads the current date: `SpecialDatetimeValues` replaces
it with the value of
+ * 'now', 'today', 'tomorrow' or 'yesterday', or it casts a string with only
a time part to a
+ * timestamp with local time zone, which completes it with the current date.
Only a cast of a
+ * constant string at the top level counts, not one into an array, map or
struct of timestamps,
+ * from a VARIANT, or of a string that only the optimizer makes constant.
+ */
+ private def castReadsCurrentDate(cast: Cast): Boolean = cast match {
+ case c if SpecialDatetimeValues.readsCurrentDate(c) => true
+ case Cast(s, TimestampType | _: TimestampLTZNanosType, _, _)
Review Comment:
Added `CAST('23:59:59.999999' AS TIMESTAMP_LTZ(9))` to the current-time test
in 9941e799a93. Without that arm, the row fails on the predicate assertion.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/constraints.scala:
##########
@@ -171,6 +209,65 @@ case class CheckConstraint(
}
}
+object CheckConstraint {
+ /**
+ * Whether `e`, with its `RuntimeReplaceable`s replaced, reads the current
time, date, catalog,
+ * user and the like, which the optimizer replaces with their values. The
CURRENT_LIKE pattern
+ * stands in for `ReplaceCurrentLike`, which needs a `CatalogManager`, and
for the functions
+ * `ComputeCurrentTime` replaces. `ComputeCurrentTime` is still applied for
the casts from TIME
+ * to a timestamp it rewrites, which carry no CURRENT_LIKE bit.
+ */
+ private def readsCurrentContext(e: Expression): Boolean = {
+ val replaced = ReplaceExpressions.replace(e)
+ replaced.containsPattern(CURRENT_LIKE) ||
+ (ComputeCurrentTime.applyForExpression(replaced) ne replaced) ||
+ replaced.exists {
+ case c: Cast => castReadsCurrentDate(c)
+ case _ => false
+ }
+ }
+
+ /**
+ * Whether `cast` reads the current date: `SpecialDatetimeValues` replaces
it with the value of
+ * 'now', 'today', 'tomorrow' or 'yesterday', or it casts a string with only
a time part to a
+ * timestamp with local time zone, which completes it with the current date.
Only a cast of a
+ * constant string at the top level counts, not one into an array, map or
struct of timestamps,
+ * from a VARIANT, or of a string that only the optimizer makes constant.
+ */
+ private def castReadsCurrentDate(cast: Cast): Boolean = cast match {
Review Comment:
Done in 9941e799a93: `SpecialDatetimeValues.constantString` returns the
constant string of a cast, with the guard and the catch, and
`castReadsCurrentDate` evaluates it once and only decides on the value.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveTableConstraints.scala:
##########
@@ -128,16 +128,22 @@ class ResolveTableConstraints(val catalogManager:
CatalogManager) extends Rule[L
private def buildColumnExtractors(
expr: Expression,
+ query: LogicalPlan,
columnExtractors: mutable.Map[String, Expression]): Unit = {
expr match {
case u: UnresolvedExtractValue =>
// When extracting a value from a Map or Array type, we display only
the specific extracted
// value rather than the entire Map or Array structure for clarity and
readability.
columnExtractors(u.sql) = u
+ // A name that is not a column of the write, such as `current_timestamp`
without parentheses
+ // or a variable, is not a value of the row.
case u: UnresolvedAttribute =>
- columnExtractors(u.name) = u
+ val resolver = catalogManager.v1SessionCatalog.conf.resolver
+ if (query.resolve(u.nameParts, resolver).isDefined) {
Review Comment:
Applied in 9941e799a93, with a test that drops `s.j` and checks the position
of the error.
--
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]
