ISSUE-2456: Support insert select query #2765

ygf11 · 2021-11-13T09:09:49Z

I hereby agree to the terms of the CLA available at: https://databend.rs/policies/cla/

Summary

support insert select query.

Changelog

New Feature

Related Issues

Fixes #2456

Test Plan

Unit Tests

Stateless Tests

databend-bot · 2021-11-13T09:09:52Z

Thanks for the contribution!
I have applied any labels matching special text in your PR Changelog.

Please review the labels and make any necessary changes.

codecov-commenter · 2021-11-13T09:29:10Z

Codecov Report

Merging #2765 (d459570) into main (387915a) will increase coverage by 0%.
The diff coverage is 57%.

@@          Coverage Diff           @@
##            main   #2765    +/-   ##
======================================
  Coverage     67%     67%            
======================================
  Files        574     585    +11     
  Lines      31149   31342   +193     
======================================
+ Hits       21041   21232   +191     
- Misses     10108   10110     +2

Impacted Files	Coverage Δ
...y/src/datasources/table/fuse/table_test_fixture.rs	`93% <ø> (ø)`
query/src/interpreters/interpreter_insert_into.rs	`59% <11%> (-29%)`	⬇️
query/src/interpreters/interpreter_factory.rs	`32% <37%> (-1%)`	⬇️
query/src/sql/plan_parser.rs	`74% <50%> (-1%)`	⬇️
common/planners/src/plan_insert_into.rs	`62% <100%> (+2%)`	⬆️
common/streams/src/stream_cast.rs	`100% <100%> (ø)`
...y/src/datasources/table/fuse/index/min_max_test.rs	`91% <100%> (+<1%)`	⬆️
.../src/datasources/table/memory/memory_table_test.rs	`92% <100%> (+<1%)`	⬆️
...uery/src/datasources/table/null/null_table_test.rs	`90% <100%> (+<1%)`	⬆️
...rc/interpreters/interpreter_truncate_table_test.rs	`87% <100%> (ø)`
... and 31 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 387915a...d459570. Read the comment docs.

sundy-li · 2021-11-15T05:59:20Z

I think we should avoid having stream inside the plan, after #2786 .

ygf11 · 2021-11-15T06:05:16Z

I think we should avoid having stream inside the plan, after #2786 .

Yeah, I see the pr #2786. It makes append more reasonably. I will do this after #2786 merged:)

sundy-li · 2021-11-15T15:04:42Z

query/src/interpreters/interpreter_insert_into.rs

