feat: support `RETURNING` in DML statements #7094

y-wei · 2022-12-28T04:26:24Z

I hereby agree to the terms of the Singularity Data, Inc. Contributor License Agreement.

What's changed and what's your intention?

This section will be used as the commit message. Please do not leave this empty!

Please explain IN DETAIL what the changes are in this PR and why they are needed:

Summarize your change (mandatory)
- add one returning field in DML related structs ( INSERT , UPDATE, and DELETE)
- corresponding executor and query handler will alter output format if returning included.
Additional side change
~~run_sql will return an extra bool to indicate if the statement is a DML with returning, used by query handler.~~

Checklist

I have written necessary rustdoc comments
I have added necessary unit tests and integration tests
All checks passed in ./risedev check (or alias, ./risedev c)

Types of user-facing changes

Please keep the types that apply to your changes, and remove those that do not apply.

SQL commands, functions, and operators

Refer to a related PR or issue link (optional)

#7044

src/batch/src/executor/insert.rs

src/sqlparser/src/parser.rs

src/sqlparser/src/ast/mod.rs

src/frontend/src/session.rs

src/frontend/src/binder/insert.rs

proto/batch_plan.proto

src/utils/pgwire/src/pg_protocol.rs

src/frontend/src/binder/insert.rs

BugenZhao · 2022-12-29T07:30:04Z

src/frontend/src/handler/query.rs

-    let rows_count = match stmt_type {
-        StatementType::SELECT => None,
+    let rows_count: Option<i32> = match stmt_type {
+        StatementType::SELECT | StatementType::INSERT_RETURNING | StatementType::DELETE_RETURNING | StatementType::UPDATE_RETURNING => None,


Actually, in Postgres, we will get both returning rows and affected row count. We may fix this in the future.

postgres=# insert into t values (233) returning *; v ----- 233 (1 row) INSERT 0 1

Any ideas? 😰 cc @BowenXiao1999

I'm also not sure, but this seems do not affect the correctness.

If the protocol allows, I think we can add the values in future. 🤔

Have fixed!

src/frontend/src/planner/insert.rs

BugenZhao

Rest LGTM. Great job!

src/frontend/src/binder/select.rs

BugenZhao · 2022-12-30T11:24:45Z

src/frontend/src/binder/insert.rs

    pub returning_list: Vec<ExprImpl>,

-    pub schema: Schema,
+    pub returning_schema: Option<Schema>,


How about adding some brief doc for this? 🥰 Same for other occurrences.

src/frontend/src/binder/select.rs

BugenZhao · 2022-12-30T11:27:48Z

src/frontend/planner_test/tests/testdata/insert.yaml

+    insert into t values (0,1), (1,2) returning *, a, a+b;
+  logical_plan: |
+    LogicalProject { exprs: [*VALUES*_0.column_0, *VALUES*_0.column_1, *VALUES*_0.column_0, (*VALUES*_0.column_0 + *VALUES*_0.column_1)] }
+    └─LogicalInsert { table: t }


It'll be better if we can add a new field of returning in fmt::Display. Only show it when returning is true.

One problem is that current we do not hold returning projection info in LogicalInsert/Delete/Update struct but just a bool indicating if there were returning. Do you think it will be better if we print LogicalInsert { table: t, returning: true }?

LogicalInsert { table: t, returning: true } looks clear 🤣

BugenZhao · 2022-12-30T11:28:27Z

src/frontend/src/handler/query.rs

-    let rows_count = match stmt_type {
-        StatementType::SELECT => None,
+    let rows_count: Option<i32> = match stmt_type {
+        StatementType::SELECT | StatementType::INSERT_RETURNING | StatementType::DELETE_RETURNING | StatementType::UPDATE_RETURNING => None,


codecov · 2022-12-30T19:41:11Z

Codecov Report

Merging #7094 (fde984e) into main (ce7c724) will decrease coverage by 0.67%.
The diff coverage is 83.52%.

@@            Coverage Diff             @@
##             main    #7094      +/-   ##
==========================================
- Coverage   72.33%   71.66%   -0.67%     
==========================================
  Files        1071     1083      +12     
  Lines      171769   173589    +1820     
==========================================
+ Hits       124243   124401     +158     
- Misses      47526    49188    +1662

Flag	Coverage Δ
rust	`71.66% <83.52%> (-0.67%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
src/frontend/src/handler/create_table_as.rs	`0.00% <0.00%> (ø)`
src/frontend/src/handler/query.rs	`17.70% <0.00%> (-0.88%)`	⬇️
...c/frontend/src/optimizer/plan_node/batch_delete.rs	`80.55% <0.00%> (-2.31%)`	⬇️
...c/frontend/src/optimizer/plan_node/batch_insert.rs	`52.08% <0.00%> (-1.11%)`	⬇️
...c/frontend/src/optimizer/plan_node/batch_update.rs	`59.18% <0.00%> (-1.24%)`	⬇️
src/sqlparser/src/keywords.rs	`100.00% <ø> (ø)`
src/utils/pgwire/src/pg_protocol.rs	`56.16% <ø> (ø)`
src/utils/pgwire/src/pg_response.rs	`71.30% <25.00%> (-2.57%)`	⬇️
src/utils/pgwire/src/pg_message.rs	`69.52% <50.00%> (-0.38%)`	⬇️
src/batch/src/executor/delete.rs	`83.33% <80.00%> (-0.73%)`	⬇️
... and 49 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

y-wei · 2022-12-30T20:20:01Z

Have checked and rebased! Ready to be reviewed!

BowenXiao1999

So now, we are returning the columns for the RETURNING clause and no rows count?

Rest LGTM

BowenXiao1999 · 2023-01-03T03:46:21Z

src/batch/src/executor/insert.rs

        }
+        // } else {
+        //     let ret_chunk = DataChunk::new(returning_columns, rows_inserted);
+        //     yield ret_chunk


chenzl25 · 2023-01-03T03:58:03Z

src/frontend/src/optimizer/plan_node/logical_delete.rs

-    pub fn new(input: PlanRef, table_name: String, table_id: TableId) -> Self {
+    pub fn new(input: PlanRef, table_name: String, table_id: TableId, returning: bool) -> Self {
        let ctx = input.ctx();
        // TODO: support `RETURNING`.


It looks like it is already fully supported, can we remove it?

chenzl25 · 2023-01-03T04:23:15Z

src/utils/pgwire/src/pg_response.rs

+                | StatementType::INSERT_RETURNING
+                | StatementType::DELETE_RETURNING
+                | StatementType::UPDATE_RETURNING


I have a question, as we treat XXX_RETURNING as a query. Once they are used in the CTE and the CTE is used multiple times in a single query, can we reuse the CTE result? Reuse means we execute the query once, while not reuse means we execute the query multiple times and then the dml will be executed multiple times.

Good catch! So we must "share" this Insert and support DAG execution in batch as well? 🥵 Luckily, the parser will reject RETURNING in CTE now.

Indeed, we need to make sure the dml of the RETURNING in CTE will be executed exactly once. That's the PG behavior.

src/batch/src/executor/insert.rs

ZENOTME · 2023-01-03T04:23:52Z

src/frontend/planner_test/tests/testdata/insert.yaml

+    insert into t values (0,1), (1,2) returning *, a, a+b;
+  logical_plan: |
+    LogicalProject { exprs: [*VALUES*_0.column_0, *VALUES*_0.column_1, *VALUES*_0.column_0, (*VALUES*_0.column_0 + *VALUES*_0.column_1)] }
+    └─LogicalInsert { table: t }


LogicalInsert { table: t, returning: true } looks clear 🤣

BugenZhao · 2023-01-03T06:01:48Z

src/batch/src/executor/update.rs

        let data_types = self.child.schema().data_types();
-        let mut builder = DataChunkBuilder::new(data_types, self.chunk_size);
+        let mut builder = DataChunkBuilder::new(data_types.clone(), self.chunk_size);
+        let mut updated_builder = DataChunkBuilder::new(data_types.clone(), self.chunk_size);


Some ideas: the Update with returning should behave like a Project, which does not "buffer" or reorganize the chunks with a builder. 🤔 In this sense, should we directly yield every updated chunk in the execution loop?

…nstead of previous INSERT_RETURNING

y-wei · 2023-01-05T03:00:32Z

Could you please also add some end-to-end tests?

Have added some. Those tests would pass should this change to sqllogictest be merged.

BugenZhao

Rest LGTM. Good work!!

BugenZhao · 2023-01-05T08:51:18Z

src/frontend/src/optimizer/plan_node/logical_delete.rs

+            "{} {{ table: {} }}, {{ returning: {} }}",
+            name, self.table_name, self.returning


There should only be one pair of brackets, like Rust's Debug. Same for other occurrences.

Suggested change

"{} {{ table: {} }}, {{ returning: {} }}",

name, self.table_name, self.returning

"{} {{ table: {}{} }}",

name, self.table_name, if self.returning { ", returning: true" } else { "" }

src/utils/pgwire/src/pg_message.rs

BugenZhao · 2023-01-05T08:58:07Z

src/utils/pgwire/src/pg_protocol.rs

                || lower_sql.starts_with("with")
                || lower_sql.starts_with("describe")
                || lower_sql.starts_with("explain")
+                || lower_sql.contains("returning")


Will this be too hacky? ~~But as we make RETURNING a reserved word and do not support RETURNING in CTE, it seems okay.~~ 🤔 cc @BowenXiao1999

risinglightdb/sqllogictest-rs#141 (comment)

directly operating on str is always hacky. For example, I guess insert into t values ('returning') will be bad for this case. starts_with looks no bad. Anyway I don't quite like process str directly before use Parser, but it's seems like non-trival to avoid. let's see whether we can remove all is_query_sql in future (by introduce Parser AST?, not sure).

cc @xxchan not sure whether has better ideas?

This reminds me of #6849 (comment) 🤪 So we are really going to introduce parser in pgwire..

directly operating on str is always hacky.

+1. For sqllogictest, this is good enough. But in pgwire, we should be more careful.

Should I revert this change?

Yes, I think we can revert these changes that is not must-required in this pr, and add some comments to explain that it miss the ability to process Returning keywords.

BugenZhao · 2023-01-05T09:01:01Z

Could you please also add some end-to-end tests?

Have added some. Those tests would pass should this change to sqllogictest be merged.

Remember to bump the docker image version and sqllogictest version after you do this. 🥰

risingwave/ci/build-ci-image.sh

Lines 13 to 17 in 3a8f9d5

    
           ###################################################### 
        
           # !!! CHANGE THIS WHEN YOU WANT TO BUMP CI IMAGE !!! # 
        
           #          AND ALSO docker-compose.yml               # 
        
           ###################################################### 
        
           export BUILD_ENV_VERSION=v20221212

risingwave/ci/Dockerfile

Line 31 in 3a8f9d5

    
           && cargo install --git https://github.com/risinglightdb/sqllogictest-rs --rev 65b122f --bin sqllogictest \

y-wei · 2023-01-09T22:26:58Z

e2e tests have been commented out (blocked by #7277). This pr should be able to merge!

Signed-off-by: Eurekaaw <[email protected]>

Signed-off-by: Bugen Zhao <[email protected]>

Signed-off-by: Eurekaaw <[email protected]>

BugenZhao

We may also need to bump the sqllogictest library version.

risingwave/src/tests/simulation/Cargo.toml

Line 31 in 6194527

sqllogictest = "0.9"

BugenZhao · 2023-01-17T06:27:08Z

src/frontend/planner_test/tests/testdata/insert.yaml

  batch_plan: |
    BatchExchange { order: [], dist: Single }
-    └─BatchInsert { table: t }
+    └─BatchInsert { table: t  }


Please remove the trailing space here. Same for Update and Delete.

BugenZhao · 2023-01-17T06:27:18Z

src/frontend/planner_test/tests/testdata/insert.yaml

    insert into t values (timestamp '2020-01-01 01:02:03'), (time '03:04:05');
  batch_plan: |
    BatchExchange { order: [], dist: Single }
+<<<<<<< HEAD


Please resolve the conflicts.

Signed-off-by: Eurekaaw <[email protected]>

y-wei requested a review from BugenZhao December 28, 2022 04:26

y-wei changed the title ~~WIP feat: support RETURNING in DML statements~~ feat: WIP support RETURNING in DML statements Dec 28, 2022

BugenZhao reviewed Dec 28, 2022

View reviewed changes

y-wei commented Dec 28, 2022

View reviewed changes

src/utils/pgwire/src/pg_protocol.rs Outdated Show resolved Hide resolved

BugenZhao reviewed Dec 29, 2022

View reviewed changes

lmatz added the user-facing-changes Contains changes that are visible to users label Dec 29, 2022

BugenZhao reviewed Dec 30, 2022

View reviewed changes

BugenZhao marked this pull request as ready for review December 30, 2022 11:30

BugenZhao changed the title ~~feat: WIP support RETURNING in DML statements~~ feat: support RETURNING in DML statements Dec 30, 2022

BugenZhao changed the title ~~feat: support RETURNING in DML statements~~ feat: support RETURNING in DML statements Dec 30, 2022

BugenZhao requested review from BowenXiao1999, ZENOTME and chenzl25 December 30, 2022 11:30

y-wei force-pushed the main branch from d50edd9 to facaa2e Compare December 30, 2022 20:19

y-wei force-pushed the main branch from 9fea7a9 to facaa2e Compare January 3, 2023 01:04

BowenXiao1999 reviewed Jan 3, 2023

View reviewed changes

BowenXiao1999 approved these changes Jan 3, 2023

View reviewed changes

chenzl25 reviewed Jan 3, 2023

View reviewed changes

ZENOTME reviewed Jan 3, 2023

View reviewed changes

BugenZhao added the type/feature Type: New feature. label Jan 3, 2023

BugenZhao reviewed Jan 3, 2023

View reviewed changes

y-wei added 7 commits January 3, 2023 18:07

define parse_returning func

67a7c57

use bind_select_items to bind returning items, refuse agg/window

9098741

implement returning in insert

9a8f735

resolve conflict in executor/insert.rs

d1edd0c

remove redundant code

0c758bd

rollback run_statement's return and corresponding changes

9faabb9

async yield small chunks in executor

75ab3b6

y-wei added 2 commits January 4, 2023 10:36

returning statement will have same output as psql, e.g., INSERT 0 6 i…

01f51bc

…nstead of previous INSERT_RETURNING

add e2e test and regard returning as query for pg_extended

7968138

BugenZhao reviewed Jan 5, 2023

View reviewed changes

y-wei added 3 commits January 5, 2023 22:00

revert changes to is_query_sql, leave a todo comment

eb5b922

update debug fmt

249cdf3

comment out returning e2e tests

e549eb3

y-wei and others added 4 commits January 11, 2023 19:06

format and update planner test

bea5fe8

Signed-off-by: Eurekaaw <[email protected]>

Merge branch 'main' into main

2ac43d2

fmt

00c2c62

Signed-off-by: Eurekaaw <[email protected]>

bump sqllogictest and add tests

8fb7df8

Signed-off-by: Bugen Zhao <[email protected]>

BugenZhao approved these changes Jan 16, 2023

View reviewed changes

y-wei added 2 commits January 16, 2023 11:30

update slt test

f16921e

Signed-off-by: Eurekaaw <[email protected]>

Merge branch 'main' of github.com:singularity-data/risingwave

ab282ce

BugenZhao added the mergify/can-merge label Jan 17, 2023

BugenZhao requested changes Jan 17, 2023

View reviewed changes

BugenZhao removed the mergify/can-merge label Jan 17, 2023

y-wei and others added 4 commits January 17, 2023 10:08

update sqllogictest version

63649b2

Signed-off-by: Eurekaaw <[email protected]>

update simulation client following sqllogictest changes

c970c1a

Signed-off-by: Eurekaaw <[email protected]>

Merge branch 'main' into main

b35bbfa

format planner test

fde984e

Signed-off-by: Eurekaaw <[email protected]>

BugenZhao approved these changes Jan 17, 2023

View reviewed changes

BugenZhao added the mergify/can-merge label Jan 17, 2023

mergify bot merged commit cc8ad46 into risingwavelabs:main Jan 17, 2023

BugenZhao mentioned this pull request Jan 18, 2023

fix(test): temporarily disable RETURNING e2e test #7461

Merged

3 tasks

hengm3467 mentioned this pull request Jan 29, 2023

Document the RETURNING clause in DML statements risingwavelabs/risingwave-docs-legacy#516

Closed

bernscode mentioned this pull request Feb 3, 2023

Add RETURNING to INSERT and DELETE risingwavelabs/risingwave-docs-legacy#540

Merged

3 tasks

xiangjinwu mentioned this pull request Mar 11, 2024

fix(binder): insert binding #15597

Merged

9 tasks

		"{} {{ table: {} }}, {{ returning: {} }}",
		name, self.table_name, self.returning

feat: support RETURNING in DML statements #7094

feat: support RETURNING in DML statements #7094

Uh oh!

Conversation

y-wei commented Dec 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What's changed and what's your intention?

Checklist

Types of user-facing changes

Refer to a related PR or issue link (optional)

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

BugenZhao left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Dec 30, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

y-wei commented Dec 30, 2022

Uh oh!

BowenXiao1999 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

y-wei commented Jan 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

BugenZhao left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

BugenZhao Jan 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

feat: support `RETURNING` in DML statements #7094

feat: support `RETURNING` in DML statements #7094

y-wei commented Dec 28, 2022 •

edited

Loading

codecov bot commented Dec 30, 2022 •

edited

Loading

y-wei commented Jan 5, 2023 •

edited

Loading

BugenZhao Jan 5, 2023 •

edited

Loading

BowenXiao1999 Jan 5, 2023 •

edited

Loading

y-wei commented Jan 9, 2023 •

edited

Loading