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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
