You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "yoavcloud (via GitHub)" <gi...@apache.org> on 2024/10/31 22:47:11 UTC
[PR] TOP before ALL/DISTINCT [datafusion-sqlparser-rs]
yoavcloud opened a new pull request, #1495:
URL: https://github.com/apache/datafusion-sqlparser-rs/pull/1495
Adds support for dialects (Redshift) that expect the TOP keyword in a SELECT statement to appear before the ALL/DISTINCT option. For example: SELECT TOP 2 DISTINCT FROM tbl;
https://docs.aws.amazon.com/redshift/latest/dg/r_SELECT_list.html
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add support for TOP before ALL/DISTINCT [datafusion-sqlparser-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb merged PR #1495:
URL: https://github.com/apache/datafusion-sqlparser-rs/pull/1495
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] TOP before ALL/DISTINCT [datafusion-sqlparser-rs]
Posted by "yoavcloud (via GitHub)" <gi...@apache.org>.
yoavcloud commented on code in PR #1495:
URL: https://github.com/apache/datafusion-sqlparser-rs/pull/1495#discussion_r1830495918
##########
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:
I decided it's best to remove this from the PR, as it's unrelated and I have not done enough research on it. In MySQL, DISTINCTROW and DISTINCT are synonymous but that's not true across all dialects. I'll add it in a future PR.
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] TOP before ALL/DISTINCT [datafusion-sqlparser-rs]
Posted by "iffyio (via GitHub)" <gi...@apache.org>.
iffyio commented on code in PR #1495:
URL: https://github.com/apache/datafusion-sqlparser-rs/pull/1495#discussion_r1830396360
##########
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:
@yoavcloud this one is unadressed? (thinking the current behavior of repurposing distinct for distinctrow isn't ideal since they're not necessarily the same flag)
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add support for TOP before ALL/DISTINCT [datafusion-sqlparser-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #1495:
URL: https://github.com/apache/datafusion-sqlparser-rs/pull/1495#issuecomment-2460337347
Thanks @yoavcloud and @iffyio
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] TOP before ALL/DISTINCT [datafusion-sqlparser-rs]
Posted by "iffyio (via GitHub)" <gi...@apache.org>.
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] TOP before ALL/DISTINCT [datafusion-sqlparser-rs]
Posted by "iffyio (via GitHub)" <gi...@apache.org>.
iffyio commented on code in PR #1495:
URL: https://github.com/apache/datafusion-sqlparser-rs/pull/1495#discussion_r1830855141
##########
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:
Alright that sounds good! We can remove the code for it (we have the keyword and the associated parsing on this line)?
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add support for TOP before ALL/DISTINCT [datafusion-sqlparser-rs]
Posted by "coveralls (via GitHub)" <gi...@apache.org>.
coveralls commented on PR #1495:
URL: https://github.com/apache/datafusion-sqlparser-rs/pull/1495#issuecomment-2460255856
## Pull Request Test Coverage Report for [Build 11707166854](https://coveralls.io/builds/70736418)
### Details
* **23** of **25** **(92.0%)** changed or added relevant lines in **5** files are covered.
* No unchanged relevant lines lost coverage.
* Overall coverage remained the same at **89.391%**
---
| Changes Missing Coverage | Covered Lines | Changed/Added Lines | % |
| :-----|--------------|--------|---: |
| [src/dialect/mod.rs](https://coveralls.io/builds/70736418/source?filename=src%2Fdialect%2Fmod.rs#L607) | 1 | 2 | 50.0%
| [src/dialect/redshift.rs](https://coveralls.io/builds/70736418/source?filename=src%2Fdialect%2Fredshift.rs#L75) | 1 | 2 | 50.0%
<!-- | **Total:** | **23** | **25** | **92.0%** | -->
| Totals | [](https://coveralls.io/builds/70736418) |
| :-- | --: |
| Change from base [Build 11707125112](https://coveralls.io/builds/70735561): | 0.0% |
| Covered Lines: | 30587 |
| Relevant Lines: | 34217 |
---
##### 💛 - [Coveralls](https://coveralls.io)
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] TOP before ALL/DISTINCT [datafusion-sqlparser-rs]
Posted by "yoavcloud (via GitHub)" <gi...@apache.org>.
yoavcloud commented on code in PR #1495:
URL: https://github.com/apache/datafusion-sqlparser-rs/pull/1495#discussion_r1826917925
##########
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:
Putting the positioning hint on the `SELECT` makes sense I was initially intimidated by introducing a change to such a central (the most central?) struct :-) See next commit
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] TOP before ALL/DISTINCT [datafusion-sqlparser-rs]
Posted by "yoavcloud (via GitHub)" <gi...@apache.org>.
yoavcloud commented on code in PR #1495:
URL: https://github.com/apache/datafusion-sqlparser-rs/pull/1495#discussion_r1830865901
##########
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:
I forgot to push my last commit, it's there now
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org