Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[fuse table] insert overwrite stmt implementation #3150

Merged
merged 4 commits into from
Nov 30, 2021

Conversation

Veeupup
Copy link
Contributor

@Veeupup Veeupup commented Nov 29, 2021

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

Summary

add insert overwrite statement

Changelog

  • New Feature

Related Issues

Fixes #3144

Test Plan

Unit Tests

Stateless Tests

@databend-bot
Copy link
Member

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.

@databend-bot databend-bot added pr-feature this PR introduces a new feature to the codebase need-review labels Nov 29, 2021
@mergify
Copy link
Contributor

mergify bot commented Nov 29, 2021

This pull request has merge conflicts that must be resolved before it can be merged. @Veeupup please rebase it 🙏

@dantengsky
Copy link
Member

@Veeupup Sorry for the misleading issue #3144 that I created, I think that impls insert overwrite instead might be better. let's discuss this tomorrow

@sundy-li
Copy link
Member

We can have both syntaxes. Replace | Insert override .

@Veeupup
Copy link
Contributor Author

Veeupup commented Nov 30, 2021

are they only different in syntaxes? or any other point I did not get?

@Veeupup Veeupup force-pushed the ISSUE-3144-replace-stmt branch from e023c43 to eb25dd9 Compare November 30, 2021 08:34
@Veeupup Veeupup changed the title [fuse table] replace stmt implementation [fuse table] insert overwrite stmt implementation Nov 30, 2021
@Veeupup Veeupup marked this pull request as ready for review November 30, 2021 11:33
@Veeupup
Copy link
Contributor Author

Veeupup commented Nov 30, 2021

/review @dantengsky

@databend-bot
Copy link
Member

Take the reviewer to dantengsky

@Veeupup Veeupup marked this pull request as draft November 30, 2021 11:37
@dantengsky
Copy link
Member

great work! 👍

It would be better if there were:

  • unit test case for Table::commit(..., overwrite=true)
    extending query/src/storages/fuse/table_test.rs, or any other way you'd like
  • a stateless test case
    like tests/suites/0_stateless/09_0004_remote_insert_into_select.sql ( a simpler version will be enough)
    but if the SQL parser layer needs some further tuning, I think we can put this in another PR

@codecov-commenter
Copy link

codecov-commenter commented Nov 30, 2021

Codecov Report

Merging #3150 (5c43f99) into main (23630e6) will decrease coverage by 0%.
The diff coverage is 92%.

Impacted file tree graph

@@          Coverage Diff           @@
##            main   #3150    +/-   ##
======================================
- Coverage     66%     66%    -1%     
======================================
  Files        673     673            
  Lines      34965   35464   +499     
======================================
+ Hits       23385   23699   +314     
- Misses     11580   11765   +185     
Impacted Files Coverage Δ
query/src/interpreters/interpreter_copy.rs 0% <0%> (ø)
query/src/storages/fuse/operations/commit.rs 88% <84%> (-1%) ⬇️
query/src/storages/fuse/table_test.rs 93% <90%> (-2%) ⬇️
common/planners/src/plan_insert_into.rs 83% <100%> (+<1%) ⬆️
query/src/interpreters/interpreter_insert_into.rs 79% <100%> (+<1%) ⬆️
query/src/sql/statements/statement_insert.rs 83% <100%> (+2%) ⬆️
query/src/storages/fuse/index/min_max_test.rs 91% <100%> (-2%) ⬇️
...ry/src/storages/fuse/statistics/statistics_test.rs 97% <100%> (ø)
query/src/storages/fuse/table.rs 88% <100%> (+<1%) ⬆️
query/src/storages/fuse/table_test_fixture.rs 95% <100%> (+<1%) ⬆️
... and 45 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 23630e6...5c43f99. Read the comment docs.

@Veeupup Veeupup marked this pull request as ready for review November 30, 2021 12:58
@Veeupup
Copy link
Contributor Author

Veeupup commented Nov 30, 2021

great work! 👍

It would be better if there were:

  • unit test case for Table::commit(..., overwrite=true)
    extending query/src/storages/fuse/table_test.rs, or any other way you'd like
  • a stateless test case
    like tests/suites/0_stateless/09_0004_remote_insert_into_select.sql ( a simpler version will be enough)
    but if the SQL parser layer needs some further tuning, I think we can put this in another PR

: D
done.

Copy link
Member

@dantengsky dantengsky left a comment

Choose a reason for hiding this comment

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

LGTM

@databend-bot
Copy link
Member

Wait for another reviewer approval

@databend-bot
Copy link
Member

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

@databend-bot databend-bot merged commit d85fa40 into databendlabs:main Nov 30, 2021
@Veeupup Veeupup deleted the ISSUE-3144-replace-stmt branch December 3, 2021 07:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr-feature this PR introduces a new feature to the codebase
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[fuse table] impl insert overwrite stmt
7 participants