peter-toth commented on code in PR #59965:
URL: https://github.com/apache/spark/pull/59965#discussion_r4435126197
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveTableConstraints.scala:
##########
@@ -123,23 +131,41 @@ class ResolveTableConstraints(val catalogManager:
CatalogManager) extends Rule[L
val colRef = UnresolvedAttribute.quoted(col.name)
val columnExtractors = Seq(col.name -> colRef)
val genExprSql = Option(genExpr.getSql()).getOrElse(catalystExpr.sql)
- CheckInvariant(
- EqualNullSafe(colRef, catalystExpr),
- columnExtractors,
- "Generated Column",
- s"${col.name} <=> $genExprSql")
+ analyzeGeneratedColumnCheck(
+ CheckInvariant(
+ EqualNullSafe(colRef, catalystExpr),
+ columnExtractors,
+ "Generated Column",
+ s"${col.name} <=> $genExprSql"),
+ v2Write.query)
}
}.toSeq
}
+ /**
+ * Analyzes the check of a provided value under the settings an auto-filled
value is analyzed
+ * under, so that a row Spark auto-filled passes the check when a later
write carries it. What
+ * those settings cannot resolve is left to the analysis of the write, as
for an auto-filled
+ * value.
+ */
+ private def analyzeGeneratedColumnCheck(check: Expression, query:
LogicalPlan): Expression = {
+ SQLConf.withExistingConf(StoredExpressionText.conf(conf)) {
+ analyze(Filter(check, LocalRelation(query.output)))
+ } match {
+ case Filter(analyzed, _: LocalRelation) => analyzed
+ // A catalog can store a text that analyzes to another plan. The write
analyzes it as is.
+ case _ => check
+ }
+ }
+
/**
* Convert a V2 GenerationExpression to a Catalyst expression.
* Try the V2 expression first, fall back to parsing the SQL string.
*/
private def buildGenerationCatalystExpression(genExpr:
GenerationExpression): Expression = {
Option(genExpr.getExpression)
.flatMap(V2ExpressionUtils.toCatalyst)
-
.getOrElse(catalogManager.v1SessionCatalog.parser.parseExpression(genExpr.getSql))
+
.getOrElse(StoredExpressionText.parse(catalogManager.v1SessionCatalog.parser,
genExpr.getSql))
Review Comment:
**Finding 5.** For a table created with ANSI off, the check of a provided
value and the auto-fill can still read the expression differently:
- The auto-fill parses the text and analyzes it with ANSI on
(`Analyzer.scala:4143`).
- The check takes the V2 form first, here. That form comes from the analysis
in the DDL's session, so it carries that session's implicit casts. They are
already explicit, so `analyzeGeneratedColumnCheck` doesn't change them.
In round 1 I said the V2 form doesn't take over, but that only holds for `a
+ 1`. `V2ExpressionBuilder` keeps a non-ANSI cast when it is an upcast, so
`coalesce` and `CASE WHEN` do get a V2 form. I ran this with ANSI off for the
DDL and the writes, on `testcat`:
```sql
CREATE TABLE t (id INT, s STRING, b STRING GENERATED ALWAYS AS (coalesce(s,
1)));
-- stored V2 form: COALESCE(s, '1')
INSERT INTO t (id, s) VALUES (1, '007'); -- head: b = '7' (s cast
to BIGINT), base: b = '007'
INSERT INTO t (id, s, b) VALUES (3, '007', '7'); -- head:
CHECK_CONSTRAINT_VIOLATION
```
Each insert ran on a fresh table, because `InMemoryTable` drops the V2 form
on its first write. `CASE WHEN id > 0 THEN s ELSE 1 END` behaves the same. Base
auto-fills `'007'`, and its check accepts `'007'`. So on head, a later write
that carries an auto-filled row fails on a connector that keeps the V2 form,
such as a copy-on-write `DELETE`. I couldn't run that `DELETE`, because
`InMemoryRowLevelOperationTableCatalog` builds its tables from a `StructType`
and drops the V2 form. That is also why the new DELETE test doesn't reach this
path. The description's "keeps a row that Spark auto-filled valid when a later
write carries it" doesn't hold here yet.
Reading the text first when there is one makes both sides agree. This also
takes the parser change from finding 9. With it, the second insert passes, and
the full `GeneratedColumnWriteSuite` and `DataSourceV2DataFrameSuite` still
pass:
```scala
Option(genExpr.getSql)
.map(StoredExpressionText.parse(CatalystSqlParser, _))
.orElse(Option(genExpr.getExpression).flatMap(V2ExpressionUtils.toCatalyst))
.get
```
The `GenerationExpression` javadoc and the comment above this method say
Spark converts the V2 form first. So they would need the same change. The other
way is to make the auto-fill read the V2 form first too. Either way, a test
with the two inserts above on `testcat` would pin it.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/StoredExpressionText.scala:
##########
@@ -0,0 +1,78 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.spark.sql.catalyst.util
+
+import org.apache.spark.sql.catalyst.analysis.{Analyzer,
ExpressionWithUnresolvedIdentifier}
+import org.apache.spark.sql.catalyst.expressions.{Alias, Expression, ExprId}
+import org.apache.spark.sql.catalyst.parser.{ParseException, ParserInterface}
+import org.apache.spark.sql.internal.SQLConf
+
+/**
+ * Reads the SQL text of an expression stored with a table, such as a CHECK
condition or a
+ * generation expression, independently of the settings of the session that
reads it.
+ */
+object StoredExpressionText {
+
+ /**
+ * The settings a stored text is read under. The session time zone and case
sensitivity stay with
+ * the given session, because the settings of the DDL are not known here and
pinning them would
+ * silently change existing tables of a non-UTC or a case-sensitive
deployment.
+ */
+ def conf(session: SQLConf): SQLConf = {
+ val c = new SQLConf
+ Analyzer.retainResolutionConfigsForAnalysis(newConf = c, existingConf =
session)
+ c.setConfString(SQLConf.ANSI_ENABLED.key, "true")
+ c.setConfString(SQLConf.SESSION_LOCAL_TIMEZONE.key,
session.sessionLocalTimeZone)
Review Comment:
**Finding 7.** The rationale for the time zone here also applies to
`spark.sql.timestampType`, but that setting is pinned. `readsAsInSession` can't
catch it, because it acts during analysis. `to_timestamp`, `make_timestamp` and
similar functions take their result type from it when they resolve. With the
session time zone `America/Los_Angeles` and
`spark.sql.timestampType=TIMESTAMP_NTZ` for the DDL and the write, on base and
head:
```sql
CREATE TABLE t (a STRING, b TIMESTAMP_NTZ GENERATED ALWAYS AS
(to_timestamp(a))); -- accepted on both
INSERT INTO t (a) VALUES ('2024-03-10 02:30:00');
-- base: b = 2024-03-10 02:30:00, which SELECT to_timestamp(a) also returns
in this session
-- head: b = 2024-03-10 03:30:00 (TIMESTAMP_LTZ in the DST gap, then cast to
NTZ)
```
On head the check of a provided value accepts both `02:30` and `03:30`, so
nothing flags the change. There are two ways out, and I tried neither:
- Keep `spark.sql.timestampType` from the session, like the time zone.
`CAST(a AS TIMESTAMP)` then follows the writer again.
- Have `validate` also analyze the text under the write settings, and reject
it when the result differs from the analyzed child. That would also catch other
settings that act during analysis.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala:
##########
@@ -4122,11 +4122,37 @@ class Analyzer(
cleanedRelation
case other => other
}
- v2Write.withNewQuery(projection).withNewTable(cleanedTable)
+ v2Write
+ .withNewQuery(analyzeAutoFilledGeneratedColumns(projection,
autoFilledGenCols))
+ .withNewTable(cleanedTable)
} else {
v2Write
}
}
+
+ /**
+ * Analyzes the auto-filled generated columns of a write under the
settings their stored text
+ * is read under, so that the implicit casts and the functions of a
generation expression do
+ * not follow the session that writes.
+ */
+ private def analyzeAutoFilledGeneratedColumns(
+ projection: LogicalPlan,
+ autoFilled: Set[String]): LogicalPlan = projection match {
+ case p @ Project(projectList, child) if autoFilled.nonEmpty =>
+ val filled = projectList.filter(c => autoFilled.contains(c.name))
+ SQLConf.withExistingConf(StoredExpressionText.conf(conf)) {
+ execute(Project(filled, LocalRelation(child.output)))
+ } match {
+ // A text these settings cannot resolve stays unresolved here, and
the analysis of the
Review Comment:
**Finding 6.** This comment doesn't hold once the text contains a node that
is fixed before the session takes over. Neither does the migration guide's "A
text that does not resolve with ANSI mode on ... also still follows that
session". `StoredExpressionText.parse` builds `Cast` and the arithmetic nodes
with ANSI eval modes, and the nested pass resolves functions such as `abs` with
ANSI too. The session then only finishes what is left, and the ANSI parts stay.
I ran both cases below on base and head, with ANSI off for the DDL and for
every write:
```sql
CREATE TABLE t1 (d DATE, b BOOLEAN GENERATED ALWAYS AS (CAST(d AS BOOLEAN)));
INSERT INTO t1 (d) VALUES (DATE'2024-01-01');
-- base: b = NULL from an ANSI-off writer; head:
DATATYPE_MISMATCH.CAST_WITH_CONF_SUGGESTION from any writer
CREATE TABLE t2 (flag BOOLEAN, a INT,
b INT GENERATED ALWAYS AS (CASE WHEN flag = 1 THEN abs(a) ELSE 0 END));
INSERT INTO t2 (flag, a) VALUES (true, -2147483648);
-- base: b = -2147483648; head: ARITHMETIC_OVERFLOW
```
The first case makes a 4.3 table created with ANSI off unwritable from any
session. CREATE accepts it on head too, because `readsAsInSession` compares
under the session's ANSI value. This also corrects my verdict on finding 3.
`flag = 1` alone behaves as on base, but these shapes don't.
Neither of these is tried here:
- **Make the comment true.** Read a column that the fixed settings don't
resolve entirely under the session. Parse its text again under `SQLConf.get`,
and leave it to the outer analysis instead of keeping the partly resolved tree.
Do the same in `analyzeGeneratedColumnCheck`. Doing it per column would also
fix `case _ => p` below. Today that case sends every auto-filled column back to
the session when one catalog-stored text analyzes to another plan.
- **Reject at CREATE.** Analyze the text under the write settings, and
reject it when it doesn't resolve. Then say in the migration guide that
existing tables with such a text fail.
A test with `t1` would pin either one.
##########
docs/sql-migration-guide.md:
##########
@@ -69,6 +69,8 @@ license: |
- Since Spark 4.4, `UPDATE` and `MERGE` on a Data Source V2 table fail with
`DATATYPE_MISMATCH.INVALID_ROW_LEVEL_OPERATION_ASSIGNMENTS` when they assign to
a metadata column of the table. This includes `UPDATE SET *` and `INSERT *` in
`MERGE WITH SCHEMA EVOLUTION` when the source has a column with the name of a
metadata column of the target table. Previously, these assignments passed
analysis and were usually ignored. To restore the previous behavior, set
`spark.sql.legacy.ignoreMetadataColumnAssignments` to `true`.
- Since Spark 4.4, `CREATE TABLE`, `REPLACE TABLE`, and `ALTER TABLE ... ADD
CONSTRAINT` on a Data Source V2 table fail with
`USER_DEFINED_FUNCTION_IN_CHECK_CONSTRAINT` when the condition of a CHECK
constraint calls a user-defined function, including a Data Source V2 function.
`ALTER TABLE ... ADD CONSTRAINT` keeps failing with `UNSUPPORTED_SQL_UDF_USAGE`
on a SQL function and with `INVALID_WHERE_CONDITION` on an aggregate function.
Previously, a write could resolve the function name in its own session, so that
it enforced another rule or failed (SPARK-60777). To run these statements,
change the condition to call only built-in functions. Existing constraints that
call a user-defined function are not rejected.
- Since Spark 4.4, a write to a Data Source V2 table that parses the SQL text
of a CHECK constraint binds an unqualified built-in function name to
`system.builtin`, regardless of the SQL path and
`spark.sql.functionResolution.sessionOrder` (SPARK-60777). Previously, an
existing constraint could call another function with the same name.
+- Since Spark 4.4, a write to a Data Source V2 table parses and analyzes the
stored text of a generation expression with ANSI mode on and the default values
of the SQL configs that change how a text reads, such as
`spark.sql.parser.escapedStringLiterals`, regardless of the session of the
write. Only the session time zone, case sensitivity, and the configs that a
view also takes from the session that reads it follow that session. A text that
does not resolve with ANSI mode on, such as `flag = 1` on a `BOOLEAN` column,
also still follows that session. Previously, the session of the write decided
how the text read, so a write could silently store a wrong value (SPARK-60757).
As a result, with `spark.sql.ansi.enabled=false`, also for a table created with
that setting, a write can fail with an error such as `CAST_INVALID_INPUT` where
Spark 4.3 stored `NULL`, and an implicit cast can change the stored value. A
row that Spark 4.3 stored that way can fail with such an error or with `CHECK_C
ONSTRAINT_VIOLATION` when a later write, such as a `DELETE` that rewrites
rows, carries it.
Review Comment:
**Finding 10.** Two gaps in this entry:
- "Only the session time zone, case sensitivity, and the configs that a view
also takes from the session that reads it follow that session" leaves out two
things the PR description lists under "The following still depend on the
session that writes". Those are the functions found through `SET PATH`, and the
settings an expression reads during evaluation, such as
`spark.sql.legacy.timeParserPolicy`.
- It covers existing tables only for ANSI. A table created in 4.3 under a
non-default parser setting now gets the default reading of its text on every
write, with no error. An example is a deployment with
`spark.sql.parser.escapedStringLiterals=true`. Its earlier rows can then fail a
later `DELETE` that rewrites them. That is worth one sentence, together with
the fix: recreate the column with a text that reads the same.
The neighbouring entries name a `spark.sql.legacy.*` config that restores
the old behaviour. With finding 6, some existing tables can't be written at
all, so such a config could be worth having here too.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveTableConstraints.scala:
##########
@@ -123,23 +131,41 @@ class ResolveTableConstraints(val catalogManager:
CatalogManager) extends Rule[L
val colRef = UnresolvedAttribute.quoted(col.name)
val columnExtractors = Seq(col.name -> colRef)
val genExprSql = Option(genExpr.getSql()).getOrElse(catalystExpr.sql)
- CheckInvariant(
- EqualNullSafe(colRef, catalystExpr),
- columnExtractors,
- "Generated Column",
- s"${col.name} <=> $genExprSql")
+ analyzeGeneratedColumnCheck(
+ CheckInvariant(
+ EqualNullSafe(colRef, catalystExpr),
+ columnExtractors,
+ "Generated Column",
+ s"${col.name} <=> $genExprSql"),
+ v2Write.query)
}
}.toSeq
}
+ /**
+ * Analyzes the check of a provided value under the settings an auto-filled
value is analyzed
+ * under, so that a row Spark auto-filled passes the check when a later
write carries it. What
+ * those settings cannot resolve is left to the analysis of the write, as
for an auto-filled
+ * value.
+ */
+ private def analyzeGeneratedColumnCheck(check: Expression, query:
LogicalPlan): Expression = {
+ SQLConf.withExistingConf(StoredExpressionText.conf(conf)) {
+ analyze(Filter(check, LocalRelation(query.output)))
+ } match {
+ case Filter(analyzed, _: LocalRelation) => analyzed
+ // A catalog can store a text that analyzes to another plan. The write
analyzes it as is.
+ case _ => check
+ }
+ }
+
/**
* Convert a V2 GenerationExpression to a Catalyst expression.
* Try the V2 expression first, fall back to parsing the SQL string.
*/
private def buildGenerationCatalystExpression(genExpr:
GenerationExpression): Expression = {
Option(genExpr.getExpression)
.flatMap(V2ExpressionUtils.toCatalyst)
-
.getOrElse(catalogManager.v1SessionCatalog.parser.parseExpression(genExpr.getSql))
+
.getOrElse(StoredExpressionText.parse(catalogManager.v1SessionCatalog.parser,
genExpr.getSql))
Review Comment:
**Finding 9.** This is the only place that reads the text with the session
parser. `v1SessionCatalog.parser` is `SparkSqlParser`, which substitutes
`${...}` under the active conf. Here that is the fixed conf, where
`spark.sql.variable.substitute` is back to `true`. The auto-fill
(`TableOutputResolver.scala:429` and `:603`) and `readsAsInSession` use
`CatalystSqlParser`, which never substitutes. With
`spark.sql.variable.substitute=false` for the DDL and the writes, on base and
head:
```sql
CREATE TABLE t (a STRING, b STRING GENERATED ALWAYS AS (concat(a,
'${system:java.version}')));
INSERT INTO t (a) VALUES ('v'); -- b =
v${system:java.version} on both
INSERT INTO t (a, b) VALUES ('w', 'w${system:java.version}'); -- base:
succeeds; head: CHECK_CONSTRAINT_VIOLATION
```
`CONCAT` has no way back from its V2 form, so this path reads the text on
any connector. With `CatalystSqlParser` here (plus its import), the second
INSERT passes, and the full `GeneratedColumnWriteSuite` and
`DataSourceV2DataFrameSuite` still pass. It also stops a parser extension from
reading the text differently in the two paths. The change in finding 5 includes
this one.
```suggestion
.getOrElse(StoredExpressionText.parse(CatalystSqlParser,
genExpr.getSql))
```
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/StoredExpressionText.scala:
##########
@@ -0,0 +1,78 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.spark.sql.catalyst.util
+
+import org.apache.spark.sql.catalyst.analysis.{Analyzer,
ExpressionWithUnresolvedIdentifier}
+import org.apache.spark.sql.catalyst.expressions.{Alias, Expression, ExprId}
+import org.apache.spark.sql.catalyst.parser.{ParseException, ParserInterface}
+import org.apache.spark.sql.internal.SQLConf
+
+/**
+ * Reads the SQL text of an expression stored with a table, such as a CHECK
condition or a
+ * generation expression, independently of the settings of the session that
reads it.
+ */
+object StoredExpressionText {
+
+ /**
+ * The settings a stored text is read under. The session time zone and case
sensitivity stay with
+ * the given session, because the settings of the DDL are not known here and
pinning them would
+ * silently change existing tables of a non-UTC or a case-sensitive
deployment.
+ */
+ def conf(session: SQLConf): SQLConf = {
Review Comment:
**Finding 12.** An alternative to the fixed settings: store the creating
session's SQL configs with the generation expression, as views and SQL
functions already do, and read the text under them on every write. Concretely:
- **At CREATE / REPLACE TABLE:** `GeneratedColumnExpression.toV2` captures
the configs with `CapturesConfig.sqlConfigsToProps`
(`CapturesConfig.scala:66`), the helper that the view commands and
`CreateSQLFunctionCommand` use. `GenerationExpression` (`@Evolving`, since
4.3.0) carries them as a new `Map<String, String>`, or they go into the column
metadata next to `GENERATION_EXPRESSION`.
- **On a write:** `TableOutputResolver`, `ResolveOutputRelation` and
`ResolveTableConstraints` parse and analyze the text under a conf built the way
`View.effectiveSQLConf` (`basicLogicalOperators.scala:978`) builds one for a
view. That conf already keeps the session-bound configs, and it replaces
`StoredExpressionText.conf`. The time zone could be left out of the captured
set, if the rationale in the scaladoc above holds for it.
- **What goes away:** `readsAsInSession`, `normalize` and the CREATE-time
rejection. The text is read under the settings it was written in, so no session
can read it differently.
- **What it fixes:** findings 6, 7 and 8 by construction, and likely 5,
because the V2 form and the text would then come from the same analysis. A
legacy cast stays legal in a table created with ANSI off. `to_timestamp` keeps
the DDL's `spark.sql.timestampType`. `allowPrecisionLoss` no longer causes a
false rejection. It also removes most of the cases the migration guide has to
explain in finding 10.
The counter-arguments are real:
- Tables created in 4.3 have no stored configs, so they still need today's
fixed reading (or the session's). Most of this PR stays as their fallback.
- `GenerationExpression` is connector API. Every catalog has to round-trip
the new field, and one that drops it silently falls back to the fixed reading.
- With the DDL's ANSI value stored, a table created with ANSI off keeps
legacy semantics on writes. That reverses this PR's choice that such a write
fails with the ANSI error, though it is arguably closer to what the DDL meant.
- It is a larger change. This PR could land first as the fallback, with the
stored configs as a follow-up.
[SPARK-60453](https://issues.apache.org/jira/browse/SPARK-60453) asks the
same question for CHECK constraints ("whether to store the confs with the
constraint"), so one stored-conf mechanism could serve both.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/StoredExpressionText.scala:
##########
@@ -0,0 +1,78 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.spark.sql.catalyst.util
+
+import org.apache.spark.sql.catalyst.analysis.{Analyzer,
ExpressionWithUnresolvedIdentifier}
+import org.apache.spark.sql.catalyst.expressions.{Alias, Expression, ExprId}
+import org.apache.spark.sql.catalyst.parser.{ParseException, ParserInterface}
+import org.apache.spark.sql.internal.SQLConf
+
+/**
+ * Reads the SQL text of an expression stored with a table, such as a CHECK
condition or a
+ * generation expression, independently of the settings of the session that
reads it.
Review Comment:
**Finding 13.** CHECK conditions don't go through this object.
`ResolveTableConstraints.buildCatalystExpression` still parses them with the
session's parser settings, which
[SPARK-60453](https://issues.apache.org/jira/browse/SPARK-60453) tracks.
```suggestion
* Reads the SQL text of a generation expression stored with a table,
independently of the
* settings of the session that reads it.
```
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/StoredExpressionText.scala:
##########
@@ -0,0 +1,78 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.spark.sql.catalyst.util
+
+import org.apache.spark.sql.catalyst.analysis.{Analyzer,
ExpressionWithUnresolvedIdentifier}
+import org.apache.spark.sql.catalyst.expressions.{Alias, Expression, ExprId}
+import org.apache.spark.sql.catalyst.parser.{ParseException, ParserInterface}
+import org.apache.spark.sql.internal.SQLConf
+
+/**
+ * Reads the SQL text of an expression stored with a table, such as a CHECK
condition or a
+ * generation expression, independently of the settings of the session that
reads it.
+ */
+object StoredExpressionText {
+
+ /**
+ * The settings a stored text is read under. The session time zone and case
sensitivity stay with
+ * the given session, because the settings of the DDL are not known here and
pinning them would
+ * silently change existing tables of a non-UTC or a case-sensitive
deployment.
+ */
+ def conf(session: SQLConf): SQLConf = {
+ val c = new SQLConf
+ Analyzer.retainResolutionConfigsForAnalysis(newConf = c, existingConf =
session)
+ c.setConfString(SQLConf.ANSI_ENABLED.key, "true")
+ c.setConfString(SQLConf.SESSION_LOCAL_TIMEZONE.key,
session.sessionLocalTimeZone)
+ c.setConfString(SQLConf.CASE_SENSITIVE.key,
session.caseSensitiveAnalysis.toString)
+ c
+ }
+
+ /** Parses `text` under [[conf]]. */
+ def parse(parser: ParserInterface, text: String): Expression = {
+ SQLConf.withExistingConf(conf(SQLConf.get)) {
+ parser.parseExpression(text)
+ }
+ }
+
+ /**
+ * Whether `text` reads the same under [[conf]] as it does in the active
session. The comparison
+ * uses the ANSI value of the session, so that it is about how the text
reads.
+ */
+ def readsAsInSession(parser: ParserInterface, text: String): Boolean = {
+ val session = SQLConf.get
+ val fixed = conf(session)
+ fixed.setConfString(SQLConf.ANSI_ENABLED.key, session.ansiEnabled.toString)
Review Comment:
**Finding 8.** The parser builds the arithmetic nodes with a
`NumericEvalContext`, which also captures
`spark.sql.decimalOperations.allowPrecisionLoss`. `conf` resets that setting to
its default. So with the setting off, every generation expression with `+`,
`-`, `*`, `/` or `%` counts as reading differently:
```sql
SET spark.sql.decimalOperations.allowPrecisionLoss=false;
CREATE TABLE t (a INT, b INT GENERATED ALWAYS AS (a + 1));
-- base: accepted; head: UNSUPPORTED_EXPRESSION_GENERATED_COLUMN (reads
differently)
```
Aligning it like ANSI fixes this. With the change below the CREATE succeeds,
and the full `GeneratedColumnWriteSuite` and `DataSourceV2DataFrameSuite` still
pass. A decimal expression still analyzes with the default on a write. The
second option in finding 7 would cover that. A test with the CREATE above would
pin it.
```suggestion
fixed.setConfString(SQLConf.ANSI_ENABLED.key,
session.ansiEnabled.toString)
fixed.setConfString(SQLConf.DECIMAL_OPERATIONS_ALLOW_PREC_LOSS.key,
session.decimalOperationsAllowPrecisionLoss.toString)
```
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/StoredExpressionText.scala:
##########
@@ -0,0 +1,78 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.spark.sql.catalyst.util
+
+import org.apache.spark.sql.catalyst.analysis.{Analyzer,
ExpressionWithUnresolvedIdentifier}
+import org.apache.spark.sql.catalyst.expressions.{Alias, Expression, ExprId}
+import org.apache.spark.sql.catalyst.parser.{ParseException, ParserInterface}
+import org.apache.spark.sql.internal.SQLConf
+
+/**
+ * Reads the SQL text of an expression stored with a table, such as a CHECK
condition or a
+ * generation expression, independently of the settings of the session that
reads it.
+ */
+object StoredExpressionText {
+
+ /**
+ * The settings a stored text is read under. The session time zone and case
sensitivity stay with
+ * the given session, because the settings of the DDL are not known here and
pinning them would
+ * silently change existing tables of a non-UTC or a case-sensitive
deployment.
+ */
+ def conf(session: SQLConf): SQLConf = {
+ val c = new SQLConf
+ Analyzer.retainResolutionConfigsForAnalysis(newConf = c, existingConf =
session)
+ c.setConfString(SQLConf.ANSI_ENABLED.key, "true")
+ c.setConfString(SQLConf.SESSION_LOCAL_TIMEZONE.key,
session.sessionLocalTimeZone)
+ c.setConfString(SQLConf.CASE_SENSITIVE.key,
session.caseSensitiveAnalysis.toString)
+ c
+ }
+
+ /** Parses `text` under [[conf]]. */
+ def parse(parser: ParserInterface, text: String): Expression = {
+ SQLConf.withExistingConf(conf(SQLConf.get)) {
Review Comment:
**Finding 11.** With `spark.sql.path.enabled=true`, `SET PATH` can still put
a temporary function before a built-in function of a generation expression. I
ran this with a temporary `upper` that appends `-temp`:
```sql
CREATE TABLE t (id INT, a STRING, b STRING GENERATED ALWAYS AS (upper(a)));
SET PATH = system.session, system.builtin;
INSERT INTO t (id, a) VALUES (1, 'x'); -- b = 'x-temp'
INSERT INTO t (id, a, b) VALUES (2, 'y', 'Y'); -- CHECK_CONSTRAINT_VIOLATION
```
Base does the same, and the description lists it as still depending on the
writer. But SPARK-60777 added `ResolveTableConstraints.qualifyBuiltinFunctions`
for the same problem in CHECK conditions, and it fits here too. Applying the
same rewrite to the result of `parse` covers both the auto-fill and the check.
With it, the run above stores `X` and accepts `Y`. The full
`GeneratedColumnWriteSuite` and `DataSourceV2DataFrameSuite` still pass. CREATE
already rejects user-defined functions in a generation expression, so binding
every built-in name can't break a valid table. Moving the helper into
`StoredExpressionText` would let both rules share it. The `SET PATH` line in
the description could then go.
With a SQL temporary function (`CREATE TEMPORARY FUNCTION upper(s STRING)
RETURNS STRING RETURN 'shadowed'`), the provided-value INSERT fails with "Max
iterations (100) reached for batch Resolution" instead, on base too. That is a
separate bug, which may be worth a ticket.
--
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]
