LuciferYang commented on code in PR #59217:
URL: https://github.com/apache/spark/pull/59217#discussion_r4206237699
##########
sql/core/src/test/scala/org/apache/spark/sql/execution/command/v2/CheckConstraintSuite.scala:
##########
@@ -147,6 +147,21 @@ class CheckConstraintSuite extends QueryTest with
CommandSuiteBase with DDLComma
}
}
+ test("SPARK-59962: Predicate of a check reading a computed value twice") {
+ // NULLIF reads `id + 1` twice. The predicate stored in the catalog is
built from the check as
+ // the optimizer leaves it, so it must not refer to a column pre-evaluated
under the command.
+ withNamespaceAndTable("ns", "tbl", nonPartitionCatalog) { t =>
+ sql(s"CREATE TABLE $t (id bigint, data string) $defaultUsing")
+ sql(s"ALTER TABLE $t ADD CONSTRAINT c1 CHECK (nullif(id + 1, 3) > 0)")
+ val constraint = getCheckConstraint(loadTable(nonPartitionCatalog, "ns",
"tbl"))
+ assert(constraint.predicateSql() == "nullif(id + 1, 3) > 0")
+ assert(constraint.predicate() != null)
Review Comment:
Fixed as you suggest in the next comment: the test now checks enforcement
through INSERTs and no longer reads the stored predicate. It passes with
`SPARK_ANSI_SQL_MODE=false` too.
##########
sql/core/src/test/scala/org/apache/spark/sql/execution/command/v2/CheckConstraintSuite.scala:
##########
@@ -147,6 +147,21 @@ class CheckConstraintSuite extends QueryTest with
CommandSuiteBase with DDLComma
}
}
+ test("SPARK-59962: Predicate of a check reading a computed value twice") {
Review Comment:
Done, with `(2, 'a')` accepted and `(-5, 'b')` failing with
CHECK_CONSTRAINT_VIOLATION. On master the first INSERT already fails with
UNRESOLVED_COLUMN on `_common_expr_1`.
##########
sql/core/src/test/scala/org/apache/spark/sql/connector/GeneratedColumnWriteSuite.scala:
##########
@@ -456,6 +456,23 @@ class GeneratedColumnWriteSuite extends QueryTest with
DatasourceV2SQLBase {
}
}
+ test("SPARK-59962: CREATE TABLE with a generation expression reading a value
twice") {
Review Comment:
Done: the test runs both CREATE TABLE and CREATE OR REPLACE TABLE. On master
the REPLACE fails at planning as well.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/RewriteWithExpression.scala:
##########
@@ -67,6 +69,23 @@ object RewriteWithExpression extends Rule[LogicalPlan] {
// changed anything: `mapExpressions` and `withNewChildren` preserve
reference equality when
// they rewrite nothing, which is what makes this detectable.
if ((rewrittenAgg eq agg) && (rewrittenProj eq proj)) p else
rewrittenProj
+ // A command's own expressions are not evaluated over its children's
rows: most are stored as
+ // metadata or turned into source predicates, neither of which can hold
a memoized value. A
+ // definition hoisted into a child would leave them reading a column
only that `Project`
+ // produces, or leave the command over a child its planner does not
expect, so a
+ // deterministic one is substituted whatever it costs. Where the
expression is evaluated
+ // after all, as by the runtime group filter of a row-level command,
that evaluates it at
+ // every reference, and it duplicates an impure one that declares itself
deterministic (see
+ // `canSubstitute`). Analysis lets no command but `CreateVariable` hold
a nondeterministic
+ // expression; one read more than once keeps its `With`.
`CreateVariable` is left out: its
+ // default is evaluated rather than stored, so a definition there should
be memoized.
+ case c: Command if !c.isInstanceOf[CreateVariable] &&
+ c.expressions.exists(_.containsPattern(WITH_EXPRESSION)) =>
+
c.mapExpressions(_.transformUpWithPruning(_.containsPattern(WITH_EXPRESSION)) {
Review Comment:
Right, `groupFilterCondition` is the second form, and my earlier reply
missed it. Row-level writes now get their own arm: `condition` is inlined as in
other commands, and `groupFilterCondition` goes through the same selection as a
conditional branch, so a definition worth memoizing keeps its `With` for the
injected filter and a condition that folds to true still folds. On a
group-based table, with a counting UDF in place of `my_udf`, your CASE WHEN
DELETE calls it 4 times again, as on master (6 on b2a5c098330), and a top-level
`my_udf(id) BETWEEN 2 AND 3` 7 times (11 on b2a5c098330). The DELETE test and a
new UPDATE test pin both slots.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/RewriteWithExpression.scala:
##########
@@ -67,6 +69,23 @@ object RewriteWithExpression extends Rule[LogicalPlan] {
// changed anything: `mapExpressions` and `withNewChildren` preserve
reference equality when
// they rewrite nothing, which is what makes this detectable.
if ((rewrittenAgg eq agg) && (rewrittenProj eq proj)) p else
rewrittenProj
+ // A command's own expressions are not evaluated over its children's
rows: most are stored as
+ // metadata or turned into source predicates, neither of which can hold
a memoized value. A
+ // definition hoisted into a child would leave them reading a column
only that `Project`
+ // produces, or leave the command over a child its planner does not
expect, so a
+ // deterministic one is substituted whatever it costs. Where the
expression is evaluated
+ // after all, as by the runtime group filter of a row-level command,
that evaluates it at
+ // every reference, and it duplicates an impure one that declares itself
deterministic (see
+ // `canSubstitute`). Analysis lets no command but `CreateVariable` hold
a nondeterministic
+ // expression; one read more than once keeps its `With`.
`CreateVariable` is left out: its
+ // default is evaluated rather than stored, so a definition there should
be memoized.
+ case c: Command if !c.isInstanceOf[CreateVariable] &&
+ c.expressions.exists(_.containsPattern(WITH_EXPRESSION)) =>
+
c.mapExpressions(_.transformUpWithPruning(_.containsPattern(WITH_EXPRESSION)) {
+ case w: With =>
+ val multiplyReferenced = multiplyReferencedIds(w.child, w.defs)
+ inlineDefs(w)(d => d.child.deterministic ||
!multiplyReferenced.contains(d.id))
Review Comment:
Added to the comment. With three levels of NULLIF over `trim(s)`, the CHECK
predicate stored by ALTER TABLE holds 8 copies of `trim(s)`. A size threshold
would need a rule for which commands may keep a `With`, so I'd rather leave it
until the nesting shows up in practice.
--
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]