+fn cast_function(output_type: &DataType) -> Result<Box<dyn Function>> {
+    let function_factory = FunctionFactory::instance();
+    match output_type {
+        DataType::Null => function_factory.get("toNull"),


Why not use CastFunction::create ?

sundy-li · 2021-11-15T15:08:37Z

query/src/interpreters/interpreter_insert_into.rs

+                    let iter = output_schema.fields().iter().zip(select_schema.fields());
+                    let mut colunm_vec = vec![];
+                    for (i, (output_field, input_field)) in iter.enumerate() {
+                        let func = cast_function(output_field.data_type())?;


we can keep funcs out of the stream iter, because trait Function is Send + Sync.

sundy-li · 2021-11-15T15:10:16Z

query/src/interpreters/interpreter_insert_into.rs

+            }
+
+            let select_input_stream = select_executor.execute(None).await?;
+            let cast_input_stream = select_input_stream.map(move |data_block| match data_block {


Better to have a struct named CastStream in common/streams folder.

sundy-li · 2021-11-17T13:54:01Z

@ygf11 hello, is this pr review for review?

ygf11 · 2021-11-17T14:07:15Z

hello, is this pr review for review?

Wait a moment, I will push it soon!

BohuTANG · 2021-11-17T14:22:34Z

query/src/interpreters/interpreter_factory.rs

@@ -52,7 +52,25 @@ impl InterpreterFactory {
            PlanNode::TruncateTable(v) => TruncateTableInterpreter::try_create(ctx, v),
            PlanNode::UseDatabase(v) => UseDatabaseInterpreter::try_create(ctx, v),
            PlanNode::SetVariable(v) => SettingInterpreter::try_create(ctx, v),
-            PlanNode::InsertInto(v) => InsertIntoInterpreter::try_create(ctx, v),
+            PlanNode::InsertInto(v) => {
+                let select = match v.select_plan.clone().take() {


Great job 👍
It would be nice to check and build select plan in the InsertIntoInterpreter, it looks more cleaner

sundy-li · 2021-11-17T14:28:45Z

common/streams/Cargo.toml

 common-datablocks = {path = "../datablocks"}
 common-datavalues = {path = "../datavalues"}
 common-exception = {path = "../exception"}
+common-functions = {path = "../functions"}


It's Clion's auto-reorder? I use vscode fail to find this format command.

It's in vscode too, the command name is Format Cell:)
I found the order in package section, is always name->version->description..., should I recover this?

No, it's better to have alpha orders.

sundy-li · 2021-11-17T14:32:32Z

query/src/interpreters/interprerer_insert_into_select_test.rs

+use crate::sql::*;
+
+#[tokio::test]
+async fn test_insert_into_select_interpreter() -> Result<()> {


sundy-li · 2021-11-17T14:32:43Z

query/src/interpreters/interprerer_insert_into_select_test.rs

+    }
+
+    Ok(())
+}


need a new line.

sundy-li · 2021-11-17T14:45:35Z

tests/suites/0_stateless/12_0000_insert_into_select.sql

+DROP TABLE t1;
+DROP TABLE t2;
+DROP TABLE t3;
+DROP DATABASE db1;


sundy-li

Almost LGTM.

BTW, if we want parallel insert from the multiple select stream, it's better to add some options to ignore last merge_processor in pipeline.rs. It can be addressed in another pr.

 pub async fn execute(&mut self, parallel_insert:  bool) -> Result<SendableDataBlockStream> {
        if self.last_pipe()?.nums() > 1 && !parallel_insert {
            self.merge_processor()?;
        }
        self.last_pipe()?.first().execute().await
    }

databend-bot · 2021-11-17T14:48:54Z

Wait for another reviewer approval

BohuTANG · 2021-11-17T23:14:02Z

/lgtm

Awesome work, thank you @ygf11

databend-bot · 2021-11-17T23:14:27Z

CI Passed
Reviewers Approved
Let's Merge
Thank you for the PR @ygf11

ygf11 · 2021-11-18T01:28:27Z

BTW, if we want parallel insert from the multiple select stream, it's better to add some options to ignore last merge_processor in pipeline.rs. It can be addressed in another pr.

I will do this in another pr.

databend-bot added pr-feature this PR introduces a new feature to the codebase need-review labels Nov 13, 2021

ygf11 force-pushed the feat_insert_select branch from 159df1f to ade6f26 Compare November 15, 2021 14:50

sundy-li reviewed Nov 15, 2021

View reviewed changes

ygf11 added 6 commits November 17, 2021 21:02

Support insert into select statment

ab6b4fd

Add stateless test

96708b9

Improve error message

0fa3509

Add CastStream

0e3089a

Add CastStream test

dd0766b

Fix stream_cast_test

d459570

ygf11 force-pushed the feat_insert_select branch from 730bffb to d459570 Compare November 17, 2021 14:10

ygf11 marked this pull request as ready for review November 17, 2021 14:10

ygf11 requested a review from BohuTANG as a code owner November 17, 2021 14:10

BohuTANG reviewed Nov 17, 2021

View reviewed changes

sundy-li reviewed Nov 17, 2021

View reviewed changes

query/src/interpreters/interprerer_insert_into_select_test.rs

}

Ok(())

}

Copy link

Member

sundy-li Nov 17, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

need a new line.

sundy-li reviewed Nov 17, 2021

View reviewed changes

tests/suites/0_stateless/12_0000_insert_into_select.sql

DROP TABLE t1;

DROP TABLE t2;

DROP TABLE t3;

DROP DATABASE db1;

Copy link

Member

sundy-li Nov 17, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

new line.

sundy-li approved these changes Nov 17, 2021

View reviewed changes

databend-bot approved these changes Nov 17, 2021

View reviewed changes

databend-bot removed the need-review label Nov 17, 2021

databend-bot merged commit ab8d2da into databendlabs:main Nov 17, 2021

BohuTANG mentioned this pull request Nov 18, 2021

Document insert select query #2863

Closed

BohuTANG mentioned this pull request Nov 18, 2021

Improve insert into select #2865

Merged

dantengsky mentioned this pull request Nov 20, 2021

Proposal: supports parallel insertion #2914

Closed

ygf11 mentioned this pull request Nov 20, 2021

Support stream level parallel insert #2921

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ISSUE-2456: Support insert select query #2765

ISSUE-2456: Support insert select query #2765

ygf11 commented Nov 13, 2021

databend-bot commented Nov 13, 2021

codecov-commenter commented Nov 13, 2021 •

edited

Loading

sundy-li commented Nov 15, 2021

ygf11 commented Nov 15, 2021

sundy-li Nov 15, 2021

sundy-li Nov 15, 2021

sundy-li Nov 15, 2021 •

edited

Loading

sundy-li commented Nov 17, 2021

ygf11 commented Nov 17, 2021 •

edited

Loading

BohuTANG Nov 17, 2021

sundy-li Nov 17, 2021 •

edited

Loading

ygf11 Nov 17, 2021 •

edited

Loading

sundy-li Nov 17, 2021

sundy-li Nov 17, 2021

sundy-li Nov 17, 2021

sundy-li Nov 17, 2021

sundy-li left a comment •

edited

Loading

databend-bot commented Nov 17, 2021

BohuTANG commented Nov 17, 2021

databend-bot commented Nov 17, 2021

ygf11 commented Nov 18, 2021

+                  }
+                  Ok(())
+              }

ISSUE-2456: Support insert select query #2765

ISSUE-2456: Support insert select query #2765

Conversation

ygf11 commented Nov 13, 2021

Summary

Changelog

Related Issues

Test Plan

databend-bot commented Nov 13, 2021

codecov-commenter commented Nov 13, 2021 • edited Loading

Codecov Report

sundy-li commented Nov 15, 2021

ygf11 commented Nov 15, 2021

sundy-li Nov 15, 2021

Choose a reason for hiding this comment

sundy-li Nov 15, 2021

Choose a reason for hiding this comment

sundy-li Nov 15, 2021 • edited Loading

Choose a reason for hiding this comment

sundy-li commented Nov 17, 2021

ygf11 commented Nov 17, 2021 • edited Loading

BohuTANG Nov 17, 2021

Choose a reason for hiding this comment

sundy-li Nov 17, 2021 • edited Loading

Choose a reason for hiding this comment

ygf11 Nov 17, 2021 • edited Loading

Choose a reason for hiding this comment

sundy-li Nov 17, 2021

Choose a reason for hiding this comment

sundy-li Nov 17, 2021

Choose a reason for hiding this comment

sundy-li Nov 17, 2021

Choose a reason for hiding this comment

sundy-li Nov 17, 2021

Choose a reason for hiding this comment

sundy-li left a comment • edited Loading

Choose a reason for hiding this comment

databend-bot commented Nov 17, 2021

BohuTANG commented Nov 17, 2021

databend-bot commented Nov 17, 2021

ygf11 commented Nov 18, 2021

codecov-commenter commented Nov 13, 2021 •

edited

Loading

sundy-li Nov 15, 2021 •

edited

Loading

ygf11 commented Nov 17, 2021 •

edited

Loading

sundy-li Nov 17, 2021 •

edited

Loading

ygf11 Nov 17, 2021 •

edited

Loading

sundy-li left a comment •

edited

Loading