spark-test commented on code in PR #59739:
URL: https://github.com/apache/spark/pull/59739#discussion_r4356259452
##########
sql/connect/server/src/test/scala/org/apache/spark/sql/connect/pipelines/PythonPipelineSuite.scala:
##########
@@ -1632,6 +1632,12 @@ class PythonPipelineSuite
"CREATE TABLE some_table (id INT)",
"CREATE VIEW some_view AS SELECT * FROM some_table",
"INSERT INTO some_table VALUES (1)",
+ "WITH v AS (SELECT 1 AS id) INSERT INTO some_table SELECT * FROM v",
+ "WITH v AS (SELECT 1 AS id) MERGE INTO some_table t USING v ON t.id = v.id
" +
+ "WHEN MATCHED THEN DELETE",
+ "WITH v AS (SELECT 1 AS id) UPDATE some_table SET id = 2 WHERE id IN
(SELECT id FROM v)",
+ "WITH v AS (SELECT 1 AS id) DELETE FROM some_table WHERE id IN (SELECT id
FROM v)",
+ "FROM some_view INSERT INTO some_table SELECT * INSERT INTO some_new_table
SELECT *",
Review Comment:
**Nit (P3):** None of the new entries combines the two wrappers. A
CTE-prefixed multi-insert parses to `UnresolvedWith(Union(UnresolvedInsert,
UnresolvedInsert))`, and it is the only DML shape where the new arms must call
`blockUnsupportedSqlCommand` recursively. Every other new entry has a blocked
node directly under the wrapper. So if the recursion became a one-level unwrap
that checks only `w.child` or each `u.children` element, all four grid tests
would still pass, and `WITH v AS (SELECT 1 AS id) FROM v INSERT INTO ... INSERT
INTO ...` would bypass the guard again. Adding that statement here covers both
the outside-flow and inside-flow cases:
```suggestion
"FROM some_view INSERT INTO some_table SELECT * INSERT INTO
some_new_table SELECT *",
"WITH v AS (SELECT 1 AS id) FROM v INSERT INTO some_table SELECT * " +
"INSERT INTO some_new_table SELECT *",
```
**Verification:**
- **Behavior:** Run the CTE-prefixed multi-insert through spark.sql in a
pipeline definition, both at module level and inside a flow function, and
assert that each attempt fails with UNSUPPORTED_PIPELINE_SPARK_SQL_COMMAND.
- **Regression:** Confirm the new entry depends on the recursion: with a
check that unwraps only one level (it inspects the direct child of the CTE
wrapper or of the multi-insert union, but does not recurse), the CTE-prefixed
multi-insert passes the guard and its test fails, while the existing
single-wrapper entries still pass.
--
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]
