Skip to content

feat: expose PyWindowFrame#509

Merged
andygrove merged 4 commits into
apache:mainfrom
xorq-labs:pywindowframe
Oct 17, 2023
Merged

feat: expose PyWindowFrame#509
andygrove merged 4 commits into
apache:mainfrom
xorq-labs:pywindowframe

Conversation

@dlovell
Copy link
Copy Markdown
Contributor

@dlovell dlovell commented Oct 5, 2023

Which issue does this PR close?

Partial progress on #191 : Expressions: Window function

What changes are included in this PR?

This PR

  • adds PyWindowFrame
  • modifies functions.window to accept an optional PyWindowFrame and optional PySessionContext
    • PySessionContext is necessary to look up aggregates registered via PySessionContext.register_udaf
  • modifies function.window to fall back to SessionContext.udaf when find_df_window_func fails
  • modifies Accumulator impl for RustAccumulator to expose retract_batch and supports_retract_batch

Are there any user-facing changes?

PyWindowFrame needs to be documented as do changes to functions.window

note: I have working code that enables window functions for ibis that I still need to create a PR for

@andygrove
Copy link
Copy Markdown
Member

Build is failing due to new clippy rule in latest Rust. I have a fix in #511

@andygrove
Copy link
Copy Markdown
Member

@dlovell Could you upmerge to pick up the clippy fix?

@dlovell
Copy link
Copy Markdown
Contributor Author

dlovell commented Oct 10, 2023

@andygrove i hadn't heard the term upmerge before, but i presume that means merge main into pywindowframe. let me know if i should do something else.

@andygrove
Copy link
Copy Markdown
Member

@andygrove i hadn't heard the term upmerge before, but i presume that means merge main into pywindowframe. let me know if i should do something else.

Yes, exactly.

Comment thread src/window_frame.rs Outdated
"groups" => Some(WindowFrameUnits::Groups),
_ => None,
};
let units = units.unwrap();
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we return an Err instead of causing a panic! here?

Comment thread src/window_frame.rs Outdated
None => match units {
WindowFrameUnits::Range => WindowFrameBound::Preceding(ScalarValue::UInt64(None)),
WindowFrameUnits::Rows => WindowFrameBound::Preceding(ScalarValue::UInt64(None)),
WindowFrameUnits::Groups => todo!(),
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comment for these todo! calls .. better to return an Err

Copy link
Copy Markdown
Member

@andygrove andygrove left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks @dlovell

@andygrove andygrove merged commit 399fa75 into apache:main Oct 17, 2023
@dlovell dlovell deleted the pywindowframe branch December 14, 2023 09:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants