iffyio commented on code in PR #1724:
URL:
https://github.com/apache/datafusion-sqlparser-rs/pull/1724#discussion_r1957698799
##########
src/parser/mod.rs:
##########
@@ -13405,6 +13419,19 @@ impl<'a> Parser<'a> {
})
}
+ pub fn parse_order_by_all(&mut self) -> Result<OrderByAll, ParserError> {
Review Comment:
```suggestion
fn parse_order_by_all(&mut self) -> Result<OrderByAll, ParserError> {
```
##########
src/parser/mod.rs:
##########
@@ -13405,6 +13419,19 @@ impl<'a> Parser<'a> {
})
}
+ pub fn parse_order_by_all(&mut self) -> Result<OrderByAll, ParserError> {
Review Comment:
can we update `parse_order_by_expr` to use this helper? since it shares this
code so that we avoid keeping both copies in sync
##########
src/dialect/clickhouse.rs:
##########
@@ -80,6 +80,11 @@ impl Dialect for ClickHouseDialect {
true
}
+ // See
<https://clickhouse.com/docs/en/sql-reference/statements/select/order-by>
Review Comment:
```suggestion
/// See
<https://clickhouse.com/docs/en/sql-reference/statements/select/order-by>
```
##########
src/ast/query.rs:
##########
@@ -2233,15 +2249,24 @@ pub struct OrderBy {
impl fmt::Display for OrderBy {
fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
write!(f, "ORDER BY")?;
- if !self.exprs.is_empty() {
- write!(f, " {}", display_comma_separated(&self.exprs))?;
- }
- if let Some(ref interpolate) = self.interpolate {
- match &interpolate.exprs {
- Some(exprs) => write!(f, " INTERPOLATE ({})",
display_comma_separated(exprs))?,
- None => write!(f, " INTERPOLATE")?,
+ match &self.kind {
+ OrderByKind::Expressions(exprs) => {
+ write!(f, " {}", display_comma_separated(exprs))?;
+
+ if let Some(ref interpolate) = self.interpolate {
+ match &interpolate.exprs {
+ Some(exprs) => {
+ write!(f, " INTERPOLATE ({})",
display_comma_separated(exprs))?
+ }
+ None => write!(f, " INTERPOLATE")?,
+ }
+ }
Review Comment:
we can move this outside the match statement and write it unconditionally,
representation wise its not tied to kind so it would be confusing to display
interpolate conditionally on the kind.
##########
src/parser/mod.rs:
##########
@@ -9191,17 +9191,31 @@ impl<'a> Parser<'a> {
pub fn parse_optional_order_by(&mut self) -> Result<Option<OrderBy>,
ParserError> {
if self.parse_keywords(&[Keyword::ORDER, Keyword::BY]) {
- let order_by_exprs =
self.parse_comma_separated(Parser::parse_order_by_expr)?;
- let interpolate = if dialect_of!(self is ClickHouseDialect |
GenericDialect) {
- self.parse_interpolations()?
+ let order_by = if self.parse_keyword(Keyword::ALL) {
+ if !self.dialect.supports_order_by_all() {
+ return parser_err!(
+ "ALL is not supported in ORDER BY",
+ self.peek_token().span.start
+ );
+ }
Review Comment:
```suggestion
let order_by = if self.dialect.supports_order_by_all() &&
self.parse_keyword(Keyword::ALL) {
```
can we maybe add a test case for this? that other dialects can successfully
parse `ORDER BY all`
##########
src/ast/query.rs:
##########
@@ -2339,6 +2364,33 @@ impl fmt::Display for InterpolateExpr {
}
}
+/// 'ORDER BY ALL' clause
+#[derive(Debug, Clone, PartialEq, PartialOrd, Eq, Ord, Hash)]
+#[cfg_attr(feature = "serde", derive(Serialize, Deserialize))]
+#[cfg_attr(feature = "visitor", derive(Visit, VisitMut))]
+pub struct OrderByAll {
+ /// Optional `ASC` or `DESC`
+ pub asc: Option<bool>,
+ /// Optional `NULLS FIRST` or `NULLS LAST`
+ pub nulls_first: Option<bool>,
+}
+
+impl fmt::Display for OrderByAll {
+ fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
+ match self.asc {
+ Some(true) => write!(f, " ASC")?,
+ Some(false) => write!(f, " DESC")?,
+ None => (),
+ }
+ match self.nulls_first {
+ Some(true) => write!(f, " NULLS FIRST")?,
+ Some(false) => write!(f, " NULLS LAST")?,
+ None => (),
+ }
+ Ok(())
+ }
Review Comment:
I think it would be ideal to reuse the same code as orderbyexpr, maybe we
can rename it to something generic like `OrderByOptions` and then replace both
`asc` and `nulls_first` on `OrderByExpr` with a field containing this struct?
##########
src/dialect/duckdb.rs:
##########
@@ -89,4 +89,9 @@ impl Dialect for DuckDbDialect {
fn supports_from_first_select(&self) -> bool {
true
}
+
+ // See DuckDB
<https://duckdb.org/docs/sql/query_syntax/orderby.html#order-by-all-examples>
Review Comment:
```suggestion
/// See DuckDB
<https://duckdb.org/docs/sql/query_syntax/orderby.html#order-by-all-examples>
```
##########
src/ast/query.rs:
##########
@@ -2218,13 +2218,29 @@ pub enum JoinConstraint {
None,
}
+#[derive(Debug, Clone, PartialEq, PartialOrd, Eq, Ord, Hash)]
+#[cfg_attr(feature = "serde", derive(Serialize, Deserialize))]
+#[cfg_attr(feature = "visitor", derive(Visit, VisitMut))]
+pub enum OrderByKind {
+ /// ALL syntax of [DuckDB] and [ClickHouse].
+ ///
+ /// [DuckDB]: <https://duckdb.org/docs/sql/query_syntax/orderby>
+ /// [ClickHouse]:
<https://clickhouse.com/docs/en/sql-reference/statements/select/order-by>
+ All(OrderByAll),
+
+ /// Expressions
+ Expressions(Vec<OrderByExpr>),
+}
+
#[derive(Debug, Clone, PartialEq, PartialOrd, Eq, Ord, Hash)]
#[cfg_attr(feature = "serde", derive(Serialize, Deserialize))]
#[cfg_attr(feature = "visitor", derive(Visit, VisitMut))]
pub struct OrderBy {
- pub exprs: Vec<OrderByExpr>,
+ pub kind: OrderByKind,
+
/// Optional: `INTERPOLATE`
/// Supported by [ClickHouse syntax]
+ /// Note that when combined with `OrderByKind::All`, it should be None, as
ClickHouse doesn't support using them together.
Review Comment:
```suggestion
```
I think we can skip this comment describing clickhouse semantics, at best it
describes parsing logic which isn't applicable here
##########
src/dialect/mod.rs:
##########
@@ -918,6 +918,16 @@ pub trait Dialect: Debug + Any {
fn supports_array_typedef_size(&self) -> bool {
false
}
+
+ /// Returns true if the dialect supports `ORDER BY ALL`.
+ /// `ALL` which means all columns of the SELECT clause.
+ ///
+ /// DuckDB:
<https://duckdb.org/docs/sql/query_syntax/orderby.html#order-by-all>
+ /// ClickHouse:
<https://clickhouse.com/docs/en/sql-reference/statements/select/order-by>
Review Comment:
```suggestion
```
might not need to list the dialects since they're already listed in the
impls. That way this list doesnt go out of sync if future dialects are added
--
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
