peter-toth commented on code in PR #57725: URL: https://github.com/apache/spark/pull/57725#discussion_r3712984757
########## docs/sql-ref-syntax-dml-delete-from.md: ########## @@ -0,0 +1,101 @@ +--- +layout: global +title: DELETE FROM +displayTitle: DELETE FROM +license: | + 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. +--- + +### Description + +The `DELETE FROM` statement removes rows from a table that satisfy an optional condition. When no +condition is specified, every row is removed. + +`DELETE FROM` is supported on tables backed by +[Data Source V2](sql-v2-data-sources.html#row-level-dml) connectors that support delete operations. + +### Syntax + +```sql +DELETE FROM table_identifier [ [ AS ] table_alias ] + [ WITH ( key = value [ , ... ] ) ] + [ WHERE boolean_expression ] +``` + +### Parameters + +* **table_identifier** + + Specifies the table from which rows are deleted. The table name may be optionally qualified + with a database name. + + **Syntax:** `[ database_name. ] table_name` + +* **table_alias** + + Specifies an optional alias for the target table. The alias may be introduced with or without + the `AS` keyword. + +* **WITH ( key = value [ , ... ] )** + + Specifies an optional list of dynamic table options passed to the Data Source V2 connector for + this statement only. The options allow per-statement tuning without changing the table's + persistent configuration. Keys and values are treated as strings; a key that is not a valid + identifier can be quoted with backticks. Spark passes options through without validating their + names, and connectors may ignore options they do not recognize. + +* **WHERE boolean_expression** + + Specifies an optional condition that selects the rows to delete. If the `WHERE` clause is + omitted, all rows are deleted. Review Comment: **Finding 2.** The page presents a single `DELETE FROM`, but there are two execution paths with different rules for the condition, and the stricter one isn't mentioned. When the table supports only filter-based delete (`SupportsDeleteV2` / `SupportsDelete` without `SupportsRowLevelOperations`), `RewriteDeleteFromTable.scala:55-56` leaves the command alone and `DataSourceV2Strategy.scala:533-552` then demands all three of: no subquery in the condition, every conjunct translatable into a V2 `Predicate`, and `canDeleteWhere` accepting the resulting set. Any one failing is a hard error at planning, not a fallback to rewriting. Two of the three are already covered by tests against `InMemoryTable`, which is filter-only (`InMemoryTable.scala:53`): `DeleteFromTests.scala:74` -- `DELETE FROM t WHERE id IN (SELECT id FROM t)` fails with "Delete by condition with subquery is not supported"; `DeleteFromTests.scala:88` and `DataSourceV2SQLSuite.scala:2578` -- `WHERE id > 3 AND p > 3` on a non-partitioned table fails with "Cannot delete from table" because `canDeleteWhere` returns `false`. The untranslatable-conjunct branch (`DataSourceV2Strategy.scala:544-545`) I only traced in code. Every example on this page is a simple predicate on the target table, so a reader has no way to learn that `DELETE FROM t WHERE id IN (SELECT ...)` works on one connector and is a planning error on another. ```suggestion Specifies an optional condition that selects the rows to delete. If the `WHERE` clause is omitted, all rows are deleted. Connectors that support only filter-based deletes place extra restrictions on the condition: it must not contain a subquery, and every conjunct must be convertible into a data source predicate that the connector accepts -- a connector may, for instance, accept predicates only on partition columns. If either does not hold, the statement fails when the query is planned. Connectors that support row-level operations have no such restriction. ``` One aside on the page you link to: `sql-v2-data-sources.md:409` says a `false` from `canDeleteWhere` makes Spark "fall back to row-level rewriting", which only holds if the table *also* implements `SupportsRowLevelOperations` -- a separate fix, not this PR. ########## docs/sql-ref-syntax-dml-delete-from.md: ########## @@ -0,0 +1,101 @@ +--- +layout: global +title: DELETE FROM +displayTitle: DELETE FROM +license: | + 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. +--- + +### Description + +The `DELETE FROM` statement removes rows from a table that satisfy an optional condition. When no +condition is specified, every row is removed. + +`DELETE FROM` is supported on tables backed by +[Data Source V2](sql-v2-data-sources.html#row-level-dml) connectors that support delete operations. + +### Syntax + +```sql +DELETE FROM table_identifier [ [ AS ] table_alias ] + [ WITH ( key = value [ , ... ] ) ] + [ WHERE boolean_expression ] +``` + +### Parameters + +* **table_identifier** + + Specifies the table from which rows are deleted. The table name may be optionally qualified + with a database name. + + **Syntax:** `[ database_name. ] table_name` Review Comment: **Finding 1.** Same as on the `UPDATE` page (`docs/sql-ref-syntax-dml-update.md:46`): a V2 table is normally reached as `catalog.db.table`, and `identifierReference` (`SqlBaseParser.g4:750`) accepts any number of name parts. ```suggestion Specifies the table from which rows are deleted. The table name may be optionally qualified with a catalog and a database name. **Syntax:** `[ catalog_name. ] [ database_name. ] table_name` ``` ########## docs/sql-ref-syntax-dml-update.md: ########## @@ -0,0 +1,113 @@ +--- +layout: global +title: UPDATE +displayTitle: UPDATE +license: | + 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. +--- + +### Description + +The `UPDATE` statement changes the values of columns in rows that satisfy an optional condition. +When no condition is specified, every row is updated. + +`UPDATE` is supported on tables backed by +[Data Source V2](sql-v2-data-sources.html#row-level-dml) connectors that support row-level +operations. + +### Syntax + +```sql +UPDATE table_identifier [ [ AS ] table_alias ] + [ WITH ( key = value [ , ... ] ) ] + SET column = value [ , ... ] + [ WHERE boolean_expression ] +``` + +### Parameters + +* **table_identifier** + + Specifies the table to update, which may be optionally qualified with a database name. + + **Syntax:** `[ database_name. ] table_name` Review Comment: **Finding 1.** `UPDATE` only works on Data Source V2 tables, and a V2 table normally lives in a catalog registered through `spark.sql.catalog.*` rather than in the session catalog -- so the form users actually type is `catalog.db.table`, which this syntax doesn't cover. The grammar allows it: `identifierReference` resolves to `multipartIdentifier` (`SqlBaseParser.g4:751`), so any number of name parts is accepted. Several reference pages already use the fuller form, e.g. `docs/sql-ref-syntax-ddl-drop-view.md:44` and `docs/sql-ref-syntax-aux-describe-function.md:52`. ```suggestion Specifies the table to update, which may be optionally qualified with a catalog and a database name. **Syntax:** `[ catalog_name. ] [ database_name. ] table_name` ``` Same on `docs/sql-ref-syntax-dml-delete-from.md:45`. ########## docs/sql-ref-syntax-dml-update.md: ########## @@ -0,0 +1,113 @@ +--- +layout: global +title: UPDATE +displayTitle: UPDATE +license: | + 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. +--- + +### Description + +The `UPDATE` statement changes the values of columns in rows that satisfy an optional condition. +When no condition is specified, every row is updated. + +`UPDATE` is supported on tables backed by +[Data Source V2](sql-v2-data-sources.html#row-level-dml) connectors that support row-level +operations. Review Comment: **Finding 4.** `RewriteUpdateTable.scala:43` calls `checkNoGeneratedColumns(r, UPDATE)`, which fails the statement with `UNSUPPORTED_FEATURE.TABLE_OPERATION` ("... does not support UPDATE TABLE with generated columns") when the table has a generated column *and* declares `GENERATE_COLUMN_VALUES_ON_WRITE`, i.e. asks Spark to compute the value. The comment above `RewriteRowLevelCommand.scala:61` says it's a deliberate fail-fast until recomputation is implemented. Since this sentence is where the page says when `UPDATE` works, one clause here saves a user the error. `MERGE INTO`'s page has the same gap, so a follow-up covering both is fine if you'd rather keep them consistent. ```suggestion `UPDATE` is supported on tables backed by [Data Source V2](sql-v2-data-sources.html#row-level-dml) connectors that support row-level operations. Tables with generated columns whose values Spark computes are not supported, because Spark cannot yet recompute those values for rewritten rows. ``` ########## docs/sql-ref-syntax-dml-update.md: ########## @@ -0,0 +1,113 @@ +--- +layout: global +title: UPDATE +displayTitle: UPDATE +license: | + 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. +--- + +### Description + +The `UPDATE` statement changes the values of columns in rows that satisfy an optional condition. +When no condition is specified, every row is updated. + +`UPDATE` is supported on tables backed by +[Data Source V2](sql-v2-data-sources.html#row-level-dml) connectors that support row-level +operations. + +### Syntax + +```sql +UPDATE table_identifier [ [ AS ] table_alias ] + [ WITH ( key = value [ , ... ] ) ] + SET column = value [ , ... ] + [ WHERE boolean_expression ] +``` + +### Parameters + +* **table_identifier** + + Specifies the table to update, which may be optionally qualified with a database name. + + **Syntax:** `[ database_name. ] table_name` + +* **table_alias** + + Specifies an optional alias for the target table. The alias may be introduced with or without + the `AS` keyword. + +* **WITH ( key = value [ , ... ] )** + + Specifies an optional list of dynamic table options passed to the Data Source V2 connector for + this statement only. The options allow per-statement tuning without changing the table's + persistent configuration. Keys and values are treated as strings; a key that is not a valid + identifier can be quoted with backticks. Spark passes options through without validating their + names, and connectors may ignore options they do not recognize. + +* **SET column = value [ , ... ]** + + Specifies the columns to update and the values to assign to them. Each `value` is an expression, + typically referencing columns of the target table, but it may also be `DEFAULT` or an + uncorrelated scalar subquery over another table. A comma separates each assignment. A nested + field may be targeted by using a qualified column name. Review Comment: **Finding 3.** Each claim here is individually true, but two of them carry a restriction that changes what a reader can write. `DEFAULT` is only resolved when it is the *whole* assignment value and the key is a top-level column: `ResolveReferencesInUpdate.scala:49-60` passes `defaultReferencesNotAllowedInComplexExpressionsInUpdateSetClause` as the error for anything else, so `SET salary = DEFAULT + 1` is rejected. "A nested field may be targeted by using a qualified column name" reads as the alias-qualified `e.salary` in the example right below, which is a top-level column, not a nested field. And nested targeting only goes through structs -- `AssignmentUtils.scala:231` errors with "Updating nested fields is only supported for StructType" for a field inside an array or a map. Two more rules belong in this bullet, both enforced in `AssignmentUtils.scala:175` and `:180`: a column may be assigned at most once, and a column and one of its nested fields cannot both be assigned. And since `RewriteUpdateTable.scala:75` computes every assignment value in one projection over the pre-update row, `SET a = b, b = a` swaps the two columns rather than setting both to the old `b` -- exactly the kind of thing a reference page should pin down. ```suggestion Specifies the columns to update and the values to assign to them. Each `value` is an expression, typically referencing columns of the target table, but it may also be `DEFAULT` or an uncorrelated scalar subquery over another table. `DEFAULT` must be the whole value; it cannot appear inside a larger expression. A comma separates each assignment. Every value is evaluated against the row as it was before the update, so `SET a = b, b = a` swaps the two columns. A column may be assigned at most once, and a column and one of its nested fields cannot both be assigned in the same statement. A field of a struct column may be targeted with a dotted path, for example `SET address.city = 'Berlin'`; fields nested inside an array or a map cannot be. ``` -- 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]
