LuciferYang commented on code in PR #59545:
URL: https://github.com/apache/spark/pull/59545#discussion_r4374699534
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/constraints.scala:
##########
@@ -120,16 +123,39 @@ 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,
+ hasParameterMarker: Boolean = false,
+ readsCurrentContext: 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)
+ // Under the legacy parameter substitution, a parameter marker stays in
`condition`, which a write
+ // cannot bind, so a condition with one keeps the predicate built from the
optimized plan, as
+ // before.
+ override def markAsAnalyzed(): CheckConstraint = copy(
+ readsCurrentContext = !hasParameterMarker &&
CheckConstraint.readsCurrentContext(child))
Review Comment:
Confirmed: after `DROP TEMPORARY VARIABLE days` every INSERT failed with
UNRESOLVED_COLUMN, and `SET VARIABLE days = 0` changed the check.
`IDENTIFIER(c)`, `CAST(v AS DATE)` and a V2 function behaved the same way.
Fixed in 57f662dda0b: `markAsAnalyzed` keeps the predicate built from the
optimized plan when the analyzed condition reads a variable or calls a
user-defined function (a `UserDefinedExpression`, or a V2 `ScalarFunction`
called through `Invoke` or `StaticInvoke`), and the parser now records
`IDENTIFIER(...)` next to parameter markers, since it is gone after analysis.
The new test drops the variable or the function before the INSERT instead of
inserting from `spark.newSession()`, because the in-memory test catalog does
not share its tables with a new session.
For variables, SPARK-57379 (#56472, still open) would reject them in CHECK
constraints on both paths. I filed SPARK-60550 for IDENTIFIER clauses and
user-defined functions in CREATE TABLE constraints.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/constraints.scala:
##########
@@ -120,16 +123,39 @@ 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,
+ hasParameterMarker: Boolean = false,
+ readsCurrentContext: 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)
+ // Under the legacy parameter substitution, a parameter marker stays in
`condition`, which a write
+ // cannot bind, so a condition with one keeps the predicate built from the
optimized plan, as
+ // before.
+ override def markAsAnalyzed(): CheckConstraint = copy(
+ readsCurrentContext = !hasParameterMarker &&
CheckConstraint.readsCurrentContext(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 `readsCurrentContext` finds reading one is stored without
a predicate, so that
+ // writes check `predicateSql`.
+ // The predicate is not built from the analyzed condition, as the default
value of a column is,
+ // because `V2ExpressionBuilder` folds `CAST('today' AS DATE)` there as an
ordinary cast. CREATE
+ // and REPLACE TABLE convert their constraints before analysis finishes,
while
+ // `readsCurrentContext` is still false.
def toV2Constraint: Constraint = {
- val predicate = new V2ExpressionBuilder(child,
true).buildPredicate().orNull
+ val predicate = if (readsCurrentContext) {
Review Comment:
Confirmed: the 'OPEN' row was rejected, and with `<> 'CLOSED'` the 'CLOSED'
row was accepted. Fixed in 57f662dda0b: a condition that reads a CHAR column
keeps the predicate built from the optimized plan, which has the padded
literal. This also keeps the predicate when the CHAR column is not compared
with a literal, which only keeps the old behavior. The new test covers `=` and
`<>`.
CREATE TABLE already rejects the 'OPEN' row, since its stored predicate is
`status = 'OPEN'` without padding. Filed as SPARK-60551.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/constraints.scala:
##########
@@ -171,6 +197,40 @@ 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 Cast(s, to, _, _) if s.foldable &&
s.dataType.isInstanceOf[StringType] =>
+ val v = s.eval()
Review Comment:
Confirmed both, and the DIVIDE_BY_ZERO in place of
NON_DETERMINISTIC_CHECK_CONSTRAINT. Fixed in 57f662dda0b: the string is
evaluated only for a cast to a type that `SpecialDatetimeValues` rewrites or to
TIMESTAMP or TIMESTAMP_LTZ(n), and an error counts as not reading the current
date. The new test covers your `coalesce` condition, the same error under a
cast to TIMESTAMP_LTZ(9) inside `IF(false, ...)`, and the nondeterministic case
with a cast to INT and to DATE.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/constraints.scala:
##########
@@ -171,6 +197,40 @@ 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 Cast(s, to, _, _) if s.foldable &&
s.dataType.isInstanceOf[StringType] =>
Review Comment:
Confirmed that the first three are frozen on master too. In 57f662dda0b, the
Scaladoc of `castReadsCurrentDate` says that only a top-level cast of a
constant string counts, and the description lists casts into nested types, from
VARIANT, and of strings that only the optimizer makes constant under "Not
covered".
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/constraints.scala:
##########
@@ -120,16 +123,39 @@ 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,
+ hasParameterMarker: Boolean = false,
+ readsCurrentContext: 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)
+ // Under the legacy parameter substitution, a parameter marker stays in
`condition`, which a write
+ // cannot bind, so a condition with one keeps the predicate built from the
optimized plan, as
+ // before.
+ override def markAsAnalyzed(): CheckConstraint = copy(
+ readsCurrentContext = !hasParameterMarker &&
CheckConstraint.readsCurrentContext(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 `readsCurrentContext` finds reading one is stored without
a predicate, so that
+ // writes check `predicateSql`.
+ // The predicate is not built from the analyzed condition, as the default
value of a column is,
+ // because `V2ExpressionBuilder` folds `CAST('today' AS DATE)` there as an
ordinary cast. CREATE
+ // and REPLACE TABLE convert their constraints before analysis finishes,
while
+ // `readsCurrentContext` is still false.
def toV2Constraint: Constraint = {
- val predicate = new V2ExpressionBuilder(child,
true).buildPredicate().orNull
+ val predicate = if (readsCurrentContext) {
+ null
Review Comment:
Done in 57f662dda0b: `buildColumnExtractors` keeps only the names that
resolve against the write query, so a name that is not a column of the write,
such as a paren-less `current_user`, no longer shows up among the values. This
covers CREATE TABLE constraints too. The new test pins the whole message for
`owner = current_user` added by CREATE TABLE and by ALTER TABLE.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/DateTimeUtils.scala:
##########
@@ -852,6 +852,32 @@ object DateTimeUtils extends SparkDateTimeUtils {
}
}
+ /**
+ * Whether `input` is one of the special values that read the current date
or time: 'now',
+ * 'today', 'tomorrow' and 'yesterday'.
+ *
+ * @param input A string to check. It can contain trailing or leading
whitespaces.
+ */
+ private[sql] def isCurrentSpecialValue(input: String): Boolean = {
+ extractSpecialValue(input.trim).exists {
+ case "now" | "today" | "tomorrow" | "yesterday" => true
+ case _ => false
+ }
+ }
+
+ /**
+ * Whether `input` is a timestamp string with only a time part, which a cast
to a timestamp with
+ * local time zone completes with the current date.
+ */
+ private[sql] def isTimeOnlyTimestamp(input: UTF8String): Boolean = {
+ try {
+ val (segments, _, justTime) = parseTimestampString(input)
+ segments.nonEmpty && justTime
Review Comment:
Done in 57f662dda0b, with '24:00:00', '23:59:60' and '12:60' added to the
negative cases.
--
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]
