fix dml logical plan output schema #10394
Conversation
Previously, `LogicalPlan::schema` would return the input schema for Dml plans, rather than the expected output schema. This is an unusal case since Dmls are typically not run for their output, but it is typical for the output to be the `count` of rows affected by the DML statement. See `fn dml_output_schema` for a test.
github-actions
bot
added
sql
logical-expr
| @@ -58,6 +58,19 @@ async fn unsupported_dml_returns_error() { | |||
| ctx.sql_with_options(sql, options).await.unwrap(); | |||
| } | |||
|
|
|||
| #[tokio::test] | |||
There was a problem hiding this comment.
its better to have this test in one of .slt files which runs end to end tests
There was a problem hiding this comment.
I suspect this might not be caught in a .slt test as this issue is not visible in the pretty-printed logical plan, and doesn't affect the physical plan or execution at all, afaict.
There was a problem hiding this comment.
Actually I see some slt tests failing on CI, I should look into that.
There was a problem hiding this comment.
Curiously, this was in fact tested in many sqllogictests. Because the tests do in fact cover checking that the physical output schema matches the logical output schema, which is pretty cool.
Let me know if you want to keep this Rust test as well, or if you rather that I remove it. It does test more behaviour than what the slts can test.
There was a problem hiding this comment.
I think keeping the rust level test is a good idea as it makes explicit the expected output schema
|
I checked current behavior I'm not even sure why we output anything after DDL/DML... I'd probably prefer no output like in duckdb |
|
@comphead Thank you for your review, I have addressed the outstanding comments. I have no opinion on desired behaviour, this PR is just trying to make things consistent. |
| @@ -259,7 +259,7 @@ statement error Error during planning: Column count doesn't match insert query! | |||
| insert into table_without_values(id) values(4, 'zoo'); | |||
|
|
|||
| # insert NULL values for the missing column (name) | |||
| query IT | |||
| query I | |||
There was a problem hiding this comment.
does it fail with old value?
| @@ -58,6 +58,19 @@ async fn unsupported_dml_returns_error() { | |||
| ctx.sql_with_options(sql, options).await.unwrap(); | |||
| } | |||
|
|
|||
| #[tokio::test] | |||
There was a problem hiding this comment.
I think keeping the rust level test is a good idea as it makes explicit the expected output schema
| @@ -3260,7 +3260,7 @@ SELECT STRING_AGG(column1, '|') FROM (values (''), (null), ('')); | |||
| statement ok | |||
| CREATE TABLE strings(g INTEGER, x VARCHAR, y VARCHAR) | |||
|
|
|||
| query ITT | |||
| query I | |||
Previously,
LogicalPlan::schemawould return the input schema for DML plans, rather than the expected output schema. It is typical for the output to be thecountof rows affected by the DML statement, so the code assumes that.See
fn dml_output_schemafor a test.Which issue does this PR close?
Closes #10393.
Rationale for this change
Current behaviour is wrong.
Are these changes tested?
Yes there is a test
fn dml_output_schema.Are there any user-facing changes?
The bug being fixed is visible to users of the DataFrame API.