iffyio commented on code in PR #1495: URL: https://github.com/apache/datafusion-sqlparser-rs/pull/1495#discussion_r1826904031
########## src/dialect/mod.rs: ########## @@ -590,6 +590,12 @@ pub trait Dialect: Debug + Any { fn supports_try_convert(&self) -> bool { false } + + /// Returns true if this dialect expects the the `TOP` option + /// before the `ALL`/`DISTINCT` options + fn expects_top_before_distinct(&self) -> bool { Review Comment: ```suggestion fn supports_top_before_distinct(&self) -> bool { ``` ########## src/keywords.rs: ########## @@ -254,6 +254,7 @@ define_keywords!( DISCARD, DISCONNECT, DISTINCT, + DISTINCTROW, Review Comment: Ah can we also mention the distinctrow support in the PR title/description? ########## tests/sqlparser_redshift.rs: ########## @@ -196,3 +196,12 @@ fn test_create_view_with_no_schema_binding() { redshift_and_generic() .verified_stmt("CREATE VIEW myevent AS SELECT eventname FROM event WITH NO SCHEMA BINDING"); } + +#[test] +fn test_select_top() { Review Comment: can we move the test to common and run the scenarios via `all_dialects_where(|| expects_top_before-distinct()`? (same for distinct row) ########## src/parser/mod.rs: ########## @@ -3534,7 +3534,9 @@ impl<'a> Parser<'a> { pub fn parse_all_or_distinct(&mut self) -> Result<Option<Distinct>, ParserError> { let loc = self.peek_token().location; let all = self.parse_keyword(Keyword::ALL); - let distinct = self.parse_keyword(Keyword::DISTINCT); + let distinct = self + .parse_one_of_keywords(&[Keyword::DISTINCT, Keyword::DISTINCTROW]) + .is_some(); Review Comment: should distinctrow be a different flag? Also maybe we can gate the behavior with a `dialect.supports_distinct_row()`? ########## src/dialect/mod.rs: ########## @@ -590,6 +590,12 @@ pub trait Dialect: Debug + Any { fn supports_try_convert(&self) -> bool { false } + + /// Returns true if this dialect expects the the `TOP` option + /// before the `ALL`/`DISTINCT` options Review Comment: ```suggestion /// before the `ALL`/`DISTINCT` options in a `SELECT` statement. ``` ########## src/parser/mod.rs: ########## @@ -11491,7 +11495,14 @@ impl<'a> Parser<'a> { /// Parse a TOP clause, MSSQL equivalent of LIMIT, /// that follows after `SELECT [DISTINCT]`. - pub fn parse_top(&mut self) -> Result<Top, ParserError> { + pub fn maybe_parse_top( + &mut self, + is_before_distinct: bool, + ) -> Result<Option<Top>, ParserError> { Review Comment: i'm thinking this this function probably wont need to change, for the 'maybe' part, the caller can check the next token before calling ```rust if self.expects_top_before_distinct() && self.peek_token() == TOP ``` Then for the `is_before_distinct`, that flag I think would make sense as a `top_before_distinct` on the `SELECT` struct instead? Its a bit unusual that the top struct contains metadata about its positioning relative to another struct given that both structs are not necessarily always tied to each other. The enclosing SELECT on the other hand can contain this info since it gets to format both top and distinct in this case -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: github-unsubscr...@datafusion.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org --------------------------------------------------------------------- To unsubscribe, e-mail: github-unsubscr...@datafusion.apache.org For additional commands, e-mail: github-h...@datafusion.apache.org