ethan-tyler commented on code in PR #19884:
URL: https://github.com/apache/datafusion/pull/19884#discussion_r2719472531
##########
datafusion/sqllogictest/test_files/update.slt:
##########
@@ -81,8 +81,22 @@ physical_plan
01)CooperativeExec
02)--DmlResultExec: rows_affected=0
+# test update from other table with actual data
statement ok
-create table t3(a int, b varchar, c double, d int);
+insert into t1 values (1, 'zoo', 2.0, 10), (2, 'qux', 3.0, 20), (3, 'bar',
4.0, 30);
+
+statement ok
+insert into t2 values (1, 'updated_b', 5.0, 40), (2, 'updated_b2', 2.5, 50),
(4, 'updated_b3', 1.5, 60);
+
+statement ok
+update t1 set b = t2.b, c = t2.a, d = 1 from t2 where t1.a = t2.a and t1.b >
'foo' and t2.c > 1.0;
Review Comment:
This expects `SET b = t2.b` to copy values from `t2` into `t1`. With current
`TableProvider::update()` and qualifier stripping, cross-table assignments
can't be evaluated against the target provider schema.
Belongs in a separate PR implementing UPDATE…FROM execution or explicitly
rejecting cross-table assignments. Otherwise this will fail under the DF engine.
##########
datafusion/sqllogictest/test_files/update.slt:
##########
@@ -81,8 +81,22 @@ physical_plan
01)CooperativeExec
02)--DmlResultExec: rows_affected=0
+# test update from other table with actual data
Review Comment:
Removing `CREATE TABLE t3` here breaks the multi-table negative test at :103
(`FROM t2, t3`).
Now fails with "table not found" instead of "Multiple tables… not
supported". Re-add `t3` setup to keep that test valid.
##########
datafusion/sqllogictest/test_files/update.slt:
##########
@@ -96,10 +110,33 @@ logical_plan
01)Dml: op=[Update] table=[t1]
02)--Projection: t.a AS a, t2.b AS b, CAST(t.a AS Float64) AS c, CAST(Int64(1)
AS Int32) AS d
03)----Filter: t.a = t2.a AND t.b > CAST(Utf8("foo") AS Utf8View) AND t2.c >
Float64(1)
-04)------Cross Join:
+04)------Cross Join:
05)--------SubqueryAlias: t
06)----------TableScan: t1
07)--------TableScan: t2
physical_plan
01)CooperativeExec
-02)--DmlResultExec: rows_affected=0
+02)--DmlResultExec: rows_affected=1
+
+# test update with table alias with actual data
+statement ok
+delete from t1;
+
+statement ok
+delete from t2;
+
+statement ok
+insert into t1 values (1, 'zebra', 1.5, 5), (2, 'wolf', 2.0, 10), (3, 'apple',
3.5, 15);
+
+statement ok
+insert into t2 values (1, 'new_val', 2.0, 100), (2, 'new_val2', 1.5, 200);
+
+statement ok
+update t1 as T set b = t2.b, c = t.a, d = 1 from t2 where t.a = t2.a and t.b >
'foo' and t2.c > 1.0;
Review Comment:
Same concern- cross-table assignment (`SET b = t2.b`) can't be evaluated
with current execution model. Should be EXPLAIN only or deferred to a follow up
PR.
--
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]
