Skip to content

Comments

Port test in subqueries.rs from rust to sqllogictest #6675

Merged
alamb merged 40 commits intoapache:mainfrom
jiangzhx:issues/6668
Jun 19, 2023
Merged

Port test in subqueries.rs from rust to sqllogictest #6675
alamb merged 40 commits intoapache:mainfrom
jiangzhx:issues/6668

Conversation

@jiangzhx
Copy link
Contributor

@jiangzhx jiangzhx commented Jun 15, 2023

Which issue does this PR close?

Closes #6668.

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

@github-actions github-actions bot added core Core DataFusion crate sqllogictest SQL Logic Tests (.slt) labels Jun 15, 2023
@jackwener
Copy link
Member

jackwener commented Jun 15, 2023

CI failed relate with #6676

jiangzhx
jiangzhx commented Jun 15, 2023
View reviewed changes
datafusion/core/tests/sql/subqueries.rs
// "| 44 | |",
// "+-------+--------+",
// ];
// assert_batches_eq!(expected, &results);
Copy link
Contributor Author

@jiangzhx jiangzhx Jun 15, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Why was this test case commented out even though it's working correctly?
@mingmwang, if you have time, could you please help me understand,i'm working on move rs to sqllogicatest

Copy link
Contributor

@mingmwang mingmwang Jun 16, 2023

Choose a reason for hiding this comment

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

I remember this was because the infer nullablity has bug causing the physical plan schema check failed.
I will take a closer look tomorrow, maybe it was already fixed recently.

Copy link
Contributor Author

@jiangzhx jiangzhx Jun 16, 2023

Choose a reason for hiding this comment

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

Got it, it should have been fixed. I uncommented the code and everything is working fine.

@jiangzhx jiangzhx marked this pull request as ready for review June 15, 2023 15:13
@jackwener jackwener requested review from alamb and jackwener June 16, 2023 12:21
@mingmwang
Copy link
Contributor

mingmwang commented Jun 16, 2023

@jiangzhx
In the original UTs, there are some comments to explain why the plan is like this,
For example:

exists_subquery_with_limit, in_correlated_subquery_with_limit

In some cases the limit is removed and the subquery is de-correlated, in some cases it is not.

Could you please add the comments back to the slt file ?

@jiangzhx
Copy link
Contributor Author

jiangzhx commented Jun 16, 2023

@jiangzhx In the original UTs, there are some comments to explain why the plan is like this, For example:

exists_subquery_with_limit, in_correlated_subquery_with_limit

In some cases the limit is removed and the subquery is de-correlated, in some cases it is not.

Could you please add the comments back to the slt file ?

No problem, I will check it tonight and rebase the commit.

jiangzhx added 21 commits June 16, 2023 22:06
@jiangzhx
port correlated_recursive_scalar_subquery to sql
679c10f
@jiangzhx
port correlated_where_in to sql
156b945
@jiangzhx
port exists_subquery_with_same_table to sql
5c24c67
@jiangzhx
port in_subquery_with_same_table to sql
256e112
@jiangzhx
port in_subquery_nested_exist_subquery to sql
0f7d7ec
@jiangzhx
port invalid_scalar_subquery to sql
4fea935
@jiangzhx
port subquery_not_allowed to sql
cfab874
@jiangzhx
port non_aggregated_correlated_scalar_subquery to sql
acf86a8
@jiangzhx
port non_aggregated_correlated_scalar_subquery_with_limit to sql
6f700d8
@jiangzhx
port non_aggregated_correlated_scalar_subquery_with_single_row to sql
ba9f3da
@jiangzhx
port non_equal_correlated_scalar_subquery to sql
27f5103
@jiangzhx
port aggregated_correlated_scalar_subquery_with_extra_group_by_column…
5bd747c 
…s to sql
@jiangzhx
port support_agg_correlated_columns to sql
5917e95
@jiangzhx
port support_agg_correlated_columns2 to sql
ded57e6
@jiangzhx
port support_join_correlated_columns to sql
73ceddd
@jiangzhx
port subquery_contains_join_contains_correlated_columns to sql
b9fc2d1
@jiangzhx
port subquery_contains_join_contains_sub_query_alias_correlated_colum…
acf8b69 
…ns to sql
@jiangzhx
port support_order_by_correlated_columns to sql
6e61c90
@jiangzhx
port exists_subquery_with_select_null to sql
aee3064
@jiangzhx
port exists_subquery_with_limit to sql
01091b0
@jiangzhx
port exists_subquery_with_limit0 to sql
14b4708
jiangzhx added 19 commits June 16, 2023 22:28
@jiangzhx
port not_exists_subquery_with_limit0 to sql
2f9a1ea
@jiangzhx
port in_correlated_subquery_with_limit to sql
6c90d05
@jiangzhx
port in_non_correlated_subquery_with_limit to sql
6a9555b
@jiangzhx
port uncorrelated_scalar_subquery_with_limit0 to sql
7173bd1
@jiangzhx
port support_union_subquery to sql
85f5fc0
@jiangzhx
port simple_uncorrelated_scalar_subquery to sql
48b6401
@jiangzhx
port simple_uncorrelated_scalar_subquery2 to sql
5124ccd
@jiangzhx
port correlated_scalar_subquery_count_agg to sql
11b6755
@jiangzhx
port correlated_scalar_subquery_count_agg2 to sql
f10a211
@jiangzhx
port correlated_scalar_subquery_count_agg_with_alias to sql
3e35640
@jiangzhx
port correlated_scalar_subquery_count_agg_complex_expr to sql
1fbe9b2
@jiangzhx
port correlated_scalar_subquery_count_agg_where_clause to sql
49d96a2
@jiangzhx
port correlated_scalar_subquery_count_agg_with_having to sql
e449ba7
@jiangzhx
port correlated_scalar_subquery_count_agg_with_pull_up_having to sql
9c49357
@jiangzhx
port correlated_scalar_subquery_count_agg_in_having to sql
4cd1ffb
@jiangzhx
port correlated_scalar_subquery_count_agg_in_nested_projection to sql
3d05781
@jiangzhx
port correlated_scalar_subquery_count_agg_in_nested_subquery to sql
10c71f0
@jiangzhx
port correlated_scalar_subquery_count_agg_in_case_when to sql
8274cf7
@jiangzhx
clippy fix
b262b8c
@jiangzhx
Copy link
Contributor Author

jiangzhx commented Jun 16, 2023

@jiangzhx In the original UTs, there are some comments to explain why the plan is like this, For example:

exists_subquery_with_limit, in_correlated_subquery_with_limit

In some cases the limit is removed and the subquery is de-correlated, in some cases it is not.

Could you please add the comments back to the slt file ?

@mingmwang Following your suggestion, I have already made the changes. I hope you have time to help me review it again. thanks for your help.

mingmwang
mingmwang approved these changes Jun 19, 2023
View reviewed changes
Copy link
Contributor

@mingmwang mingmwang left a comment

Choose a reason for hiding this comment

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

LGTM

@alamb alamb merged commit 873d417 into apache:main Jun 19, 2023
@alamb
Copy link
Contributor

alamb commented Jun 19, 2023

Thank you @jiangzhx and @mingmwang

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mingmwang mingmwang mingmwang approved these changes

@alamb alamb Awaiting requested review from alamb

@jackwener jackwener Awaiting requested review from jackwener

Assignees

No one assigned

Labels

core Core DataFusion crate sqllogictest SQL Logic Tests (.slt)

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Port test in subqueries.rs from rust to sqllogictest

4 participants

@jiangzhx @jackwener @mingmwang @alamb