From fa0f65cda6abfba1fc94aef905b3216f1528d982 Mon Sep 17 00:00:00 2001 From: lmatz Date: Wed, 18 Dec 2024 00:17:15 +0800 Subject: [PATCH 01/15] feat: support expression in limit clause --- src/frontend/src/binder/query.rs | 10 +++--- src/frontend/src/planner/query.rs | 43 +++++++++++++++++++++++-- src/sqlparser/src/ast/query.rs | 4 +-- src/sqlparser/src/parser.rs | 12 +++---- src/tests/sqlsmith/src/sql_gen/query.rs | 8 +++-- 5 files changed, 58 insertions(+), 19 deletions(-) diff --git a/src/frontend/src/binder/query.rs b/src/frontend/src/binder/query.rs index f227b77c9df90..c70bd3a49c695 100644 --- a/src/frontend/src/binder/query.rs +++ b/src/frontend/src/binder/query.rs @@ -38,7 +38,7 @@ use crate::expr::{CorrelatedId, Depth, ExprImpl, ExprRewriter}; pub struct BoundQuery { pub body: BoundSetExpr, pub order: Vec, - pub limit: Option, + pub limit: Option, pub offset: Option, pub with_ties: bool, pub extra_order_exprs: Vec, @@ -174,13 +174,15 @@ impl Binder { ) => { with_ties = fetch_with_ties; match quantity { - Some(v) => Some(parse_non_negative_i64("LIMIT", &v)? as u64), - None => Some(1), + Some(v) => Some(v), + None => Some(Expr::Value(Value::Number("1".to_string()))), } } - (Some(limit), None) => Some(parse_non_negative_i64("LIMIT", &limit)? as u64), + (Some(limit), None) => Some(limit), (Some(_), Some(_)) => unreachable!(), // parse error }; + let limit = limit.map(|expr| self.bind_expr(expr)).transpose()?; + let offset = offset .map(|s| parse_non_negative_i64("OFFSET", &s)) .transpose()? diff --git a/src/frontend/src/planner/query.rs b/src/frontend/src/planner/query.rs index bbf152c36fdfa..db116a0b07111 100644 --- a/src/frontend/src/planner/query.rs +++ b/src/frontend/src/planner/query.rs @@ -13,9 +13,11 @@ // limitations under the License. use fixedbitset::FixedBitSet; +use risingwave_common::types::DataType; use crate::binder::BoundQuery; -use crate::error::Result; +use crate::error::{ErrorCode, Result, RwError}; +use crate::expr::ExprImpl; use crate::optimizer::plan_node::{LogicalLimit, LogicalTopN}; use crate::optimizer::property::{Order, RequiredDist}; use crate::optimizer::PlanRoot; @@ -50,7 +52,44 @@ impl Planner { let func_dep = plan.functional_dependency(); order = func_dep.minimize_order_key(order, &[]); - let limit = limit.unwrap_or(LIMIT_ALL_COUNT); + let limit = limit.unwrap_or(ExprImpl::literal_bigint(LIMIT_ALL_COUNT as i64)); + if !limit.is_const() { + return Err(ErrorCode::ExprError( + format!( + "expects an integer or expression after LIMIT, but found:{:?}", + limit + ) + .into(), + ) + .into()); + } + let limit_original = limit.clone(); + let limit_err = ErrorCode::ExprError( + format!( + "expects an integer or expression after LIMIT, but found:{:?}", + limit_original + ) + .into(), + ); + let limit_cast_to_bigint = limit.cast_explicit(DataType::Int64).map_err(|_| { + RwError::from(ErrorCode::ExprError( + format!( + "expects an integer or expression after LIMIT, but found:{:?}", + limit_original + ) + .into(), + )) + })?; + let limit = match limit_cast_to_bigint.fold_const() { + Ok(datum) => match datum { + Some(datum) => datum.as_integral() as u64, + None => { + return Err(limit_err.into()); + } + }, + _ => return Err(limit_err.into()), + }; + let offset = offset.unwrap_or_default(); plan = if order.column_orders.is_empty() { // Should be rejected by parser. diff --git a/src/sqlparser/src/ast/query.rs b/src/sqlparser/src/ast/query.rs index 428fe4e4c5417..310b9ceb882d2 100644 --- a/src/sqlparser/src/ast/query.rs +++ b/src/sqlparser/src/ast/query.rs @@ -30,7 +30,7 @@ pub struct Query { /// ORDER BY pub order_by: Vec, /// `LIMIT { | ALL }` - pub limit: Option, + pub limit: Option, /// `OFFSET [ { ROW | ROWS } ]` /// /// `ROW` and `ROWS` are noise words that don't influence the effect of the clause. @@ -655,7 +655,7 @@ impl fmt::Display for OrderByExpr { #[cfg_attr(feature = "serde", derive(Serialize, Deserialize))] pub struct Fetch { pub with_ties: bool, - pub quantity: Option, + pub quantity: Option, } impl fmt::Display for Fetch { diff --git a/src/sqlparser/src/parser.rs b/src/sqlparser/src/parser.rs index dda74a64b4369..17d7e77911a52 100644 --- a/src/sqlparser/src/parser.rs +++ b/src/sqlparser/src/parser.rs @@ -5357,16 +5357,12 @@ impl Parser<'_> { } /// Parse a LIMIT clause - pub fn parse_limit(&mut self) -> PResult> { + pub fn parse_limit(&mut self) -> PResult> { if self.parse_keyword(Keyword::ALL) { Ok(None) } else { - let number = self.parse_number_value()?; - // TODO(Kexiang): support LIMIT expr - if self.consume_token(&Token::DoubleColon) { - self.expect_keyword(Keyword::BIGINT)? - } - Ok(Some(number)) + let expr = self.parse_expr()?; + Ok(Some(expr)) } } @@ -5390,7 +5386,7 @@ impl Parser<'_> { { None } else { - let quantity = self.parse_number_value()?; + let quantity = self.parse_expr()?; self.expect_one_of_keywords(&[Keyword::ROW, Keyword::ROWS])?; Some(quantity) }; diff --git a/src/tests/sqlsmith/src/sql_gen/query.rs b/src/tests/sqlsmith/src/sql_gen/query.rs index cfbfdb70cbf8e..4e43204348d96 100644 --- a/src/tests/sqlsmith/src/sql_gen/query.rs +++ b/src/tests/sqlsmith/src/sql_gen/query.rs @@ -23,7 +23,7 @@ use rand::prelude::SliceRandom; use rand::Rng; use risingwave_common::types::DataType; use risingwave_sqlparser::ast::{ - Cte, Distinct, Expr, Ident, Query, Select, SelectItem, SetExpr, TableWithJoins, With, + Cte, Distinct, Expr, Ident, Query, Select, SelectItem, SetExpr, TableWithJoins, Value, With, }; use crate::sql_gen::utils::create_table_with_joins_from_table; @@ -160,10 +160,12 @@ impl SqlGenerator<'_, R> { } } - fn gen_limit(&mut self, has_order_by: bool) -> Option { + fn gen_limit(&mut self, has_order_by: bool) -> Option { if (!self.is_mview || has_order_by) && self.flip_coin() { let start = if self.is_mview { 1 } else { 0 }; - Some(self.rng.gen_range(start..=100).to_string()) + Some(Expr::Value(Value::Number( + self.rng.gen_range(start..=100).to_string(), + ))) } else { None } From caab0d8c2b4ce144db6b4d34136da982b7f76a52 Mon Sep 17 00:00:00 2001 From: lmatz Date: Wed, 18 Dec 2024 00:52:29 +0800 Subject: [PATCH 02/15] fix test --- src/sqlparser/tests/sqlparser_common.rs | 23 ++++++++++++++++------- 1 file changed, 16 insertions(+), 7 deletions(-) diff --git a/src/sqlparser/tests/sqlparser_common.rs b/src/sqlparser/tests/sqlparser_common.rs index 496a34b347573..8f0b308e4032d 100644 --- a/src/sqlparser/tests/sqlparser_common.rs +++ b/src/sqlparser/tests/sqlparser_common.rs @@ -206,17 +206,20 @@ fn parse_simple_select() { assert_eq!(select.distinct, Distinct::All); assert_eq!(3, select.projection.len()); let select = verified_query(sql); - assert_eq!(Some("5".to_owned()), select.limit); + assert_eq!( + Some(Expr::Value(Value::Number("5".to_string()))), + select.limit + ); } #[test] fn parse_limit_is_not_an_alias() { // In dialects supporting LIMIT it shouldn't be parsed as a table alias let ast = verified_query("SELECT id FROM customer LIMIT 1"); - assert_eq!(Some("1".to_owned()), ast.limit); + assert_eq!(Some(Expr::Value(Value::Number("1".to_string()))), ast.limit); let ast = verified_query("SELECT 1 LIMIT 5"); - assert_eq!(Some("5".to_owned()), ast.limit); + assert_eq!(Some(Expr::Value(Value::Number("5".to_string()))), ast.limit); } #[test] @@ -1068,7 +1071,10 @@ fn parse_select_order_by_limit() { ], select.order_by ); - assert_eq!(Some("2".to_owned()), select.limit); + assert_eq!( + Some(Expr::Value(Value::Number("2".to_string()))), + select.limit + ); } #[test] @@ -1091,7 +1097,10 @@ fn parse_select_order_by_nulls_order() { ], select.order_by ); - assert_eq!(Some("2".to_owned()), select.limit); + assert_eq!( + Some(Expr::Value(Value::Number("2".to_string()))), + select.limit + ); } #[test] @@ -3542,7 +3551,7 @@ fn parse_fetch() { let fetch_first_two_rows_only = Some(Fetch { with_ties: false, - quantity: Some("2".to_owned()), + quantity: Some(Expr::Value(Value::Number("2".to_string()))), }); let ast = verified_query("SELECT foo FROM bar FETCH FIRST 2 ROWS ONLY"); assert_eq!(ast.fetch, fetch_first_two_rows_only); @@ -3567,7 +3576,7 @@ fn parse_fetch() { ast.fetch, Some(Fetch { with_ties: true, - quantity: Some("2".to_owned()), + quantity: Some(Expr::Value(Value::Number("2".to_string()))), }) ); let ast = verified_query( From 437f510ff6c0d5d7cbd4a34194dd3d4e6d27cfca Mon Sep 17 00:00:00 2001 From: lmatz Date: Wed, 18 Dec 2024 02:21:25 +0800 Subject: [PATCH 03/15] fix style --- src/frontend/src/binder/query.rs | 2 +- src/frontend/src/planner/query.rs | 7 +------ src/sqlparser/tests/sqlparser_common.rs | 14 +++++++------- 3 files changed, 9 insertions(+), 14 deletions(-) diff --git a/src/frontend/src/binder/query.rs b/src/frontend/src/binder/query.rs index c70bd3a49c695..c7a1118662f6c 100644 --- a/src/frontend/src/binder/query.rs +++ b/src/frontend/src/binder/query.rs @@ -175,7 +175,7 @@ impl Binder { with_ties = fetch_with_ties; match quantity { Some(v) => Some(v), - None => Some(Expr::Value(Value::Number("1".to_string()))), + None => Some(Expr::Value(Value::Number("1".to_owned()))), } } (Some(limit), None) => Some(limit), diff --git a/src/frontend/src/planner/query.rs b/src/frontend/src/planner/query.rs index db116a0b07111..fc125574bc689 100644 --- a/src/frontend/src/planner/query.rs +++ b/src/frontend/src/planner/query.rs @@ -81,12 +81,7 @@ impl Planner { )) })?; let limit = match limit_cast_to_bigint.fold_const() { - Ok(datum) => match datum { - Some(datum) => datum.as_integral() as u64, - None => { - return Err(limit_err.into()); - } - }, + Ok(Some(datum)) => datum.as_integral() as u64, _ => return Err(limit_err.into()), }; diff --git a/src/sqlparser/tests/sqlparser_common.rs b/src/sqlparser/tests/sqlparser_common.rs index 8f0b308e4032d..9ce0b620bf963 100644 --- a/src/sqlparser/tests/sqlparser_common.rs +++ b/src/sqlparser/tests/sqlparser_common.rs @@ -207,7 +207,7 @@ fn parse_simple_select() { assert_eq!(3, select.projection.len()); let select = verified_query(sql); assert_eq!( - Some(Expr::Value(Value::Number("5".to_string()))), + Some(Expr::Value(Value::Number("5".to_owned()))), select.limit ); } @@ -216,10 +216,10 @@ fn parse_simple_select() { fn parse_limit_is_not_an_alias() { // In dialects supporting LIMIT it shouldn't be parsed as a table alias let ast = verified_query("SELECT id FROM customer LIMIT 1"); - assert_eq!(Some(Expr::Value(Value::Number("1".to_string()))), ast.limit); + assert_eq!(Some(Expr::Value(Value::Number("1".to_owned()))), ast.limit); let ast = verified_query("SELECT 1 LIMIT 5"); - assert_eq!(Some(Expr::Value(Value::Number("5".to_string()))), ast.limit); + assert_eq!(Some(Expr::Value(Value::Number("5".to_owned()))), ast.limit); } #[test] @@ -1072,7 +1072,7 @@ fn parse_select_order_by_limit() { select.order_by ); assert_eq!( - Some(Expr::Value(Value::Number("2".to_string()))), + Some(Expr::Value(Value::Number("2".to_owned()))), select.limit ); } @@ -1098,7 +1098,7 @@ fn parse_select_order_by_nulls_order() { select.order_by ); assert_eq!( - Some(Expr::Value(Value::Number("2".to_string()))), + Some(Expr::Value(Value::Number("2".to_owned()))), select.limit ); } @@ -3551,7 +3551,7 @@ fn parse_fetch() { let fetch_first_two_rows_only = Some(Fetch { with_ties: false, - quantity: Some(Expr::Value(Value::Number("2".to_string()))), + quantity: Some(Expr::Value(Value::Number("2".to_owned()))), }); let ast = verified_query("SELECT foo FROM bar FETCH FIRST 2 ROWS ONLY"); assert_eq!(ast.fetch, fetch_first_two_rows_only); @@ -3576,7 +3576,7 @@ fn parse_fetch() { ast.fetch, Some(Fetch { with_ties: true, - quantity: Some(Expr::Value(Value::Number("2".to_string()))), + quantity: Some(Expr::Value(Value::Number("2".to_owned()))), }) ); let ast = verified_query( From 126df2cab4680eb8b7d97d6cde1f887a643489da Mon Sep 17 00:00:00 2001 From: lmatz Date: Wed, 18 Dec 2024 12:21:08 +0800 Subject: [PATCH 04/15] fix error message and test cases --- src/frontend/src/planner/query.rs | 29 +++++++++++++++-------------- 1 file changed, 15 insertions(+), 14 deletions(-) diff --git a/src/frontend/src/planner/query.rs b/src/frontend/src/planner/query.rs index fc125574bc689..b6073370044a9 100644 --- a/src/frontend/src/planner/query.rs +++ b/src/frontend/src/planner/query.rs @@ -63,26 +63,27 @@ impl Planner { ) .into()); } - let limit_original = limit.clone(); - let limit_err = ErrorCode::ExprError( - format!( - "expects an integer or expression after LIMIT, but found:{:?}", - limit_original - ) - .into(), - ); let limit_cast_to_bigint = limit.cast_explicit(DataType::Int64).map_err(|_| { RwError::from(ErrorCode::ExprError( - format!( - "expects an integer or expression after LIMIT, but found:{:?}", - limit_original - ) + "expects an integer or expression that can be evaluated to an integer after LIMIT" .into(), )) })?; let limit = match limit_cast_to_bigint.fold_const() { - Ok(Some(datum)) => datum.as_integral() as u64, - _ => return Err(limit_err.into()), + Ok(Some(datum)) => { + let value = datum.as_integral(); + if value < 0 { + return Err(ErrorCode::ExprError( + format!("LIMIT must not be negative, but found: {}", value).into(), + ) + .into()); + } + value as u64 + } + _ => return Err(ErrorCode::ExprError( + "expects an integer or expression that can be evaluated to an integer after LIMIT" + .into(), + ).into()), }; let offset = offset.unwrap_or_default(); From 21848a35bc4b0f8a7f1acc124995cc24582d768a Mon Sep 17 00:00:00 2001 From: lmatz Date: Wed, 18 Dec 2024 15:48:23 +0800 Subject: [PATCH 05/15] revert changes for fetch and add test cases for limit --- e2e_test/batch/order/negative_offset.slt.part | 28 +++++++++++++++++++ src/frontend/src/binder/query.rs | 2 +- src/sqlparser/src/ast/query.rs | 2 +- src/sqlparser/src/parser.rs | 2 +- src/sqlparser/tests/sqlparser_common.rs | 4 +-- 5 files changed, 33 insertions(+), 5 deletions(-) diff --git a/e2e_test/batch/order/negative_offset.slt.part b/e2e_test/batch/order/negative_offset.slt.part index 88c5c47ed15a2..38dadc4e1fa11 100644 --- a/e2e_test/batch/order/negative_offset.slt.part +++ b/e2e_test/batch/order/negative_offset.slt.part @@ -15,6 +15,34 @@ SELECT * FROM generate_series(0,10,1) LIMIT -3; statement error SELECT * FROM generate_series(0,10,1) LIMIT -1%; +statement ok +SELECT * FROM generate_series(0,10,1) as t(v) order by v limit 1+2; +---- +0 +1 +2 + + +statement ok +SELECT * FROM generate_series(0,10,1) as t(v) order by v limit 1.5*2; +--- +0 +1 +2 + + +statement ok +SELECT * FROM generate_series(0,10,1) as t(v) order by v limit -3+4; +--- +0 + +statement ok +SELECT * FROM generate_series(0,10,1) as t(v) order by v limit 1.5*2 OFFSET 2; +--- +2 +3 +4 + statement ok CREATE TABLE integers(k int); diff --git a/src/frontend/src/binder/query.rs b/src/frontend/src/binder/query.rs index c7a1118662f6c..ab1de8e625ba1 100644 --- a/src/frontend/src/binder/query.rs +++ b/src/frontend/src/binder/query.rs @@ -174,7 +174,7 @@ impl Binder { ) => { with_ties = fetch_with_ties; match quantity { - Some(v) => Some(v), + Some(v) => Some(Expr::Value(Value::Number(v))), None => Some(Expr::Value(Value::Number("1".to_owned()))), } } diff --git a/src/sqlparser/src/ast/query.rs b/src/sqlparser/src/ast/query.rs index 310b9ceb882d2..925023dfa0466 100644 --- a/src/sqlparser/src/ast/query.rs +++ b/src/sqlparser/src/ast/query.rs @@ -655,7 +655,7 @@ impl fmt::Display for OrderByExpr { #[cfg_attr(feature = "serde", derive(Serialize, Deserialize))] pub struct Fetch { pub with_ties: bool, - pub quantity: Option, + pub quantity: Option, } impl fmt::Display for Fetch { diff --git a/src/sqlparser/src/parser.rs b/src/sqlparser/src/parser.rs index 17d7e77911a52..8aad038a56cd1 100644 --- a/src/sqlparser/src/parser.rs +++ b/src/sqlparser/src/parser.rs @@ -5386,7 +5386,7 @@ impl Parser<'_> { { None } else { - let quantity = self.parse_expr()?; + let quantity = self.parse_number_value()?; self.expect_one_of_keywords(&[Keyword::ROW, Keyword::ROWS])?; Some(quantity) }; diff --git a/src/sqlparser/tests/sqlparser_common.rs b/src/sqlparser/tests/sqlparser_common.rs index 9ce0b620bf963..9018a4f00510b 100644 --- a/src/sqlparser/tests/sqlparser_common.rs +++ b/src/sqlparser/tests/sqlparser_common.rs @@ -3551,7 +3551,7 @@ fn parse_fetch() { let fetch_first_two_rows_only = Some(Fetch { with_ties: false, - quantity: Some(Expr::Value(Value::Number("2".to_owned()))), + quantity: Some("2".to_owned()), }); let ast = verified_query("SELECT foo FROM bar FETCH FIRST 2 ROWS ONLY"); assert_eq!(ast.fetch, fetch_first_two_rows_only); @@ -3576,7 +3576,7 @@ fn parse_fetch() { ast.fetch, Some(Fetch { with_ties: true, - quantity: Some(Expr::Value(Value::Number("2".to_owned()))), + quantity: Some("2".to_owned()), }) ); let ast = verified_query( From 51e55855f1425a2b856153f4db13e779ea0eac12 Mon Sep 17 00:00:00 2001 From: lmatz Date: Wed, 18 Dec 2024 15:50:24 +0800 Subject: [PATCH 06/15] format --- src/frontend/src/planner/query.rs | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/src/frontend/src/planner/query.rs b/src/frontend/src/planner/query.rs index b6073370044a9..50b61f6403fc7 100644 --- a/src/frontend/src/planner/query.rs +++ b/src/frontend/src/planner/query.rs @@ -55,10 +55,7 @@ impl Planner { let limit = limit.unwrap_or(ExprImpl::literal_bigint(LIMIT_ALL_COUNT as i64)); if !limit.is_const() { return Err(ErrorCode::ExprError( - format!( - "expects an integer or expression after LIMIT, but found:{:?}", - limit - ) + "expects an integer or expression that can be evaluated to an integer after LIMIT" .into(), ) .into()); From a29c6e6d293d7b00c27945b85c59074028f2579e Mon Sep 17 00:00:00 2001 From: lmatz Date: Wed, 18 Dec 2024 16:22:00 +0800 Subject: [PATCH 07/15] fix test cases --- e2e_test/batch/order/negative_offset.slt.part | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/e2e_test/batch/order/negative_offset.slt.part b/e2e_test/batch/order/negative_offset.slt.part index 38dadc4e1fa11..8991ef1bb023c 100644 --- a/e2e_test/batch/order/negative_offset.slt.part +++ b/e2e_test/batch/order/negative_offset.slt.part @@ -15,28 +15,26 @@ SELECT * FROM generate_series(0,10,1) LIMIT -3; statement error SELECT * FROM generate_series(0,10,1) LIMIT -1%; -statement ok +query I SELECT * FROM generate_series(0,10,1) as t(v) order by v limit 1+2; ---- 0 1 2 - -statement ok +query I SELECT * FROM generate_series(0,10,1) as t(v) order by v limit 1.5*2; --- 0 1 2 - -statement ok +query I SELECT * FROM generate_series(0,10,1) as t(v) order by v limit -3+4; --- 0 -statement ok +query I SELECT * FROM generate_series(0,10,1) as t(v) order by v limit 1.5*2 OFFSET 2; --- 2 From 8663d2c07d96b62ae9443b224b9f0e6b4d858d73 Mon Sep 17 00:00:00 2001 From: lmatz Date: Wed, 18 Dec 2024 17:19:08 +0800 Subject: [PATCH 08/15] fix format --- e2e_test/batch/order/negative_offset.slt.part | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/e2e_test/batch/order/negative_offset.slt.part b/e2e_test/batch/order/negative_offset.slt.part index 8991ef1bb023c..8acfd257e25cd 100644 --- a/e2e_test/batch/order/negative_offset.slt.part +++ b/e2e_test/batch/order/negative_offset.slt.part @@ -24,19 +24,19 @@ SELECT * FROM generate_series(0,10,1) as t(v) order by v limit 1+2; query I SELECT * FROM generate_series(0,10,1) as t(v) order by v limit 1.5*2; ---- +---- 0 1 2 query I SELECT * FROM generate_series(0,10,1) as t(v) order by v limit -3+4; ---- +---- 0 query I SELECT * FROM generate_series(0,10,1) as t(v) order by v limit 1.5*2 OFFSET 2; ---- +---- 2 3 4 From 0e9a847aec27ea1d7eb13b1e20385df5e81644ff Mon Sep 17 00:00:00 2001 From: lmatz Date: Sun, 22 Dec 2024 16:45:42 +0800 Subject: [PATCH 09/15] move evaluation to binder and add test cases --- e2e_test/batch/order/negative_offset.slt.part | 26 ------------- e2e_test/batch/order/test_limit.slt.part | 39 +++++++++++++++++++ src/frontend/src/binder/query.rs | 38 ++++++++++++++++-- src/frontend/src/planner/query.rs | 35 +---------------- 4 files changed, 76 insertions(+), 62 deletions(-) diff --git a/e2e_test/batch/order/negative_offset.slt.part b/e2e_test/batch/order/negative_offset.slt.part index 8acfd257e25cd..88c5c47ed15a2 100644 --- a/e2e_test/batch/order/negative_offset.slt.part +++ b/e2e_test/batch/order/negative_offset.slt.part @@ -15,32 +15,6 @@ SELECT * FROM generate_series(0,10,1) LIMIT -3; statement error SELECT * FROM generate_series(0,10,1) LIMIT -1%; -query I -SELECT * FROM generate_series(0,10,1) as t(v) order by v limit 1+2; ----- -0 -1 -2 - -query I -SELECT * FROM generate_series(0,10,1) as t(v) order by v limit 1.5*2; ----- -0 -1 -2 - -query I -SELECT * FROM generate_series(0,10,1) as t(v) order by v limit -3+4; ----- -0 - -query I -SELECT * FROM generate_series(0,10,1) as t(v) order by v limit 1.5*2 OFFSET 2; ----- -2 -3 -4 - statement ok CREATE TABLE integers(k int); diff --git a/e2e_test/batch/order/test_limit.slt.part b/e2e_test/batch/order/test_limit.slt.part index bb9db52868a61..6a7a4bc978fd1 100644 --- a/e2e_test/batch/order/test_limit.slt.part +++ b/e2e_test/batch/order/test_limit.slt.part @@ -182,6 +182,45 @@ INSERT INTO integers VALUES (1), (2), (3), (4), (5); # 4 # 5 +# expression in the limit clause +query I +SELECT * FROM generate_series(0,10,1) as t(v) order by v limit 1+2; +---- +0 +1 +2 + +query I +SELECT * FROM generate_series(0,10,1) as t(v) order by v limit 1.5*2; +---- +0 +1 +2 + +query I +SELECT * FROM generate_series(0,10,1) as t(v) order by v limit -3+4; +---- +0 + +query I +SELECT * FROM generate_series(0,10,1) as t(v) order by v limit 1.5*2 OFFSET 2; +---- +2 +3 +4 + +query I +SELECT * FROM generate_series(0,3,1) as t(v) order by v limit NULL + 1; +---- +0 +1 +2 + +query I +SELECT * FROM generate_series(0,3,1) as t(v) order by v limit 1.1::Decimal; +---- +0 + # Subqueries that return negative values statement error diff --git a/src/frontend/src/binder/query.rs b/src/frontend/src/binder/query.rs index ab1de8e625ba1..c36d31cbfe1b0 100644 --- a/src/frontend/src/binder/query.rs +++ b/src/frontend/src/binder/query.rs @@ -29,7 +29,7 @@ use super::statement::RewriteExprsRecursive; use super::BoundValues; use crate::binder::bind_context::{BindingCte, RecursiveUnion}; use crate::binder::{Binder, BoundSetExpr}; -use crate::error::{ErrorCode, Result}; +use crate::error::{ErrorCode, Result, RwError}; use crate::expr::{CorrelatedId, Depth, ExprImpl, ExprRewriter}; /// A validated sql query, including order and union. @@ -38,7 +38,7 @@ use crate::expr::{CorrelatedId, Depth, ExprImpl, ExprRewriter}; pub struct BoundQuery { pub body: BoundSetExpr, pub order: Vec, - pub limit: Option, + pub limit: Option, pub offset: Option, pub with_ties: bool, pub extra_order_exprs: Vec, @@ -181,7 +181,39 @@ impl Binder { (Some(limit), None) => Some(limit), (Some(_), Some(_)) => unreachable!(), // parse error }; - let limit = limit.map(|expr| self.bind_expr(expr)).transpose()?; + let limit_expr = limit.map(|expr| self.bind_expr(expr)).transpose()?; + let limit = if let Some(limit_expr) = limit_expr { + let limit_cast_to_bigint = limit_expr.cast_assign(DataType::Int64).map_err(|_| { + RwError::from(ErrorCode::ExprError( + "expects an integer or expression that can be evaluated to an integer after LIMIT" + .into(), + )) + })?; + let limit = match limit_cast_to_bigint.try_fold_const() { + Some(Ok(Some(datum))) => { + let value = datum.as_integral(); + if value < 0 { + return Err(ErrorCode::ExprError( + format!("LIMIT must not be negative, but found: {}", value).into(), + ) + .into()); + } + value as u64 + } + // If evaluated to NULL, we follow PG to treat NULL as no limit + Some(Ok(None)) => { + u64::MAX + } + // wrong type, not const, eval error all belongs to this branch + _ => return Err(ErrorCode::ExprError( + "expects an integer or expression that can be evaluated to an integer after LIMIT" + .into(), + ).into()), + }; + Some(limit) + } else { + None + }; let offset = offset .map(|s| parse_non_negative_i64("OFFSET", &s)) diff --git a/src/frontend/src/planner/query.rs b/src/frontend/src/planner/query.rs index 50b61f6403fc7..099e318632564 100644 --- a/src/frontend/src/planner/query.rs +++ b/src/frontend/src/planner/query.rs @@ -13,11 +13,9 @@ // limitations under the License. use fixedbitset::FixedBitSet; -use risingwave_common::types::DataType; use crate::binder::BoundQuery; -use crate::error::{ErrorCode, Result, RwError}; -use crate::expr::ExprImpl; +use crate::error::Result; use crate::optimizer::plan_node::{LogicalLimit, LogicalTopN}; use crate::optimizer::property::{Order, RequiredDist}; use crate::optimizer::PlanRoot; @@ -52,36 +50,7 @@ impl Planner { let func_dep = plan.functional_dependency(); order = func_dep.minimize_order_key(order, &[]); - let limit = limit.unwrap_or(ExprImpl::literal_bigint(LIMIT_ALL_COUNT as i64)); - if !limit.is_const() { - return Err(ErrorCode::ExprError( - "expects an integer or expression that can be evaluated to an integer after LIMIT" - .into(), - ) - .into()); - } - let limit_cast_to_bigint = limit.cast_explicit(DataType::Int64).map_err(|_| { - RwError::from(ErrorCode::ExprError( - "expects an integer or expression that can be evaluated to an integer after LIMIT" - .into(), - )) - })?; - let limit = match limit_cast_to_bigint.fold_const() { - Ok(Some(datum)) => { - let value = datum.as_integral(); - if value < 0 { - return Err(ErrorCode::ExprError( - format!("LIMIT must not be negative, but found: {}", value).into(), - ) - .into()); - } - value as u64 - } - _ => return Err(ErrorCode::ExprError( - "expects an integer or expression that can be evaluated to an integer after LIMIT" - .into(), - ).into()), - }; + let limit = limit.unwrap_or(LIMIT_ALL_COUNT); let offset = offset.unwrap_or_default(); plan = if order.column_orders.is_empty() { From aac965c343a5821fe7814b2d979a930edc688bff Mon Sep 17 00:00:00 2001 From: lmatz Date: Sun, 22 Dec 2024 17:05:33 +0800 Subject: [PATCH 10/15] add more test cases --- e2e_test/batch/order/test_limit.slt.part | 33 +++++++++++++++++++++++- 1 file changed, 32 insertions(+), 1 deletion(-) diff --git a/e2e_test/batch/order/test_limit.slt.part b/e2e_test/batch/order/test_limit.slt.part index 6a7a4bc978fd1..9bcf832215ff4 100644 --- a/e2e_test/batch/order/test_limit.slt.part +++ b/e2e_test/batch/order/test_limit.slt.part @@ -215,12 +215,43 @@ SELECT * FROM generate_series(0,3,1) as t(v) order by v limit NULL + 1; 0 1 2 +3 + +query I +SELECT * FROM generate_series(0,3,1) as t(v) order by v limit NULL + '1'::bigint; +---- +0 +1 +2 +3 + +statement error +SELECT * FROM generate_series(0,3,1) as t(v) order by v limit NULL + '1'; + +statement error +SELECT * FROM generate_series(0,3,1) as t(v) order by v limit NULL + '1'::jsonb; query I -SELECT * FROM generate_series(0,3,1) as t(v) order by v limit 1.1::Decimal; +SELECT * FROM generate_series(0,3,1) as t(v) order by v limit 2.1::Decimal; ---- 0 +1 + +statement error +SELECT * FROM generate_series(0,3,1) as t(v) order by v limit '1'::jsonb; + + +query I +SELECT * FROM generate_series(0,3,1) as t(v) order by v limit '2'; +---- +0 +1 +statement error +SELECT * FROM generate_series(0,3,1) as t(v) order by v limit '-2'; + +statement error +SELECT * FROM generate_series(0,3,1) as t(v) order by v limit '2.2'; # Subqueries that return negative values statement error From 6c4a44626b912f6e8a7f9494359e0ef1fbd23647 Mon Sep 17 00:00:00 2001 From: lmatz Date: Fri, 27 Dec 2024 16:49:09 +0800 Subject: [PATCH 11/15] separate errors and add test case errors --- e2e_test/batch/order/test_limit.slt.part | 53 +++++++++++++++++++++++- src/frontend/src/binder/query.rs | 22 ++++++---- 2 files changed, 67 insertions(+), 8 deletions(-) diff --git a/e2e_test/batch/order/test_limit.slt.part b/e2e_test/batch/order/test_limit.slt.part index 9bcf832215ff4..c8f69a7ed3dfd 100644 --- a/e2e_test/batch/order/test_limit.slt.part +++ b/e2e_test/batch/order/test_limit.slt.part @@ -225,11 +225,28 @@ SELECT * FROM generate_series(0,3,1) as t(v) order by v limit NULL + '1'::bigint 2 3 + statement error SELECT * FROM generate_series(0,3,1) as t(v) order by v limit NULL + '1'; +---- +db error: ERROR: Failed to run the query + +Caused by these errors (recent errors listed first): + 1: Failed to bind expression: NULL + '1' + 2: Bind error: function add(unknown, unknown) is not unique +HINT: Could not choose a best candidate function. You might need to add explicit type casts. + statement error SELECT * FROM generate_series(0,3,1) as t(v) order by v limit NULL + '1'::jsonb; +---- +db error: ERROR: Failed to run the query + +Caused by these errors (recent errors listed first): + 1: Failed to bind expression: NULL + CAST('1' AS JSONB) + 2: function add(unknown, jsonb) does not exist + + query I SELECT * FROM generate_series(0,3,1) as t(v) order by v limit 2.1::Decimal; @@ -237,8 +254,16 @@ SELECT * FROM generate_series(0,3,1) as t(v) order by v limit 2.1::Decimal; 0 1 + statement error SELECT * FROM generate_series(0,3,1) as t(v) order by v limit '1'::jsonb; +---- +db error: ERROR: Failed to run the query + +Caused by these errors (recent errors listed first): + 1: Expr error + 2: expects an integer or expression that can be evaluated to an integer after LIMIT + query I @@ -247,11 +272,37 @@ SELECT * FROM generate_series(0,3,1) as t(v) order by v limit '2'; 0 1 -statement error + +statement error SELECT * FROM generate_series(0,3,1) as t(v) order by v limit '-2'; +---- +db error: ERROR: Failed to run the query + +Caused by these errors (recent errors listed first): + 1: Expr error + 2: LIMIT must not be negative, but found: -2 + statement error SELECT * FROM generate_series(0,3,1) as t(v) order by v limit '2.2'; +---- +db error: ERROR: Failed to run the query + +Caused by these errors (recent errors listed first): + 1: Expr error + 2: expects an integer or expression that can be evaluated to an integer after LIMIT, +but the evaluation of the expression returns error:Expr error: error while evaluating expression `str_parse('2.2')` + + +statement error +select 1 limit generate_series(1, 2); +---- +db error: ERROR: Failed to run the query + +Caused by these errors (recent errors listed first): + 1: Expr error + 2: expects an integer or expression that can be evaluated to an integer after LIMIT, but found non-const expression + # Subqueries that return negative values statement error diff --git a/src/frontend/src/binder/query.rs b/src/frontend/src/binder/query.rs index c36d31cbfe1b0..085b3342d4dba 100644 --- a/src/frontend/src/binder/query.rs +++ b/src/frontend/src/binder/query.rs @@ -183,6 +183,7 @@ impl Binder { }; let limit_expr = limit.map(|expr| self.bind_expr(expr)).transpose()?; let limit = if let Some(limit_expr) = limit_expr { + // wrong type error is handled here let limit_cast_to_bigint = limit_expr.cast_assign(DataType::Int64).map_err(|_| { RwError::from(ErrorCode::ExprError( "expects an integer or expression that can be evaluated to an integer after LIMIT" @@ -191,24 +192,31 @@ impl Binder { })?; let limit = match limit_cast_to_bigint.try_fold_const() { Some(Ok(Some(datum))) => { - let value = datum.as_integral(); - if value < 0 { + let value = datum.as_int64(); + if *value < 0 { return Err(ErrorCode::ExprError( - format!("LIMIT must not be negative, but found: {}", value).into(), + format!("LIMIT must not be negative, but found: {}", *value).into(), ) .into()); } - value as u64 + *value as u64 } // If evaluated to NULL, we follow PG to treat NULL as no limit Some(Ok(None)) => { u64::MAX } - // wrong type, not const, eval error all belongs to this branch - _ => return Err(ErrorCode::ExprError( - "expects an integer or expression that can be evaluated to an integer after LIMIT" + // not const error + None => return Err(ErrorCode::ExprError( + "expects an integer or expression that can be evaluated to an integer after LIMIT, but found non-const expression" .into(), ).into()), + // eval error + Some(Err(e)) => { + return Err(ErrorCode::ExprError( + format!("expects an integer or expression that can be evaluated to an integer after LIMIT,\nbut the evaluation of the expression returns error:{}", e + ).into(), + ).into()) + } }; Some(limit) } else { From 453d221b746925be47c5508a24d102325697fe3c Mon Sep 17 00:00:00 2001 From: lmatz Date: Fri, 27 Dec 2024 17:05:37 +0800 Subject: [PATCH 12/15] revove trailing space --- e2e_test/batch/order/test_limit.slt.part | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/e2e_test/batch/order/test_limit.slt.part b/e2e_test/batch/order/test_limit.slt.part index c8f69a7ed3dfd..7bc321e0eb6cc 100644 --- a/e2e_test/batch/order/test_limit.slt.part +++ b/e2e_test/batch/order/test_limit.slt.part @@ -273,7 +273,7 @@ SELECT * FROM generate_series(0,3,1) as t(v) order by v limit '2'; 1 -statement error +statement error SELECT * FROM generate_series(0,3,1) as t(v) order by v limit '-2'; ---- db error: ERROR: Failed to run the query @@ -294,7 +294,7 @@ Caused by these errors (recent errors listed first): but the evaluation of the expression returns error:Expr error: error while evaluating expression `str_parse('2.2')` -statement error +statement error select 1 limit generate_series(1, 2); ---- db error: ERROR: Failed to run the query From 031dcc29be9ff9eaa471ef4ed04bafa036606298 Mon Sep 17 00:00:00 2001 From: lmatz Date: Fri, 27 Dec 2024 17:34:56 +0800 Subject: [PATCH 13/15] use as_report --- src/frontend/src/binder/query.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/frontend/src/binder/query.rs b/src/frontend/src/binder/query.rs index 085b3342d4dba..f7d4d607837e4 100644 --- a/src/frontend/src/binder/query.rs +++ b/src/frontend/src/binder/query.rs @@ -213,7 +213,7 @@ impl Binder { // eval error Some(Err(e)) => { return Err(ErrorCode::ExprError( - format!("expects an integer or expression that can be evaluated to an integer after LIMIT,\nbut the evaluation of the expression returns error:{}", e + format!("expects an integer or expression that can be evaluated to an integer after LIMIT,\nbut the evaluation of the expression returns error:{}", e.as_report() ).into(), ).into()) } From 11b61f7b2ae5c4d7870e4f8a0ca7d16f2962e069 Mon Sep 17 00:00:00 2001 From: lmatz Date: Fri, 27 Dec 2024 18:06:51 +0800 Subject: [PATCH 14/15] update test case --- e2e_test/batch/order/test_limit.slt.part | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/e2e_test/batch/order/test_limit.slt.part b/e2e_test/batch/order/test_limit.slt.part index 7bc321e0eb6cc..00230d2d7beaa 100644 --- a/e2e_test/batch/order/test_limit.slt.part +++ b/e2e_test/batch/order/test_limit.slt.part @@ -291,7 +291,7 @@ db error: ERROR: Failed to run the query Caused by these errors (recent errors listed first): 1: Expr error 2: expects an integer or expression that can be evaluated to an integer after LIMIT, -but the evaluation of the expression returns error:Expr error: error while evaluating expression `str_parse('2.2')` +but the evaluation of the expression returns error:Expr error: error while evaluating expression `str_parse('2.2')`: Parse error: bigint invalid digit found in string statement error From 705aaa753a4e3abdf64c407c285d6a06bc01c783 Mon Sep 17 00:00:00 2001 From: lmatz Date: Fri, 27 Dec 2024 19:44:02 +0800 Subject: [PATCH 15/15] increase timeout for integration tests --- ci/workflows/pull-request.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ci/workflows/pull-request.yml b/ci/workflows/pull-request.yml index 1139681a4d5e6..3288d496b0965 100644 --- a/ci/workflows/pull-request.yml +++ b/ci/workflows/pull-request.yml @@ -607,7 +607,7 @@ steps: config: ci/docker-compose.yml mount-buildkite-agent: true - ./ci/plugins/upload-failure-logs - timeout_in_minutes: 20 + timeout_in_minutes: 22 retry: *auto-retry - label: "end-to-end test (deterministic simulation)"