Re: [PR] Hive: Support DISTRIBUTE BY and SORT BY in window specs [datafusion-sqlparser-rs]

["iffyio (via GitHub)" <gi...@apache.org>]

iffyio commented on code in PR #2340:
URL: https://github.com/apache/datafusion-sqlparser-rs/pull/2340#discussion_r3433088582


##########
src/ast/mod.rs:
##########
@@ -2267,12 +2267,58 @@ pub struct WindowSpec {
     pub window_name: Option<Ident>,
     /// `OVER (PARTITION BY ...)`
     pub partition_by: Vec<Expr>,
+    /// The kind of partitioning clause used in the window specification.
+    pub partition_by_kind: WindowPartitionByKind,

Review Comment:
   for partition and order by kind, can we instead wrap the field with the enum instead of introducing a new field? i.e.
   ```rust
   partition_by: WindowPartitionByKind
   
   // where 
   enum WindowPartitionByKind {
        PartitionBy(Vec<Expr>)
        DistributeBy(Vec<Expr>)
   }
   ```



##########
src/dialect/mod.rs:
##########
@@ -341,6 +341,17 @@ pub trait Dialect: Debug + Any {
         false
     }
 
+    /// Returns true if the dialect supports Hive-style `DISTRIBUTE BY` and `SORT BY`
+    /// clauses within a window specification.
+    ///
+    /// Example
+    /// ```sql
+    /// SELECT row_number() OVER (DISTRIBUTE BY a SORT BY b)
+    /// ```
+    fn supports_window_spec_distribute_sort(&self) -> bool {

Review Comment:
   if this doesnt conflict with any existing syntax I think we can skip the dialect method and let the parser always accept the clauses when they show up



##########
src/ast/mod.rs:
##########
@@ -2267,12 +2267,58 @@ pub struct WindowSpec {
     pub window_name: Option<Ident>,
     /// `OVER (PARTITION BY ...)`
     pub partition_by: Vec<Expr>,
+    /// The kind of partitioning clause used in the window specification.
+    pub partition_by_kind: WindowPartitionByKind,
     /// `OVER (ORDER BY ...)`
     pub order_by: Vec<OrderByExpr>,
+    /// The kind of ordering clause used in the window specification.
+    pub order_by_kind: WindowOrderByKind,
     /// `OVER (window frame)`
     pub window_frame: Option<WindowFrame>,
 }
 
+/// The kind of partitioning clause in a window specification.
+#[derive(Debug, Default, Clone, Copy, PartialEq, PartialOrd, Eq, Ord, Hash)]
+#[cfg_attr(feature = "serde", derive(Serialize, Deserialize))]
+#[cfg_attr(feature = "visitor", derive(Visit, VisitMut))]
+pub enum WindowPartitionByKind {
+    /// `PARTITION BY`
+    #[default]

Review Comment:
   is there aneed for these default attributes? (if not we can skip them I think)



-- 
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