Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat(frontend): support window table function #1396

Merged
merged 14 commits into from
Apr 1, 2022
Merged

feat(frontend): support window table function #1396

merged 14 commits into from
Apr 1, 2022

Conversation

TennyZhuang
Copy link
Contributor

@TennyZhuang TennyZhuang commented Mar 30, 2022 •
edited
Loading

Signed-off-by: TennyZhuang [email protected]

What's changed and what's your intention?

PLEASE DO NOT LEAVE THIS EMPTY !!!

  • Implement window table function
  • Implement tumble

Checklist

Refer to a related PR or issue link (optional)

@TennyZhuang
fix
1496ba8 
Signed-off-by: TennyZhuang <[email protected]>
@TennyZhuang
Merge remote-tracking branch 'origin/main' into feat/window-table-fun…
0097ea1 
…ction
@TennyZhuang
fix clippy
246722c 
Signed-off-by: TennyZhuang <[email protected]>
@TennyZhuang
eq_ignore_case
e2db102 
Signed-off-by: TennyZhuang <[email protected]>
@TennyZhuang
simplify codes using let_else
de0ddfb 
Signed-off-by: TennyZhuang <[email protected]>
@TennyZhuang
support date
05bd8e9 
Signed-off-by: TennyZhuang <[email protected]>
@TennyZhuang
fix clippy
789b8bb 
Signed-off-by: TennyZhuang <[email protected]>
@TennyZhuang TennyZhuang marked this pull request as ready for review March 31, 2022 10:22
@codecov
Copy link

codecov bot commented Mar 31, 2022 •
edited
Loading

Codecov Report

Merging #1396 (8f410d4) into main (82ccb48) will decrease coverage by 0.00%.
The diff coverage is 63.55%.

@@             Coverage Diff              @@
##               main    #1396      +/-   ##
============================================
- Coverage     70.60%   70.60%   -0.01%     
  Complexity     2766     2766              
============================================
  Files          1029     1030       +1     
  Lines         90247    90458     +211     
  Branches       1790     1790              
============================================
+ Hits          63721    63864     +143     
- Misses        25635    25703      +68     
  Partials        891      891
Flag Coverage Δ
java 61.01% <ø> (ø)
rust 72.63% <63.55%> (-0.02%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
rust/common/src/expr/expr_binary_nonnull.rs 76.89% <0.00%> (-6.64%) ⬇️
rust/frontend/src/binder/mod.rs 100.00% <ø> (ø)
rust/common/src/vector_op/tumble.rs 64.00% <28.57%> (-26.91%) ⬇️
rust/frontend/src/binder/relation.rs 84.72% <63.63%> (-1.27%) ⬇️
rust/frontend/src/binder/window_table_function.rs 70.65% <70.65%> (ø)
rust/frontend/src/planner/relation.rs 90.12% <87.69%> (-9.88%) ⬇️
rust/frontend/src/binder/expr/function.rs 91.20% <100.00%> (+0.62%) ⬆️
.../src/executor/managed_state/aggregation/extreme.rs 90.13% <0.00%> (-0.28%) ⬇️
rust/common/src/hash/key.rs 85.51% <0.00%> (ø)
rust/common/src/hash/dispatcher.rs 91.66% <0.00%> (ø)
... and 4 more

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

@fuyufjh
Copy link
Member

fuyufjh commented Mar 31, 2022

Please add some tests

@TennyZhuang TennyZhuang requested a review from xxchan April 1, 2022 02:32
@TennyZhuang
Copy link
Contributor Author

Please add some tests

I'd prefer to add some planner tests, the e2e result is not correct (likely because of some executor bugs).

@TennyZhuang TennyZhuang requested a review from st1page April 1, 2022 02:34
@fuyufjh
Copy link
Member

fuyufjh commented Apr 1, 2022 •
edited
Loading

I'd prefer to add some planner tests, the e2e result is not correct (likely because of some executor bugs).

Okay to me

@TennyZhuang
add planner test
ee5dc78 
Signed-off-by: TennyZhuang <[email protected]>
xiangjinwu
xiangjinwu approved these changes Apr 1, 2022
View reviewed changes
Copy link
Contributor

@xiangjinwu xiangjinwu left a comment

Choose a reason for hiding this comment

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

lgtm

rust/frontend/src/binder/relation.rs
Comment on lines +152 to +155
ErrorCode::NotImplementedError(format!(
"unknown window function kind: {}",
name.0[0].value
))
Copy link
Contributor

Choose a reason for hiding this comment

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

When there is no match, it might be a non-window table function. But we can handle it later.

rust/frontend/src/planner/relation.rs
Comment on lines +104 to +106
// TODO: check if the names `window_[start|end]` is valid.
expr_aliases.push(Some("window_start".to_string()));
expr_aliases.push(Some("window_end".to_string()));
Copy link
Contributor

Choose a reason for hiding this comment

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

FYI there will not be aliases on LogicalProject #1433

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@st1page can fix here later (

Copy link
Contributor

@xiangjinwu xiangjinwu Apr 1, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Agree. No action on this PR. Just in case you need it somewhere later, you are aware of this upcoming change.

@TennyZhuang TennyZhuang enabled auto-merge (squash) April 1, 2022 03:42
@TennyZhuang TennyZhuang merged commit c7a31f4 into main Apr 1, 2022
@TennyZhuang TennyZhuang deleted the feat/window-table-function branch April 1, 2022 03:44
@TennyZhuang TennyZhuang mentioned this pull request Apr 1, 2022
Closed
@xiangjinwu xiangjinwu linked an issue Apr 14, 2022 that may be closed by this pull request
Support temporal windows in TVF (Tumble & Hop) #1191
Closed
@xiangjinwu xiangjinwu removed a link to an issue Apr 14, 2022
Support temporal windows in TVF (Tumble & Hop) #1191
Closed
@xiangjinwu xiangjinwu mentioned this pull request Apr 14, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@xiangjinwu xiangjinwu xiangjinwu approved these changes

@skyzh skyzh Awaiting requested review from skyzh

@fuyufjh fuyufjh Awaiting requested review from fuyufjh

@xxchan xxchan Awaiting requested review from xxchan

@st1page st1page Awaiting requested review from st1page

Assignees
No one assigned
Labels
type/feature
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@TennyZhuang @fuyufjh @xiangjinwu