[GitHub] [arrow-datafusion] alamb commented on a diff in pull request #5286: Linear search support for Window Group queries

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

alamb commented on code in PR #5286:
URL: https://github.com/apache/arrow-datafusion/pull/5286#discussion_r1108917882


##########
datafusion/proto/src/logical_plan/from_proto.rs:
##########
@@ -907,16 +908,13 @@ pub fn parse_expr(
                 .window_frame
                 .as_ref()
                 .map::<Result<WindowFrame, _>, _>(|window_frame| {
-                    let window_frame: WindowFrame = window_frame.clone().try_into()?;
-                    if WindowFrameUnits::Range == window_frame.units
-                        && order_by.len() != 1
-                    {
-                        Err(proto_error("With window frame of type RANGE, the order by expression must be of length 1"))
-                    } else {
-                        Ok(window_frame)
-                    }
+                    let window_frame = window_frame.clone().try_into()?;
+                    regularize(window_frame, order_by.len())

Review Comment:
   I don't understand why `regularize` is called in the serialization code. It makes sense to be called as part of the SQL planner (or LogicalPlanBuilder) so that by the time the structures were serialized / deserialized they should already be valid (and if they are not it should be an error).



##########
datafusion/proto/src/logical_plan/from_proto.rs:
##########
@@ -907,16 +908,13 @@ pub fn parse_expr(
                 .window_frame
                 .as_ref()
                 .map::<Result<WindowFrame, _>, _>(|window_frame| {
-                    let window_frame: WindowFrame = window_frame.clone().try_into()?;
-                    if WindowFrameUnits::Range == window_frame.units
-                        && order_by.len() != 1
-                    {
-                        Err(proto_error("With window frame of type RANGE, the order by expression must be of length 1"))
-                    } else {
-                        Ok(window_frame)
-                    }
+                    let window_frame = window_frame.clone().try_into()?;
+                    regularize(window_frame, order_by.len())
                 })
-                .transpose()?.ok_or_else(||{DataFusionError::Execution("expects somothing".to_string())})?;
+                .transpose()?
+                .ok_or_else(|| {
+                    DataFusionError::Execution("expects something".to_string())

Review Comment:
   ```suggestion
                       DataFusionError::Execution("missing window frame during deserialization".to_string())
   ```



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