You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "kosiew (via GitHub)" <gi...@apache.org> on 2025/11/17 02:00:22 UTC
[PR] Add `BoundsTypeExt` to Support GROUPS Window Frame Unit in Substrait Round-Trip [datafusion]
kosiew opened a new pull request, #18753:
URL: https://github.com/apache/datafusion/pull/18753
## Which issue does this PR close?
* Closes #16297.
## Rationale for this change
This change implements support for the `GROUPS` window frame unit in Substrait round-trip conversions. Several SQLLogicTest cases were failing due to DataFusion not supporting `GROUPS` when interpreting Substrait window frame bounds. Adding this support improves compatibility and eliminates these failures, contributing to broader enhancement efforts described in issue #16248.
### Before
```
❯ cargo test --test sqllogictests -- --substrait-round-trip window.slt:1030
Finished `test` profile [unoptimized + debuginfo] target(s) in 0.52s
Running bin/sqllogictests.rs (target/debug/deps/sqllogictests-abc7c402056c6d3d)
Completed 2 test files in 1 second External error: 1 errors in file /Users/kosiew/GitHub/datafusion/datafusion/sqllogictest/test_files/window.slt
1. query failed: DataFusion error: This feature is not implemented: Unsupported window frame unit: Groups
```
### After
```
❯ cargo test --test sqllogictests -- --substrait-round-trip window.slt:1030
Finished `test` profile [unoptimized + debuginfo] target(s) in 0.79s
Running bin/sqllogictests.rs (target/debug/deps/sqllogictests-abc7c402056c6d3d)
Completed 2 test files in 1 second
```
## What changes are included in this PR?
* Introduces a new `BoundsTypeExt` enum to bridge Substrait and DataFusion window frame units, including the previously unsupported `GROUPS` variant.
* Updates Substrait consumer logic to interpret `bounds_type` using `BoundsTypeExt` and apply appropriate defaulting logic.
* Updates producer logic to emit the correct Substrait representation for `GROUPS`.
* Refactors existing code paths to centralize conversion logic, improving maintainability and consistency.
## Are these changes tested?
Yes. These changes are validated by existing SQLLogicTest coverage, including the failing Substrait round-trip tests such as `window.slt:1030`. These tests now pass, confirming correct behavior.
## Are there any user-facing changes?
No user-facing API changes. Substrait-related behavior is improved internally, enabling correct handling of `GROUPS` window frame units without altering public interfaces.
## LLM-generated code disclosure
This pull request includes code, comments that were generated with assistance from an LLM. All LLM-generated content has been manually reviewed and tested.
--
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 `BoundsTypeExt` to Support GROUPS Window Frame Unit in Substrait Round-Trip [datafusion]
Posted by "gabotechs (via GitHub)" <gi...@apache.org>.
gabotechs commented on code in PR #18753:
URL: https://github.com/apache/datafusion/pull/18753#discussion_r2554930787
##########
datafusion/substrait/src/logical_plan/producer/expr/window_function.rs:
##########
@@ -100,20 +101,20 @@ fn make_substrait_window_function(
lower_bound: Some(bounds.0),
upper_bound: Some(bounds.1),
args: vec![],
- bounds_type: bounds_type as i32,
+ bounds_type: bounds_type.to_i32(),
Review Comment:
I imagine that if this is of type `BoundsTypeExt::Groups`, it will produce invalid Substrait, as this is not yet supported in there ([link](https://github.com/substrait-io/substrait/blob/84816cd57e23b99b9e1816e1fffc740f121dbb22/proto/substrait/algebra.proto#L1301)). I wonder if it's better to just fail if we find a `WindowFrameUnits::Groups` until Substrait support groups bounds.
Probably worth opening an issue in https://github.com/substrait-io/substrait, as I imagine the big bulk of the work will need to happen there.
What do you 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
Re: [PR] Add `BoundsTypeExt` to Support GROUPS Window Frame Unit in Substrait Round-Trip [datafusion]
Posted by "gabotechs (via GitHub)" <gi...@apache.org>.
gabotechs commented on PR #18753:
URL: https://github.com/apache/datafusion/pull/18753#issuecomment-3558255624
Sure! will give it a look shortly, thanks for working on this!
--
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 `BoundsTypeExt` to Support GROUPS Window Frame Unit in Substrait Round-Trip [datafusion]
Posted by "gabotechs (via GitHub)" <gi...@apache.org>.
gabotechs commented on code in PR #18753:
URL: https://github.com/apache/datafusion/pull/18753#discussion_r2554930787
##########
datafusion/substrait/src/logical_plan/producer/expr/window_function.rs:
##########
@@ -100,20 +101,20 @@ fn make_substrait_window_function(
lower_bound: Some(bounds.0),
upper_bound: Some(bounds.1),
args: vec![],
- bounds_type: bounds_type as i32,
+ bounds_type: bounds_type.to_i32(),
Review Comment:
I imagine that if this is of type `BoundsTypeExt::Groups`, it will produce invalid Substrait, as this is not yet supported in Substrait ([link](https://github.com/substrait-io/substrait/blob/84816cd57e23b99b9e1816e1fffc740f121dbb22/proto/substrait/algebra.proto#L1301)). I wonder if it's better to just fail if we find a `WindowFrameUnits::Groups` until Substrait support groups bounds. What do you think? probably even worth opening an issue in https://github.com/substrait-io/substrait
--
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 `BoundsTypeExt` to Support GROUPS Window Frame Unit in Substrait Round-Trip [datafusion]
Posted by "kosiew (via GitHub)" <gi...@apache.org>.
kosiew commented on PR #18753:
URL: https://github.com/apache/datafusion/pull/18753#issuecomment-3578256042
This can only continue after
https://github.com/substrait-io/substrait/issues/902 is resolved.
Closing.
--
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 `BoundsTypeExt` to Support GROUPS Window Frame Unit in Substrait Round-Trip [datafusion]
Posted by "martin-g (via GitHub)" <gi...@apache.org>.
martin-g commented on code in PR #18753:
URL: https://github.com/apache/datafusion/pull/18753#discussion_r2534283736
##########
datafusion/substrait/src/logical_plan/bounds_type.rs:
##########
@@ -0,0 +1,76 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+use datafusion::{
+ common::{plan_datafusion_err, Result},
+ logical_expr::WindowFrameUnits,
+};
+use substrait::proto::expression::window_function::BoundsType;
+
+/// Wrapper for the Substrait `BoundsType` to add the `GROUPS` (value 3)
+/// variant which `substrait::proto::expression::window_function::BoundsType`
+/// currently does not support. This type centralizes conversions to/from the
+/// Substrait `i32` representation and to DataFusion's `WindowFrameUnits`.
+#[derive(Debug, Clone, Copy, PartialEq, Eq)]
+pub(crate) enum BoundsTypeExt {
+ Rows,
+ Range,
+ Groups,
+ Unspecified,
+}
+
+impl BoundsTypeExt {
+ /// Convert from an i32 value (from Substrait protobuf) to BoundsTypeExt
+ pub(crate) fn from_i32(value: i32) -> Result<Self> {
+ match value {
+ v if v == BoundsType::Rows as i32 => Ok(BoundsTypeExt::Rows),
+ v if v == BoundsType::Range as i32 => Ok(BoundsTypeExt::Range),
+ 3 => Ok(BoundsTypeExt::Groups), // Groups variant from Substrait spec
Review Comment:
Consider extracting a constant (e.g. `SUBSTRAIT_GROUPS_VALUE`) instead of using a `magic number`
##########
datafusion/substrait/src/logical_plan/bounds_type.rs:
##########
@@ -0,0 +1,76 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+use datafusion::{
+ common::{plan_datafusion_err, Result},
+ logical_expr::WindowFrameUnits,
+};
+use substrait::proto::expression::window_function::BoundsType;
+
+/// Wrapper for the Substrait `BoundsType` to add the `GROUPS` (value 3)
+/// variant which `substrait::proto::expression::window_function::BoundsType`
+/// currently does not support. This type centralizes conversions to/from the
+/// Substrait `i32` representation and to DataFusion's `WindowFrameUnits`.
+#[derive(Debug, Clone, Copy, PartialEq, Eq)]
+pub(crate) enum BoundsTypeExt {
+ Rows,
+ Range,
+ Groups,
+ Unspecified,
+}
+
+impl BoundsTypeExt {
Review Comment:
Please add some unit tests
--
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 `BoundsTypeExt` to Support GROUPS Window Frame Unit in Substrait Round-Trip [datafusion]
Posted by "kosiew (via GitHub)" <gi...@apache.org>.
kosiew commented on code in PR #18753:
URL: https://github.com/apache/datafusion/pull/18753#discussion_r2532443323
##########
datafusion/substrait/src/logical_plan/consumer/expr/window_function.rs:
##########
@@ -66,22 +63,8 @@ pub async fn from_window_function(
let mut order_by =
from_substrait_sorts(consumer, &window.sorts, input_schema).await?;
- let bound_units = match BoundsType::try_from(window.bounds_type).map_err(|e| {
- plan_datafusion_err!("Invalid bound type {}: {e}", window.bounds_type)
- })? {
- BoundsType::Rows => WindowFrameUnits::Rows,
- BoundsType::Range => WindowFrameUnits::Range,
- BoundsType::Unspecified => {
- // If the plan does not specify the bounds type, then we use a simple logic to determine the units
- // If there is no `ORDER BY`, then by default, the frame counts each row from the lower up to upper boundary
- // If there is `ORDER BY`, then by default, each frame is a range starting from unbounded preceding to current row
- if order_by.is_empty() {
- WindowFrameUnits::Rows
- } else {
- WindowFrameUnits::Range
- }
Review Comment:
This is moved to
datafusion/substrait/src/logical_plan/bounds_type.rs
--
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 `BoundsTypeExt` to Support GROUPS Window Frame Unit in Substrait Round-Trip [datafusion]
Posted by "kosiew (via GitHub)" <gi...@apache.org>.
kosiew commented on code in PR #18753:
URL: https://github.com/apache/datafusion/pull/18753#discussion_r2536046437
##########
datafusion/substrait/src/logical_plan/bounds_type.rs:
##########
@@ -0,0 +1,76 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+use datafusion::{
+ common::{plan_datafusion_err, Result},
+ logical_expr::WindowFrameUnits,
+};
+use substrait::proto::expression::window_function::BoundsType;
+
+/// Wrapper for the Substrait `BoundsType` to add the `GROUPS` (value 3)
+/// variant which `substrait::proto::expression::window_function::BoundsType`
+/// currently does not support. This type centralizes conversions to/from the
+/// Substrait `i32` representation and to DataFusion's `WindowFrameUnits`.
+#[derive(Debug, Clone, Copy, PartialEq, Eq)]
+pub(crate) enum BoundsTypeExt {
+ Rows,
+ Range,
+ Groups,
+ Unspecified,
+}
+
+impl BoundsTypeExt {
Review Comment:
Added tests
--
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 `BoundsTypeExt` to Support GROUPS Window Frame Unit in Substrait Round-Trip [datafusion]
Posted by "kosiew (via GitHub)" <gi...@apache.org>.
kosiew commented on PR #18753:
URL: https://github.com/apache/datafusion/pull/18753#issuecomment-3557458428
@gabotechs
Can you review this?
--
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 `BoundsTypeExt` to Support GROUPS Window Frame Unit in Substrait Round-Trip [datafusion]
Posted by "kosiew (via GitHub)" <gi...@apache.org>.
kosiew commented on PR #18753:
URL: https://github.com/apache/datafusion/pull/18753#issuecomment-3547604323
@martin-g
Thank you for your review and feedback
--
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 `BoundsTypeExt` to Support GROUPS Window Frame Unit in Substrait Round-Trip [datafusion]
Posted by "gabotechs (via GitHub)" <gi...@apache.org>.
gabotechs commented on PR #18753:
URL: https://github.com/apache/datafusion/pull/18753#issuecomment-3579589640
> This can only continue after
https://github.com/substrait-io/substrait/issues/902 is resolved.
👍 makes sense. Thanks!
--
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 `BoundsTypeExt` to Support GROUPS Window Frame Unit in Substrait Round-Trip [datafusion]
Posted by "kosiew (via GitHub)" <gi...@apache.org>.
kosiew commented on code in PR #18753:
URL: https://github.com/apache/datafusion/pull/18753#discussion_r2562205409
##########
datafusion/substrait/src/logical_plan/producer/expr/window_function.rs:
##########
@@ -100,20 +101,20 @@ fn make_substrait_window_function(
lower_bound: Some(bounds.0),
upper_bound: Some(bounds.1),
args: vec![],
- bounds_type: bounds_type as i32,
+ bounds_type: bounds_type.to_i32(),
Review Comment:
I agree.
--
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 `BoundsTypeExt` to Support GROUPS Window Frame Unit in Substrait Round-Trip [datafusion]
Posted by "kosiew (via GitHub)" <gi...@apache.org>.
kosiew closed pull request #18753: Add `BoundsTypeExt` to Support GROUPS Window Frame Unit in Substrait Round-Trip
URL: https://github.com/apache/datafusion/pull/18753
--
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