From 4ec26aa22ce102f5a98ca9a5406e2c0b67d4aa8b Mon Sep 17 00:00:00 2001 From: leiysky Date: Thu, 28 Jul 2022 20:17:07 +0800 Subject: [PATCH 1/7] fix order by column with invalid table name --- query/src/sql/planner/binder/sort.rs | 5 ++++- tests/logictest/suites/query/order.test | 9 +++++++++ 2 files changed, 13 insertions(+), 1 deletion(-) create mode 100644 tests/logictest/suites/query/order.test diff --git a/query/src/sql/planner/binder/sort.rs b/query/src/sql/planner/binder/sort.rs index b0d03d48272a..7130cfaeec6f 100644 --- a/query/src/sql/planner/binder/sort.rs +++ b/query/src/sql/planner/binder/sort.rs @@ -78,7 +78,10 @@ impl<'a> Binder { // We first search the identifier in select list let mut found = false; for item in projections.iter() { - if item.column_name == ident.name { + if item.column_name == ident.name + && table_name.is_none() + && database_name.is_none() + { order_items.push(OrderItem { expr: order.clone(), index: item.index, diff --git a/tests/logictest/suites/query/order.test b/tests/logictest/suites/query/order.test new file mode 100644 index 000000000000..fe1cafcb2960 --- /dev/null +++ b/tests/logictest/suites/query/order.test @@ -0,0 +1,9 @@ +statement ok +set enable_planner_v2 = 1; + +statement error +select number from numbers(10) as a order by b.number; + +statement error +select number from (select * from numbers(10) as b) as a order by b.number; + From 19fb51ae2e277e0b0645b4f207e307fee90f169e Mon Sep 17 00:00:00 2001 From: leiysky Date: Thu, 28 Jul 2022 20:42:19 +0800 Subject: [PATCH 2/7] refine order by --- .../src/sql/executor/physical_plan_builder.rs | 4 +-- query/src/sql/planner/binder/sort.rs | 8 ++--- .../planner/format/display_rel_operator.rs | 6 +--- query/src/sql/planner/plans/sort.rs | 4 +-- tests/logictest/suites/query/order.test | 32 +++++++++++++++++++ 5 files changed, 41 insertions(+), 13 deletions(-) diff --git a/query/src/sql/executor/physical_plan_builder.rs b/query/src/sql/executor/physical_plan_builder.rs index c93deea1bea4..a9c295554499 100644 --- a/query/src/sql/executor/physical_plan_builder.rs +++ b/query/src/sql/executor/physical_plan_builder.rs @@ -239,8 +239,8 @@ impl PhysicalPlanBuilder { .items .iter() .map(|v| SortDesc { - asc: v.asc.unwrap_or(true), - nulls_first: v.nulls_first.unwrap_or_default(), + asc: v.asc, + nulls_first: v.nulls_first, order_by: v.index.to_string(), }) .collect(), diff --git a/query/src/sql/planner/binder/sort.rs b/query/src/sql/planner/binder/sort.rs index 7130cfaeec6f..28b0e5741499 100644 --- a/query/src/sql/planner/binder/sort.rs +++ b/query/src/sql/planner/binder/sort.rs @@ -232,8 +232,8 @@ impl<'a> Binder { } let order_by_item = SortItem { index: order.index, - asc: order.expr.asc, - nulls_first: order.expr.nulls_first, + asc: order.expr.asc.unwrap_or(true), + nulls_first: order.expr.nulls_first.unwrap_or(false), }; order_by_items.push(order_by_item); } @@ -269,8 +269,8 @@ impl<'a> Binder { Scalar::BoundColumnRef(BoundColumnRef { column }) => { let order_by_item = SortItem { index: column.index, - asc: order.asc, - nulls_first: order.nulls_first, + asc: order.asc.unwrap_or(true), + nulls_first: order.nulls_first.unwrap_or(false), }; order_by_items.push(order_by_item); } diff --git a/query/src/sql/planner/format/display_rel_operator.rs b/query/src/sql/planner/format/display_rel_operator.rs index 2c28d2a95195..dd6cc3f2fd4d 100644 --- a/query/src/sql/planner/format/display_rel_operator.rs +++ b/query/src/sql/planner/format/display_rel_operator.rs @@ -330,11 +330,7 @@ pub fn format_sort( "{} (#{}) {}", name, item.index, - if item.asc.unwrap_or(false) { - "ASC" - } else { - "DESC" - } + if item.asc { "ASC" } else { "DESC" } ) }) .collect::>() diff --git a/query/src/sql/planner/plans/sort.rs b/query/src/sql/planner/plans/sort.rs index 478e8a574157..6269af3b3db8 100644 --- a/query/src/sql/planner/plans/sort.rs +++ b/query/src/sql/planner/plans/sort.rs @@ -33,8 +33,8 @@ pub struct Sort { #[derive(Clone, Debug)] pub struct SortItem { pub index: IndexType, - pub asc: Option, - pub nulls_first: Option, + pub asc: bool, + pub nulls_first: bool, } impl Operator for Sort { diff --git a/tests/logictest/suites/query/order.test b/tests/logictest/suites/query/order.test index fe1cafcb2960..e7f6a71c450b 100644 --- a/tests/logictest/suites/query/order.test +++ b/tests/logictest/suites/query/order.test @@ -1,9 +1,41 @@ statement ok set enable_planner_v2 = 1; +statement ok +create table order_test(a int null); + +statement ok +insert into order_test values(1),(2),(null); + +statement query I +select * from order_test order by a; + +---- +1 +2 +NULL + +statement query I +select * from order_test order by a desc; + +---- +2 +1 +NULL + +statement query I +select * from order_test order by a nulls first; + +---- +NULL +1 +2 + statement error select number from numbers(10) as a order by b.number; statement error select number from (select * from numbers(10) as b) as a order by b.number; +statement ok +drop table order_test; From 3d1a1db12b36ee9521bcd2640892eb47f9a97e66 Mon Sep 17 00:00:00 2001 From: leiysky Date: Fri, 29 Jul 2022 11:25:36 +0800 Subject: [PATCH 3/7] fix --- query/src/sql/planner/binder/sort.rs | 27 +++++++++++++++++++++++---- 1 file changed, 23 insertions(+), 4 deletions(-) diff --git a/query/src/sql/planner/binder/sort.rs b/query/src/sql/planner/binder/sort.rs index 28b0e5741499..891c8e74a918 100644 --- a/query/src/sql/planner/binder/sort.rs +++ b/query/src/sql/planner/binder/sort.rs @@ -78,10 +78,29 @@ impl<'a> Binder { // We first search the identifier in select list let mut found = false; for item in projections.iter() { - if item.column_name == ident.name - && table_name.is_none() - && database_name.is_none() - { + let matched = match ( + (&item.database_name, &item.table_name), + (&database_name, &table_name), + ) { + ( + (Some(target_database), Some(target_table)), + (Some(source_database), Some(source_table)), + ) if target_database == &source_database.name + && target_table == &source_table.name + && &ident.name == &item.column_name => + { + true + } + ((None, Some(target_table)), (_, Some(source_table))) + if target_table == &source_table.name + && &ident.name == &item.column_name => + { + true + } + ((None, None), (_, _)) if ident.name == item.column_name => true, + _ => false, + }; + if matched { order_items.push(OrderItem { expr: order.clone(), index: item.index, From d680363471d8e64957792752cbe37674be08b399 Mon Sep 17 00:00:00 2001 From: leiysky Date: Fri, 29 Jul 2022 11:36:44 +0800 Subject: [PATCH 4/7] fix --- .../suites/gen/03_dml/03_0004_select_order_by_db_table_col_v2 | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/logictest/suites/gen/03_dml/03_0004_select_order_by_db_table_col_v2 b/tests/logictest/suites/gen/03_dml/03_0004_select_order_by_db_table_col_v2 index 692b49a29ba4..940258b1872f 100644 --- a/tests/logictest/suites/gen/03_dml/03_0004_select_order_by_db_table_col_v2 +++ b/tests/logictest/suites/gen/03_dml/03_0004_select_order_by_db_table_col_v2 @@ -67,7 +67,7 @@ SELECT * FROM a.t ORDER BY a.t.id ASC; 1 1 2 2 -statement error 1003 +statement error 1065 SELECT * FROM a.t ORDER BY B.T.id ASC; statement query II From 692206bce7a6cf844533aa6cc181341416a38d83 Mon Sep 17 00:00:00 2001 From: leiysky Date: Fri, 29 Jul 2022 12:35:54 +0800 Subject: [PATCH 5/7] fix --- query/src/sql/planner/binder/sort.rs | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-) diff --git a/query/src/sql/planner/binder/sort.rs b/query/src/sql/planner/binder/sort.rs index 891c8e74a918..1a890d87a1d3 100644 --- a/query/src/sql/planner/binder/sort.rs +++ b/query/src/sql/planner/binder/sort.rs @@ -79,21 +79,20 @@ impl<'a> Binder { let mut found = false; for item in projections.iter() { let matched = match ( - (&item.database_name, &item.table_name), (&database_name, &table_name), + (&item.database_name, &item.table_name), ) { ( - (Some(target_database), Some(target_table)), - (Some(source_database), Some(source_table)), - ) if target_database == &source_database.name - && target_table == &source_table.name - && &ident.name == &item.column_name => + (Some(ident_database), Some(ident_table)), + (Some(database), Some(table)), + ) if &ident_database.name == database + && &ident_table.name == table + && ident.name == item.column_name => { true } - ((None, Some(target_table)), (_, Some(source_table))) - if target_table == &source_table.name - && &ident.name == &item.column_name => + ((None, Some(ident_table)), (_, Some(table))) + if &ident_table.name == table && ident.name == item.column_name => { true } From 884d42973ffc81d6b37d7e330123f6edefc6386a Mon Sep 17 00:00:00 2001 From: leiysky Date: Fri, 29 Jul 2022 13:34:54 +0800 Subject: [PATCH 6/7] fix --- .../gen/03_dml/03_0004_select_order_by_db_table_col_v2 | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/tests/logictest/suites/gen/03_dml/03_0004_select_order_by_db_table_col_v2 b/tests/logictest/suites/gen/03_dml/03_0004_select_order_by_db_table_col_v2 index 940258b1872f..c0b66fe1c7a6 100644 --- a/tests/logictest/suites/gen/03_dml/03_0004_select_order_by_db_table_col_v2 +++ b/tests/logictest/suites/gen/03_dml/03_0004_select_order_by_db_table_col_v2 @@ -47,12 +47,9 @@ SELECT DISTINCT(id) FROM a.t ORDER BY a.t.id; statement error 1065 SELECT DISTINCT(id) FROM a.t ORDER BY a.t.id2; -statement query I +statement error SELECT SUM(id) as id2 FROM a.t ORDER BY a.t.id2; ----- -3 - statement query I SELECT DISTINCT(id) as id2 FROM a.t ORDER BY a.t.id2; From 2c707636017aa4e77010f62a05d27373a11e361d Mon Sep 17 00:00:00 2001 From: leiysky Date: Fri, 29 Jul 2022 18:05:30 +0800 Subject: [PATCH 7/7] fix --- .../suites/gen/03_dml/03_0004_select_order_by_db_table_col_v2 | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/logictest/suites/gen/03_dml/03_0004_select_order_by_db_table_col_v2 b/tests/logictest/suites/gen/03_dml/03_0004_select_order_by_db_table_col_v2 index c0b66fe1c7a6..7416a95805da 100644 --- a/tests/logictest/suites/gen/03_dml/03_0004_select_order_by_db_table_col_v2 +++ b/tests/logictest/suites/gen/03_dml/03_0004_select_order_by_db_table_col_v2 @@ -74,7 +74,7 @@ SELECT * FROM a.t ORDER BY a.t.id DESC; 2 2 1 1 -statement error 1003 +statement error 1065 SELECT * FROM a.t ORDER BY B.T.id DESC; statement ok