feat!: remove tumble function
#4367
Conversation
WalkthroughThe Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
github-actions
bot
added
breaking-change
github-actions
bot
added
docs-not-required
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (10)
- src/flow/src/compute/render/reduce.rs (1 hunks)
- src/flow/src/expr/func.rs (5 hunks)
- src/flow/src/expr/scalar.rs (1 hunks)
- src/flow/src/expr/signature.rs (1 hunks)
- src/flow/src/transform.rs (2 hunks)
- src/flow/src/transform/aggr.rs (13 hunks)
- src/flow/src/transform/expr.rs (2 hunks)
- src/flow/src/transform/plan.rs (3 hunks)
- tests/cases/standalone/common/flow/basic.result (1 hunks)
- tests/cases/standalone/common/flow/basic.sql (1 hunks)
Files skipped from review due to trivial changes (3)
- src/flow/src/compute/render/reduce.rs
- src/flow/src/expr/signature.rs
- src/flow/src/transform/expr.rs
Additional comments not posted (13)
tests/cases/standalone/common/flow/basic.sql (1)
Line range hint
1-24:
LGTM!The SQL commands are correctly updated to use the
date_truncfunction instead of thetumblefunction.tests/cases/standalone/common/flow/basic.result (1)
Line range hint
1-36:
LGTM!The expected results are correctly updated to reflect the changes in
basic.sql.src/flow/src/transform/plan.rs (1)
Line range hint
1-81:
LGTM!The code is correctly updated to remove references to the
tumblefunction and related logic.src/flow/src/transform.rs (1)
149-150: LGTM!The code is correctly updated to remove references to the
tumblefunction and related logic.src/flow/src/expr/func.rs (1)
70-74: LGTM! But verify the enum usage in the codebase.The removal of the
TumbleWindowvariant from theUnmaterializableFuncenum and the update to thefrom_str_argsmethod are consistent with the objective of removing thetumblefunction and its related code.However, ensure that all references to this variant have been removed or updated accordingly.
Verification successful
Verification Complete: No issues found.
The removal of the
TumbleWindowvariant from theUnmaterializableFuncenum has been successfully completed. There are no remaining references toTumbleWindowin the codebase.
- No occurrences of
UnmaterializableFunc::TumbleWindowfound.- The
UnmaterializableFuncreferences are related to other variants and are intact.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all references to `UnmaterializableFunc::TumbleWindow` have been removed or updated. # Test: Search for the enum variant usage. Expect: No occurrences of the variant. rg --type rust -A 5 $'UnmaterializableFunc::TumbleWindow'Length of output: 56
Script:
#!/bin/bash # Description: Verify all references to `TumbleWindow` have been removed or updated. # Test 1: Search for any references to `TumbleWindow` in the codebase. Expect: No occurrences of the variant. rg --type rust -A 5 $'TumbleWindow' # Test 2: Search for any references to `UnmaterializableFunc` to ensure all variants are accounted for. Expect: No references to `TumbleWindow`. rg --type rust -A 5 $'UnmaterializableFunc'Length of output: 9478
src/flow/src/transform/aggr.rs (8)
316-316: Verify the impact of settingtime_indextoNone.The
time_indexvariable is now set toNone. Ensure that this change does not affect the functionality of theTypedPlan::from_substrait_agg_relfunction and related components.
Line range hint
450-550:
LGTM!The
test_df_func_basictest function appears comprehensive and covers various cases.
Line range hint
556-647:
LGTM!The
test_df_func_expr_treetest function appears comprehensive and covers various cases.
Line range hint
649-715:
LGTM!The
test_avg_group_bytest function appears comprehensive and covers various cases.
Line range hint
717-775:
LGTM!The
test_avgtest function appears comprehensive and covers various cases.
Line range hint
777-835:
LGTM!The
test_sumtest function appears comprehensive and covers various cases.
Line range hint
837-899:
LGTM!The
test_sum_group_bytest function appears comprehensive and covers various cases.
Line range hint
901-961:
LGTM!The
test_sum_addtest function appears comprehensive and covers various cases.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4367 +/- ##
==========================================
- Coverage 85.15% 84.79% -0.36%
==========================================
Files 1063 1063
Lines 189987 189043 -944
==========================================
- Hits 161782 160298 -1484
- Misses 28205 28745 +540 |
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
test.logis excluded by!**/*.log
Files selected for processing (11)
- flow.ans (1 hunks)
- src/flow/src/compute/render/reduce.rs (1 hunks)
- src/flow/src/expr/func.rs (5 hunks)
- src/flow/src/expr/scalar.rs (1 hunks)
- src/flow/src/expr/signature.rs (1 hunks)
- src/flow/src/transform.rs (2 hunks)
- src/flow/src/transform/aggr.rs (13 hunks)
- src/flow/src/transform/expr.rs (2 hunks)
- src/flow/src/transform/plan.rs (3 hunks)
- tests/cases/standalone/common/flow/basic.result (1 hunks)
- tests/cases/standalone/common/flow/basic.sql (1 hunks)
Files skipped from review due to trivial changes (4)
- src/flow/src/compute/render/reduce.rs
- src/flow/src/expr/scalar.rs
- src/flow/src/expr/signature.rs
- src/flow/src/transform/expr.rs
Additional comments not posted (9)
tests/cases/standalone/common/flow/basic.sql (1)
Line range hint
1-18: Ensure alignment with newdate_binfunction.The SQL commands for table creation, data insertion, and selection should align with the new
date_binfunction. Verify that the changes accurately reflect the removal of thetumblefunction and the introduction of thedate_binfunction.tests/cases/standalone/common/flow/basic.result (1)
Line range hint
1-71: Ensure alignment with newdate_binfunction.The expected results should align with the new
date_binfunction. Verify that the changes accurately reflect the removal of thetumblefunction and the introduction of thedate_binfunction.flow.ans (1)
1-105: Ensure alignment with newdate_binfunction.The compilation and test results should align with the new
date_binfunction. Verify that the changes accurately reflect the removal of thetumblefunction and the introduction of thedate_binfunction.src/flow/src/transform/plan.rs (1)
81-81: Ensure alignment with newdate_binfunction.The Rust code should align with the new
date_binfunction. Verify that the changes accurately reflect the removal of thetumblefunction and the introduction of thedate_binfunction.src/flow/src/transform.rs (1)
149-150: Placeholder for custom functions.The placeholder comment indicates that custom functions can be added here in the future.
src/flow/src/expr/func.rs (2)
70-74: RemovedTumbleWindowvariant fromUnmaterializableFuncenum.The removal simplifies the enum and aligns with the PR objectives.
74-79: RemovedTumbleWindowFloorandTumbleWindowCeilingvariants fromUnaryFuncenum.The removal simplifies the enum and aligns with the PR objectives.
src/flow/src/transform/aggr.rs (2)
312-316: Removedfind_time_index_in_group_exprsfunction call.The removal simplifies the function and aligns with the PR objectives.
Line range hint
450-550:
Removedtest_tumble_group_byfunction.The removal aligns with the removal of the
tumblefunction and simplifies the test suite.
* feat!: remove `tumble` function * chore: forget to rm log file
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
|
Will wait for #4347 to merge first and rebase on it since same files are modified in both PR |
|
Close due to change of plan, will fix |
I hereby agree to the terms of the GreptimeDB CLA.
Refer to a related PR or issue link (optional)
What's changed and what's your intention?
Remove
tumblefunction which is buggy and caused many problem, the function is replaced bydate_binfunction instead:tumblefunction and related teststumblefunctions workChecklist
Summary by CodeRabbit
Refactor
TumbleWindowrelated variants and methods from functions, enums, and structs.TypedPlan.Bug Fixes
Chores
TumbleWindowacross multiple files.