[GitHub] [arrow-datafusion] Ted-Jiang commented on a diff in pull request #4516: Remove Option from window frame

[GitBox <gi...@apache.org>]

Ted-Jiang commented on code in PR #4516:
URL: https://github.com/apache/arrow-datafusion/pull/4516#discussion_r1040387998


##########
datafusion/expr/src/window_frame.rs:
##########
@@ -66,33 +66,51 @@ impl TryFrom<ast::WindowFrame> for WindowFrame {
             None => WindowFrameBound::CurrentRow,
         };
 
-        if let WindowFrameBound::Following(ScalarValue::Utf8(None)) = start_bound {
-            Err(DataFusionError::Execution(
-                "Invalid window frame: start bound cannot be unbounded following"
-                    .to_owned(),
-            ))
-        } else if let WindowFrameBound::Preceding(ScalarValue::Utf8(None)) = end_bound {
-            Err(DataFusionError::Execution(
-                "Invalid window frame: end bound cannot be unbounded preceding"
-                    .to_owned(),
-            ))
-        } else {
-            let units = value.units.into();
-            Ok(Self {
-                units,
-                start_bound,
-                end_bound,
-            })
-        }
+        if let WindowFrameBound::Following(val) = &start_bound {
+            if val.is_null() {
+                return Err(DataFusionError::Execution(
+                    "Invalid window frame: start bound cannot be unbounded following"
+                        .to_owned(),
+                ));
+            }
+        } else if let WindowFrameBound::Preceding(val) = &end_bound {
+            if val.is_null() {
+                return Err(DataFusionError::Execution(
+                    "Invalid window frame: end bound cannot be unbounded preceding"
+                        .to_owned(),
+                ));
+            }
+        };
+        Ok(Self {
+            units: value.units.into(),
+            start_bound,
+            end_bound,
+        })
     }
 }
 
-impl Default for WindowFrame {
-    fn default() -> Self {
-        WindowFrame {
-            units: WindowFrameUnits::Range,
-            start_bound: WindowFrameBound::Preceding(ScalarValue::Utf8(None)),
-            end_bound: WindowFrameBound::CurrentRow,
+impl WindowFrame {
+    /// Creates a new, default window frame (with the meaning of default depending on whether the
+    /// frame contains an `ORDER BY` clause.
+    pub fn new(has_order_by: bool) -> Self {

Review Comment:
   Thanks for this fix, i also want to change this default behavior.



##########
datafusion/proto/proto/datafusion.proto:
##########
@@ -573,9 +573,7 @@ message WindowExprNode {
   repeated LogicalExprNode partition_by = 5;
   repeated LogicalExprNode order_by = 6;
   // repeated LogicalExprNode filter = 7;
-  oneof window_frame {
-    WindowFrame frame = 8;
-  }
+  WindowFrame window_frame = 8;

Review Comment:
   👍



-- 
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@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org