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

feat(planner): select without from #5256

Merged
merged 9 commits into from
May 9, 2022

Conversation

Veeupup
Copy link
Contributor

@Veeupup Veeupup commented May 9, 2022

Signed-off-by: Veeupup code@tanweime.com

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

Summary

implement select with from in new planner

Changelog

  • New Feature

Related Issues

Fixes #5163

Signed-off-by: Veeupup <code@tanweime.com>
@vercel
Copy link

vercel bot commented May 9, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Ignored Deployment
Name Status Preview Updated
databend ⬜️ Ignored (Inspect) May 9, 2022 at 0:45AM (UTC)

@mergify
Copy link
Contributor

mergify bot commented May 9, 2022

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.

@mergify mergify bot added the pr-feature this PR introduces a new feature to the codebase label May 9, 2022
@leiysky leiysky changed the title feat(planner): select with from feat(planner): select without from May 9, 2022
Copy link
Contributor

@leiysky leiysky left a comment

Choose a reason for hiding this comment

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

LGTM

@Veeupup Veeupup requested review from sundy-li and xudong963 May 9, 2022 07:31
@@ -65,4 +65,10 @@ drop table t;
drop table t1;
drop table t2;

-- Select without from
Copy link
Contributor

Choose a reason for hiding this comment

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

Would you like to add select 'test case kind'; for the test cases above? So the test result can be seperated.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, will be done now

Copy link
Member

Choose a reason for hiding this comment

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

It seems we should split some small tests for v2 in some conflicts PR, name them like 20_0001_planner_v2_select_without_from.sql? cc @leiysky @xudong963

Copy link
Member

Choose a reason for hiding this comment

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

When planner v2 is ready, we can backport and merge them to our other stateless tests.

Copy link
Contributor

Choose a reason for hiding this comment

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

It seems we should split some small tests for v2 in some conflicts PR, name them like 20_0001_planner_v2_select_without_from.sql? cc @leiysky @xudong963

@BohuTANG This test suite won't exist for too long, we just exploit it to make sure there is no regression of our recent work.

We are running stateless tests with new planner enabled to find out the features we haven't implemented yet. As soon as the new planner can pass all the test suites, this file will be deprecated.

And after migrating to new planner, we are going to migrate the test suites to sqllogic test framework. Which means we won't put more effort on merging this with other tests.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, I think the migration will be completed soon 🐛

Signed-off-by: Veeupup <code@tanweime.com>
@BohuTANG
Copy link
Member

BohuTANG commented May 9, 2022

CI failed:

test parser::test_statements_in_legacy_suites ... FAILED

failures:

---- parser::test_statements_in_legacy_suites stdout ----
thread 'parser::test_statements_in_legacy_suites' panicked at 'called `Result::unwrap()` on an `Err` value: Code: 1005, displayText = error: 
  --> SQL:4:1
  |
1 | set enable_planner_v2 = 1;
2 | 
3 | select '====SELECT_FROM_NUMBERS===='
4 | select * from numbers(10);
  | ^^^^^^ expected `.`, `(`, `IS`, `NOT`, `IN`, `BETWEEN`, or 54 more ...

https://github.com/datafuselabs/databend/runs/6349095598?check_suite_focus=true#step:3:328

How to find CI error, search failed:
image

@@ -1,23 +1,30 @@
set enable_planner_v2 = 1;

select '====SELECT_FROM_NUMBERS===='
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
select '====SELECT_FROM_NUMBERS===='
select '====SELECT_FROM_NUMBERS====';

select * from numbers(10);

-- Comparison expressions
select '====COMPARSION===='
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
select '====COMPARSION===='
select '====COMPARSION====';

select * from numbers(10) where number between 1 and 9 and number > 2 and number < 8 and number is not null and number = 5 and number >= 5 and number <= 5;

-- Cast expression
select '====CAST===='
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
select '====CAST===='
select '====CAST====';

select * from numbers(10) where cast(number as string) = '5';

-- Binary operator
select '====BINARY_OPERATOR===='
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
select '====BINARY_OPERATOR===='
select '====BINARY_OPERATOR====';

select (number + 1 - 2) * 3 / 4 from numbers(1);

-- Functions
select '====FUNCTIONS===='
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
select '====FUNCTIONS===='
select '====FUNCTIONS====';

select sin(cos(number)) from numbers(1);

-- In list
select '====IN_LIST===='
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
select '====IN_LIST===='
select '====IN_LIST====';

select * from numbers(5) where number in (1, 3);

-- Aggregator operator
select '====AGGREGATER===='
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
select '====AGGREGATER===='
select '====AGGREGATER====';

@@ -45,6 +52,7 @@ SELECT max(number) FROM numbers_mt (10) where number > 99999999998;
SELECT max(number) FROM numbers_mt (10) where number > 2;

-- Inner join
select '====INNER_JOIN===='
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
select '====INNER_JOIN===='
select '====INNER_JOIN====';

@@ -67,4 +75,11 @@ drop table t2;

select count(*) from numbers(1000) as t inner join numbers(1000) as t1 on t.number = t1.number;

-- Select without from
select '====SELECT_WITHOUT_FROM===='
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
select '====SELECT_WITHOUT_FROM===='
select '====SELECT_WITHOUT_FROM====';

Signed-off-by: Xuanwo <github@xuanwo.io>
@BohuTANG BohuTANG removed their request for review May 9, 2022 12:45
@mergify mergify bot merged commit 19a3f7c into databendlabs:main May 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
need-review pr-feature this PR introduces a new feature to the codebase
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement SELECT without FROM clause in new planner
7 participants