-
Notifications
You must be signed in to change notification settings - Fork 3.7k
[feature](function) Support interval function #57569
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.
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
base: master
Are you sure you want to change the base?
Conversation
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
run buildall |
TPC-DS: Total hot run time: 190260 ms |
ClickBench: Total hot run time: 27.61 s |
|
run feut |
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
FE UT Coverage ReportIncrement line coverage |
|
run external |
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
FE Regression Coverage ReportIncrement line coverage |
| size_t right = num_thresholds; | ||
| size_t result_idx = num_thresholds; | ||
|
|
||
| while (left < right) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why not use std::upper_bound?
| scalar(Unhex.class, "unhex"), | ||
| scalar(UnhexNull.class, "unhex_null"), | ||
| scalar(Uncompress.class, "uncompress"), | ||
| scalar(UnhexNull.class, "unhex_null"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
format
| String get_name() const override { return name; } | ||
| bool is_variadic() const override { return true; } | ||
| size_t get_number_of_arguments() const override { return 0; } | ||
| bool use_default_implementation_for_nulls() const override { return true; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
default value is already true
| Status execute_impl(FunctionContext* /*context*/, Block& block, const ColumnNumbers& arguments, | ||
| uint32_t result, size_t input_rows_count) const override { | ||
| if (arguments.size() < 2) { | ||
| return Status::InvalidArgument("interval requires at least 2 arguments"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should make sure it's checked in FE. add [[unlikely]] and return InternalError
| WHERE id <= 7 | ||
| ORDER BY id; | ||
| """ | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add test which some of thresholds are const
| res_data); | ||
| break; | ||
| default: | ||
| return Status::InvalidArgument( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
| auto compare_cwn = block.get_by_position(arguments[0]); | ||
| auto compare_col_ptr = compare_cwn.column; | ||
| bool compare_is_const = false; | ||
| std::tie(compare_col_ptr, compare_is_const) = unpack_if_const(compare_col_ptr); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use default_preprocess_parameter_columns here is more proper
|
run buildall |
FE UT Coverage ReportIncrement line coverage |
TPC-DS: Total hot run time: 189552 ms |
ClickBench: Total hot run time: 27.63 s |
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
|
run cloud_p0 |
|
run nonConcurrent |
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
1 similar comment
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
|
run nonConcurrent |
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
FE Regression Coverage ReportIncrement line coverage |
| ColumnPtr compare_col_ptr = ColumnPtr {}; | ||
| bool compare_is_const = false; | ||
| compare_is_const = is_column_const(*block.get_by_position(arguments[0]).column); | ||
| default_preprocess_parameter_columns(&compare_col_ptr, &compare_is_const, {0}, block, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
default_preprocess_parameter_columns should process parameter columns, that is, column 1 .. n-1
|
|
||
| switch (compare_cwn.type->get_primitive_type()) { | ||
| case PrimitiveType::TYPE_TINYINT: | ||
| compute_interval<ColumnInt8>(block, arguments, *compare_col_ptr, compare_is_const, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
then, you can reference FunctionMonthsBetween, there's 3 possibility: data const, parameters all const, non const
… process (#58885) Related PR: #57569 Problem Summary: fix the wrong useage of function `default_preprocess_parameter_columns`, the variable `all_const` should not be affected by manually handled columns. ### Release note doc: apache/doris-website#3176 The INTERVAL function uses binary search to return the index of the first threshold strictly greater than N. ```sql SELECT INTERVAL(0, 1, 10, 100); +--------------------------+ | INTERVAL(0, 1, 10, 100) | +--------------------------+ | 0 | +--------------------------+ SELECT INTERVAL(10, 1, 10, 100, 1000); +-----------------------------------+ | INTERVAL(10, 1, 10, 100, 1000) | +-----------------------------------+ | 2 | +-----------------------------------+ -- First parameter is NULL SELECT INTERVAL(NULL, 1, 10, 100); +----------------------------+ | INTERVAL(NULL, 1, 10, 100) | +----------------------------+ | -1 | +----------------------------+ -- Subsequent parameters are NULL, treated as 0 SELECT INTERVAL(3, -1, NULL, 2, 3, 4); +--------------------------------+ | INTERVAL(3, -1, NULL, 2, 3, 4) | +--------------------------------+ | 4 | +--------------------------------+ ``` Co-authored-by: jianhao <1367919489@qq.com>
… process (#58885) Related PR: #57569 Problem Summary: fix the wrong useage of function `default_preprocess_parameter_columns`, the variable `all_const` should not be affected by manually handled columns. ### Release note doc: apache/doris-website#3176 The INTERVAL function uses binary search to return the index of the first threshold strictly greater than N. ```sql SELECT INTERVAL(0, 1, 10, 100); +--------------------------+ | INTERVAL(0, 1, 10, 100) | +--------------------------+ | 0 | +--------------------------+ SELECT INTERVAL(10, 1, 10, 100, 1000); +-----------------------------------+ | INTERVAL(10, 1, 10, 100, 1000) | +-----------------------------------+ | 2 | +-----------------------------------+ -- First parameter is NULL SELECT INTERVAL(NULL, 1, 10, 100); +----------------------------+ | INTERVAL(NULL, 1, 10, 100) | +----------------------------+ | -1 | +----------------------------+ -- Subsequent parameters are NULL, treated as 0 SELECT INTERVAL(3, -1, NULL, 2, 3, 4); +--------------------------------+ | INTERVAL(3, -1, NULL, 2, 3, 4) | +--------------------------------+ | 4 | +--------------------------------+ ``` Co-authored-by: jianhao <1367919489@qq.com>
… process (apache#58885) Related PR: apache#57569 Problem Summary: fix the wrong useage of function `default_preprocess_parameter_columns`, the variable `all_const` should not be affected by manually handled columns. ### Release note doc: apache/doris-website#3176 The INTERVAL function uses binary search to return the index of the first threshold strictly greater than N. ```sql SELECT INTERVAL(0, 1, 10, 100); +--------------------------+ | INTERVAL(0, 1, 10, 100) | +--------------------------+ | 0 | +--------------------------+ SELECT INTERVAL(10, 1, 10, 100, 1000); +-----------------------------------+ | INTERVAL(10, 1, 10, 100, 1000) | +-----------------------------------+ | 2 | +-----------------------------------+ -- First parameter is NULL SELECT INTERVAL(NULL, 1, 10, 100); +----------------------------+ | INTERVAL(NULL, 1, 10, 100) | +----------------------------+ | -1 | +----------------------------+ -- Subsequent parameters are NULL, treated as 0 SELECT INTERVAL(3, -1, NULL, 2, 3, 4); +--------------------------------+ | INTERVAL(3, -1, NULL, 2, 3, 4) | +--------------------------------+ | 4 | +--------------------------------+ ``` Co-authored-by: jianhao <1367919489@qq.com>
What problem does this PR solve?
Issue Number: close #xxx
Related PR: #xxx
https://github.com/apache/doris-website/pull/3032/files
Problem Summary:
Release note
None
Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)