-
Notifications
You must be signed in to change notification settings - Fork 3.7k
[Feature](function) support function array_combinations #60192
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:
|
866f02d to
8317b21
Compare
| return comb; | ||
| } | ||
|
|
||
| bool _next_combination(std::vector<size_t>& comb, Int64 k) 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.
what's the meaning of I, j, k? dont use those meaningless identifier
|
|
||
| Status execute_impl(FunctionContext* context, Block& block, const ColumnNumbers& arguments, | ||
| uint32_t result, size_t input_rows_count) const override { | ||
| auto left = block.get_by_position(arguments[0]).column->convert_to_full_column_if_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.
dont directly use convert_to_full_column_if_const, but vector_const...
| Status execute_impl(FunctionContext* context, Block& block, const ColumnNumbers& arguments, | ||
| uint32_t result, size_t input_rows_count) const override { | ||
| auto left = block.get_by_position(arguments[0]).column->convert_to_full_column_if_const(); | ||
| auto* src_arr = assert_cast<ColumnArray*>(remove_nullable(left)->assume_mutable().get()); |
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 need assume_mutable here?
| "array_combinations first argument must be Array"); | ||
| DataType itemType = ((ArrayType) arg0Type).getItemType(); | ||
| return FunctionSignature.ret(ArrayType.of(ArrayType.of(itemType))) | ||
| .args(getArgument(0).getDataType(), getArgument(1).getDataType()); |
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.
what if arg1 is not number?
| sql """ INSERT INTO t_array_combinations VALUES(1, ['foo','bar','baz'], [1,2,3], [1,2,2], [[1,1],[4,5],[1,4]]) """ | ||
|
|
||
| qt_test """ | ||
| select k1, array_combinations(s1, 2), array_combinations(a1, 2), array_combinations(a2, 2), array_combinations(aa1, 2) from t_array_combinations order by k1; |
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.
there's too few testcases. should add more to cover situations
|
|
||
| ColumnPtr _execute_combination(const ColumnArray* nested, size_t input_rows_count, | ||
| const ColumnArray::ColumnOffsets& offsets, Int64 k) const { | ||
| const auto& data_col = nested->get_data(); |
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.
re-consider all your var names
|
|
||
| std::vector comb = _first_combination(k, row_len); | ||
|
|
||
| for (int i = 0; i < static_cast<size_t>(k); ++i) { |
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 put a single same for-loop outside the while-loop? maybe you need a do-while?
| size_t curr_off = in_offs[row]; | ||
| size_t row_len = curr_off - prev_off; | ||
|
|
||
| if (k <= 0 || static_cast<size_t>(k) > row_len) { |
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 reg case and make sure this behaviour is same with target system
| for (int i = 0; i < static_cast<size_t>(k); ++i) { | ||
| size_t idx = prev_off + comb[i]; | ||
| data_col.get(idx, element); | ||
| inner->get_data().insert(element); |
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.
directly use insert_from could get rid of Field?
|
|
||
| ColumnPtr _execute_combination(const ColumnArray* nested, size_t input_rows_count, | ||
| const ColumnArray::ColumnOffsets& offsets, Int64 k) const { | ||
| const auto& data_col = nested->get_data(); |
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.
you can reserve for result column
What problem does this PR solve?
Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)