peter-toth commented on code in PR #60075:
URL: https://github.com/apache/spark/pull/60075#discussion_r4465472532
##########
docs/sql-migration-guide.md:
##########
@@ -74,6 +74,7 @@ license: |
- 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 scalar function, including a Data Source V2
function. `ALTER TABLE ... ADD CONSTRAINT` keeps failing with
`UNSUPPORTED_SQL_UDF_USAGE` on a SQL 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, `CREATE TABLE` and `REPLACE TABLE` on a Data Source V2
table fail with `AGGREGATE_OR_WINDOW_FUNCTION_IN_CHECK_CONSTRAINT` when the
condition of a CHECK constraint calls an aggregate or window function,
including a user-defined aggregate function, `grouping`, and `grouping_id`.
`ALTER TABLE ... ADD CONSTRAINT` keeps failing with its existing errors, such
as `INVALID_WHERE_CONDITION` and `WINDOW_FUNCTION_NOT_ALLOWED_IN_CLAUSE`.
Previously, these statements succeeded and every later write to the table
failed (SPARK-60800). To run these statements, remove the function from the
condition. Existing constraints that call these functions are not rejected.
- Since Spark 4.4, `CREATE TABLE` and `REPLACE TABLE` on a Data Source V2
table fail with `GENERATOR_IN_CHECK_CONSTRAINT` when the condition of a CHECK
constraint calls a generator function, such as `explode`, `posexplode`, and
`inline`. `ALTER TABLE ... ADD CONSTRAINT` keeps failing with
`UNSUPPORTED_GENERATOR.OUTSIDE_SELECT`. Previously, these statements succeeded
and every later write to the table failed (SPARK-60821). To run these
statements, remove the function from the condition. Existing constraints that
call a generator function are not rejected.
+- Since Spark 4.4, `CREATE TABLE` and `REPLACE TABLE` fail with
`UNSUPPORTED_EXPRESSION_GENERATED_COLUMN` when the data type of a generation
expression names its struct fields differently from the column type, in name or
in order, for example `named_struct('y', a, 'x', b)` or `struct(a, b)` for a
`STRUCT<x: INT, y: INT>` column. Previously, such an expression was accepted,
although Spark lines up the struct fields by name when it computes the value of
a generated column of a Data Source V2 table, and by position when it checks a
provided value (SPARK-60857). To line up the fields by position, cast the
expression to the column type, for example `CAST(struct(a, b) AS STRUCT<x: INT,
y: INT>)`.
Review Comment:
**Finding 1.** The PR description says auto-filling and checking generated
columns on write "are not in a released version, so no released version stored
values this way". Both are in 4.3.0. SPARK-57644 added the auto-fill, and
SPARK-58311 added the `GENERATE_COLUMN_VALUES_ON_WRITE` capability (`@since
4.3.0`). On `v4.3.0`, with the table of the description:
```sql
CREATE TABLE testcat.t (a INT, b INT,
s STRUCT<x: INT, y: INT> GENERATED ALWAYS AS (named_struct('y', a, 'x',
b))) USING foo; -- OK
INSERT INTO testcat.t (a, b) VALUES (1, 2); -- stores [1, 2, [2, 1]]
INSERT INTO testcat.t SELECT * FROM testcat.t; -- CHECK_CONSTRAINT_VIOLATION
```
So a 4.3.0 table can already hold such an auto-filled row. This PR does not
reject that table, since `validate` runs only on `CREATE TABLE` and `REPLACE
TABLE`, and the write path is unchanged. Could you fix the user-facing change
section? This entry could also say what happens to an existing table, as the
entries above say "Existing constraints that ... are not rejected". For
example: such an existing table is not rejected, and a write that carries a row
Spark auto-filled into it keeps failing the check.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/GeneratedColumnExpression.scala:
##########
@@ -120,6 +121,14 @@ case class GeneratedColumnExpression(
s"is incompatible with column data type
${targetDataType.simpleString}")
}
+ // Store assignment lines up the struct fields of an auto-filled value
with the column type by
+ // name, while the check of a provided value lines them up by position, so
the two must agree.
+ if (!DataType.equalsStructurallyByName(child.dataType, targetDataType,
resolver)) {
Review Comment:
**Finding 2.** `resolver` here is the definer's, but store assignment
matches the fields by name under the writer's session. So under the default
case-insensitive resolver this accepts `named_struct('X', b, 'Y', a)` for a
`STRUCT<x: INT, y: INT>` column. A writer with `spark.sql.caseSensitive=true`
then can't auto-fill it:
```sql
-- on master with this PR, in a case-insensitive session
CREATE TABLE testcat.t (a INT, b INT,
s STRUCT<x: INT, y: INT> GENERATED ALWAYS AS (named_struct('X', b, 'Y',
a))) USING foo;
-- in a session with spark.sql.caseSensitive=true
INSERT INTO testcat.t (a, b) VALUES (1, 2); --
INCOMPATIBLE_DATA_FOR_TABLE.CANNOT_FIND_DATA
```
This is the "other field names" case that the description says this check
leaves out. 4.3.0 behaves the same, so it predates the PR. Comparing the names
exactly accepts only names that line up for every writer:
```suggestion
if (!DataType.equalsStructurallyByName(child.dataType, targetDataType, _
== _)) {
```
With it, `CREATE TABLE` rejects the expression in both sessions. The suites
listed in the description pass with it on master: `GeneratedColumnWriteSuite`,
`DataSourceV2SQLSuiteV1Filter`, `DataSourceV2SQLSuiteV2Filter`,
`DataSourceV2DataFrameSuite`, `CollationSuite`, and the four `catalyst` ones.
The row for `named_struct('X', b, 'Y', a)` in the description's table would
then become "rejected".
--
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]
