Skip to content

Commit eb698ee

Browse files
yoavcloudyoavcloud
committed
Code review comments
1 parent fa78cb5 commit eb698ee

File tree

11 files changed

+55
-37
lines changed

11 files changed

+55
-37
lines changed

‎src/ast/query.rs

Lines changed: 4 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -279,6 +279,8 @@ pub struct Select {
279279
pub distinct: Option<Distinct>,
280280
/// MSSQL syntax: `TOP (<N>) [ PERCENT ] [ WITH TIES ]`
281281
pub top: Option<Top>,
282+
/// Whether the top was located before `ALL`/`DISTINCT`
283+
pub top_before_distinct: bool,
282284
/// projection expressions
283285
pub projection: Vec<SelectItem>,
284286
/// INTO
@@ -328,15 +330,15 @@ impl fmt::Display for Select {
328330
}
329331

330332
if let Some(ref top) = self.top {
331-
if top.is_before_distinct {
333+
if self.top_before_distinct {
332334
write!(f, " {top}")?;
333335
}
334336
}
335337
if let Some(ref distinct) = self.distinct {
336338
write!(f, " {distinct}")?;
337339
}
338340
if let Some(ref top) = self.top {
339-
if !top.is_before_distinct {
341+
if !self.top_before_distinct {
340342
write!(f, " {top}")?;
341343
}
342344
}
@@ -2004,9 +2006,6 @@ pub struct Top {
20042006
/// MSSQL only.
20052007
pub percent: bool,
20062008
pub quantity: Option<TopQuantity>,
2007-
// Whether this option was parsed before `ALL`/`DISTINCT`
2008-
// See `Dialect::expects_top_before_distinct`
2009-
pub is_before_distinct: bool,
20102009
}
20112010

20122011
#[derive(Debug, Clone, PartialEq, PartialOrd, Eq, Ord, Hash)]

‎src/dialect/mod.rs

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -592,8 +592,8 @@ pub trait Dialect: Debug + Any {
592592
}
593593

594594
/// Returns true if this dialect expects the the `TOP` option
595-
/// before the `ALL`/`DISTINCT` options
596-
fn expects_top_before_distinct(&self) -> bool {
595+
/// before the `ALL`/`DISTINCT` options in a `SELECT` statement.
596+
fn supports_top_before_distinct(&self) -> bool {
597597
false
598598
}
599599
}

‎src/dialect/redshift.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -71,7 +71,7 @@ impl Dialect for RedshiftSqlDialect {
7171

7272
/// Redshift expects the `TOP` option before the `ALL/DISTINCT` option:
7373
/// <https://docs.aws.amazon.com/redshift/latest/dg/r_SELECT_list.html#r_SELECT_list-parameters>
74-
fn expects_top_before_distinct(&self) -> bool {
74+
fn supports_top_before_distinct(&self) -> bool {
7575
true
7676
}
7777
}

‎src/parser/mod.rs

Lines changed: 14 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -9176,15 +9176,16 @@ impl<'a> Parser<'a> {
91769176
None
91779177
};
91789178

9179-
let (distinct, top) = if self.dialect.expects_top_before_distinct() {
9180-
let top = self.maybe_parse_top(true)?;
9181-
let distinct = self.parse_all_or_distinct()?;
9182-
(distinct, top)
9183-
} else {
9184-
let distinct = self.parse_all_or_distinct()?;
9185-
let top = self.maybe_parse_top(false)?;
9186-
(distinct, top)
9187-
};
9179+
let mut top_before_distinct = false;
9180+
let mut top = None;
9181+
if self.dialect.supports_top_before_distinct() && self.parse_keyword(Keyword::TOP) {
9182+
top = Some(self.parse_top()?);
9183+
top_before_distinct = true;
9184+
}
9185+
let distinct = self.parse_all_or_distinct()?;
9186+
if !self.dialect.supports_top_before_distinct() && self.parse_keyword(Keyword::TOP) {
9187+
top = Some(self.parse_top()?);
9188+
}
91889189

91899190
let projection = self.parse_projection()?;
91909191

@@ -9327,6 +9328,7 @@ impl<'a> Parser<'a> {
93279328
Ok(Select {
93289329
distinct,
93299330
top,
9331+
top_before_distinct,
93309332
projection,
93319333
into,
93329334
from,
@@ -11537,14 +11539,7 @@ impl<'a> Parser<'a> {
1153711539

1153811540
/// Parse a TOP clause, MSSQL equivalent of LIMIT,
1153911541
/// that follows after `SELECT [DISTINCT]`.
11540-
pub fn maybe_parse_top(
11541-
&mut self,
11542-
is_before_distinct: bool,
11543-
) -> Result<Option<Top>, ParserError> {
11544-
if !self.parse_keyword(Keyword::TOP) {
11545-
return Ok(None);
11546-
}
11547-
11542+
pub fn parse_top(&mut self) -> Result<Top, ParserError> {
1154811543
let quantity = if self.consume_token(&Token::LParen) {
1154911544
let quantity = self.parse_expr()?;
1155011545
self.expect_token(&Token::RParen)?;
@@ -11562,12 +11557,11 @@ impl<'a> Parser<'a> {
1156211557

1156311558
let with_ties = self.parse_keywords(&[Keyword::WITH, Keyword::TIES]);
1156411559

11565-
Ok(Some(Top {
11560+
Ok(Top {
1156611561
with_ties,
1156711562
percent,
1156811563
quantity,
11569-
is_before_distinct,
11570-
}))
11564+
})
1157111565
}
1157211566

1157311567
/// Parse a LIMIT clause

‎tests/sqlparser_clickhouse.rs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -40,6 +40,7 @@ fn parse_map_access_expr() {
4040
Select {
4141
distinct: None,
4242
top: None,
43+
top_before_distinct: false,
4344
projection: vec![UnnamedExpr(MapAccess {
4445
column: Box::new(Identifier(Ident {
4546
value: "string_values".to_string(),

‎tests/sqlparser_common.rs

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -379,6 +379,7 @@ fn parse_update_set_from() {
379379
body: Box::new(SetExpr::Select(Box::new(Select {
380380
distinct: None,
381381
top: None,
382+
top_before_distinct: false,
382383
projection: vec![
383384
SelectItem::UnnamedExpr(Expr::Identifier(Ident::new("name"))),
384385
SelectItem::UnnamedExpr(Expr::Identifier(Ident::new("id"))),
@@ -4649,6 +4650,7 @@ fn test_parse_named_window() {
46494650
let expected = Select {
46504651
distinct: None,
46514652
top: None,
4653+
top_before_distinct: false,
46524654
projection: vec![
46534655
SelectItem::ExprWithAlias {
46544656
expr: Expr::Function(Function {
@@ -5289,6 +5291,7 @@ fn parse_interval_and_or_xor() {
52895291
body: Box::new(SetExpr::Select(Box::new(Select {
52905292
distinct: None,
52915293
top: None,
5294+
top_before_distinct: false,
52925295
projection: vec![UnnamedExpr(Expr::Identifier(Ident {
52935296
value: "col".to_string(),
52945297
quote_style: None,
@@ -7367,6 +7370,7 @@ fn lateral_function() {
73677370
let expected = Select {
73687371
distinct: None,
73697372
top: None,
7373+
top_before_distinct: false,
73707374
projection: vec![SelectItem::Wildcard(WildcardAdditionalOptions {
73717375
opt_ilike: None,
73727376
opt_exclude: None,
@@ -8215,6 +8219,7 @@ fn parse_merge() {
82158219
body: Box::new(SetExpr::Select(Box::new(Select {
82168220
distinct: None,
82178221
top: None,
8222+
top_before_distinct: false,
82188223
projection: vec![SelectItem::Wildcard(
82198224
WildcardAdditionalOptions::default()
82208225
)],
@@ -9803,6 +9808,7 @@ fn parse_unload() {
98039808
body: Box::new(SetExpr::Select(Box::new(Select {
98049809
distinct: None,
98059810
top: None,
9811+
top_before_distinct: false,
98069812
projection: vec![UnnamedExpr(Expr::Identifier(Ident::new("cola"))),],
98079813
into: None,
98089814
from: vec![TableWithJoins {
@@ -9978,6 +9984,7 @@ fn parse_connect_by() {
99789984
let expect_query = Select {
99799985
distinct: None,
99809986
top: None,
9987+
top_before_distinct: false,
99819988
projection: vec![
99829989
SelectItem::UnnamedExpr(Expr::Identifier(Ident::new("employee_id"))),
99839990
SelectItem::UnnamedExpr(Expr::Identifier(Ident::new("manager_id"))),
@@ -10064,6 +10071,7 @@ fn parse_connect_by() {
1006410071
Select {
1006510072
distinct: None,
1006610073
top: None,
10074+
top_before_distinct: false,
1006710075
projection: vec![
1006810076
SelectItem::UnnamedExpr(Expr::Identifier(Ident::new("employee_id"))),
1006910077
SelectItem::UnnamedExpr(Expr::Identifier(Ident::new("manager_id"))),
@@ -11399,3 +11407,13 @@ fn test_show_dbs_schemas_tables_views() {
1139911407
verified_stmt("SHOW MATERIALIZED VIEWS FROM db1");
1140011408
verified_stmt("SHOW MATERIALIZED VIEWS FROM db1 'abc'");
1140111409
}
11410+
11411+
#[test]
11412+
fn test_select_top() {
11413+
let dialects = all_dialects_where(|d| d.supports_top_before_distinct());
11414+
dialects.one_statement_parses_to("SELECT ALL * FROM tbl", "SELECT * FROM tbl");
11415+
dialects.verified_stmt("SELECT TOP 3 * FROM tbl");
11416+
dialects.one_statement_parses_to("SELECT TOP 3 ALL * FROM tbl", "SELECT TOP 3 * FROM tbl");
11417+
dialects.verified_stmt("SELECT TOP 3 DISTINCT * FROM tbl");
11418+
dialects.verified_stmt("SELECT TOP 3 DISTINCT a, b, c FROM tbl");
11419+
}

‎tests/sqlparser_duckdb.rs

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -261,6 +261,7 @@ fn test_select_union_by_name() {
261261
left: Box::<SetExpr>::new(SetExpr::Select(Box::new(Select {
262262
distinct: None,
263263
top: None,
264+
top_before_distinct: false,
264265
projection: vec![SelectItem::Wildcard(WildcardAdditionalOptions {
265266
opt_ilike: None,
266267
opt_exclude: None,
@@ -301,6 +302,7 @@ fn test_select_union_by_name() {
301302
right: Box::<SetExpr>::new(SetExpr::Select(Box::new(Select {
302303
distinct: None,
303304
top: None,
305+
top_before_distinct: false,
304306
projection: vec![SelectItem::Wildcard(WildcardAdditionalOptions {
305307
opt_ilike: None,
306308
opt_exclude: None,

‎tests/sqlparser_mssql.rs

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -114,6 +114,7 @@ fn parse_create_procedure() {
114114
body: Box::new(SetExpr::Select(Box::new(Select {
115115
distinct: None,
116116
top: None,
117+
top_before_distinct: false,
117118
projection: vec![SelectItem::UnnamedExpr(Expr::Value(number("1")))],
118119
into: None,
119120
from: vec![],
@@ -514,6 +515,7 @@ fn parse_substring_in_select() {
514515
body: Box::new(SetExpr::Select(Box::new(Select {
515516
distinct: Some(Distinct::Distinct),
516517
top: None,
518+
top_before_distinct: false,
517519
projection: vec![SelectItem::UnnamedExpr(Expr::Substring {
518520
expr: Box::new(Expr::Identifier(Ident {
519521
value: "description".to_string(),

‎tests/sqlparser_mysql.rs

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -957,6 +957,7 @@ fn parse_escaped_quote_identifiers_with_escape() {
957957
body: Box::new(SetExpr::Select(Box::new(Select {
958958
distinct: None,
959959
top: None,
960+
top_before_distinct: false,
960961
projection: vec![SelectItem::UnnamedExpr(Expr::Identifier(Ident {
961962
value: "quoted ` identifier".into(),
962963
quote_style: Some('`'),
@@ -1007,6 +1008,7 @@ fn parse_escaped_quote_identifiers_with_no_escape() {
10071008
body: Box::new(SetExpr::Select(Box::new(Select {
10081009
distinct: None,
10091010
top: None,
1011+
top_before_distinct: false,
10101012
projection: vec![SelectItem::UnnamedExpr(Expr::Identifier(Ident {
10111013
value: "quoted `` identifier".into(),
10121014
quote_style: Some('`'),
@@ -1050,6 +1052,7 @@ fn parse_escaped_backticks_with_escape() {
10501052
body: Box::new(SetExpr::Select(Box::new(Select {
10511053
distinct: None,
10521054
top: None,
1055+
top_before_distinct: false,
10531056
projection: vec![SelectItem::UnnamedExpr(Expr::Identifier(Ident {
10541057
value: "`quoted identifier`".into(),
10551058
quote_style: Some('`'),
@@ -1097,6 +1100,7 @@ fn parse_escaped_backticks_with_no_escape() {
10971100
body: Box::new(SetExpr::Select(Box::new(Select {
10981101
distinct: None,
10991102
top: None,
1103+
top_before_distinct: false,
11001104
projection: vec![SelectItem::UnnamedExpr(Expr::Identifier(Ident {
11011105
value: "``quoted identifier``".into(),
11021106
quote_style: Some('`'),
@@ -1741,6 +1745,7 @@ fn parse_select_with_numeric_prefix_column_name() {
17411745
Box::new(SetExpr::Select(Box::new(Select {
17421746
distinct: None,
17431747
top: None,
1748+
top_before_distinct: false,
17441749
projection: vec![SelectItem::UnnamedExpr(Expr::Identifier(Ident::new(
17451750
"123col_$@123abc"
17461751
)))],
@@ -1795,6 +1800,7 @@ fn parse_select_with_concatenation_of_exp_number_and_numeric_prefix_column() {
17951800
Box::new(SetExpr::Select(Box::new(Select {
17961801
distinct: None,
17971802
top: None,
1803+
top_before_distinct: false,
17981804
projection: vec![
17991805
SelectItem::UnnamedExpr(Expr::Value(number("123e4"))),
18001806
SelectItem::UnnamedExpr(Expr::Identifier(Ident::new("123col_$@123abc")))
@@ -2295,6 +2301,7 @@ fn parse_substring_in_select() {
22952301
body: Box::new(SetExpr::Select(Box::new(Select {
22962302
distinct: Some(Distinct::Distinct),
22972303
top: None,
2304+
top_before_distinct: false,
22982305
projection: vec![SelectItem::UnnamedExpr(Expr::Substring {
22992306
expr: Box::new(Expr::Identifier(Ident {
23002307
value: "description".to_string(),
@@ -2616,6 +2623,7 @@ fn parse_hex_string_introducer() {
26162623
body: Box::new(SetExpr::Select(Box::new(Select {
26172624
distinct: None,
26182625
top: None,
2626+
top_before_distinct: false,
26192627
projection: vec![SelectItem::UnnamedExpr(Expr::IntroducedString {
26202628
introducer: "_latin1".to_string(),
26212629
value: Value::HexStringLiteral("4D7953514C".to_string())

‎tests/sqlparser_postgres.rs

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1165,6 +1165,7 @@ fn parse_copy_to() {
11651165
body: Box::new(SetExpr::Select(Box::new(Select {
11661166
distinct: None,
11671167
top: None,
1168+
top_before_distinct: false,
11681169
projection: vec![
11691170
SelectItem::ExprWithAlias {
11701171
expr: Expr::Value(number("42")),
@@ -2505,6 +2506,7 @@ fn parse_array_subquery_expr() {
25052506
left: Box::new(SetExpr::Select(Box::new(Select {
25062507
distinct: None,
25072508
top: None,
2509+
top_before_distinct: false,
25082510
projection: vec![SelectItem::UnnamedExpr(Expr::Value(number("1")))],
25092511
into: None,
25102512
from: vec![],
@@ -2525,6 +2527,7 @@ fn parse_array_subquery_expr() {
25252527
right: Box::new(SetExpr::Select(Box::new(Select {
25262528
distinct: None,
25272529
top: None,
2530+
top_before_distinct: false,
25282531
projection: vec![SelectItem::UnnamedExpr(Expr::Value(number("2")))],
25292532
into: None,
25302533
from: vec![],

‎tests/sqlparser_redshift.rs

Lines changed: 0 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -196,12 +196,3 @@ fn test_create_view_with_no_schema_binding() {
196196
redshift_and_generic()
197197
.verified_stmt("CREATE VIEW myevent AS SELECT eventname FROM event WITH NO SCHEMA BINDING");
198198
}
199-
200-
#[test]
201-
fn test_select_top() {
202-
redshift().one_statement_parses_to("SELECT ALL * FROM tbl", "SELECT * FROM tbl");
203-
redshift().verified_stmt("SELECT TOP 3 * FROM tbl");
204-
redshift().one_statement_parses_to("SELECT TOP 3 ALL * FROM tbl", "SELECT TOP 3 * FROM tbl");
205-
redshift().verified_stmt("SELECT TOP 3 DISTINCT * FROM tbl");
206-
redshift().verified_stmt("SELECT TOP 3 DISTINCT a, b, c FROM tbl");
207-
}

0 commit comments

Comments
 (0)