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

Reply via email to