Uh oh!
There was an error while loading. Please reload this page.
[Improve](array-func)improve array union support multi params - #24327
Conversation
amorynan
commented
Sep 13, 2023
run buildall |
clang-tidy review says "All clean, LGTM! 👍" |
doris-robot
commented
Sep 13, 2023
TeamCity be ut coverage result: |
doris-robot
commented
Sep 13, 2023
(From new machine)TeamCity pipeline, clickbench performance test result: |
clang-tidy review says "All clean, LGTM! 👍" |
amorynan
commented
Sep 14, 2023
run buildall |
doris-robot
commented
Sep 14, 2023
TeamCity be ut coverage result: |
doris-robot
commented
Sep 14, 2023
(From new machine)TeamCity pipeline, clickbench performance test result: |
| [['array_union'], 'ARRAY_VARCHAR', ['ARRAY_VARCHAR', 'ARRAY_VARCHAR'], ''], | ||
| [['array_union'], 'ARRAY_STRING', ['ARRAY_STRING', 'ARRAY_STRING'], ''], | ||
| [['array_union'], 'ARRAY_BOOLEAN', ['ARRAY_BOOLEAN', 'ARRAY_BOOLEAN', '...'], ''], | ||
| [['array_union'], 'ARRAY_TINYINT', ['ARRAY_TINYINT', 'ARRAY_TINYINT', '...'], ''], |
There was a problem hiding this comment.
should update org.apache.doris.nereids.trees.expressions.functions.scalar.ArrayUnion to let it support multi param in Nereids too
| if (entry.get_mapped() == params.size()) { | ||
| if constexpr (operation == MapOperation::INTERSECT) { | ||
| if (entry.get_mapped() == params.size()) { | ||
| ++dst_off; |
There was a problem hiding this comment.
I will make micro define
Uh oh!
There was an error while loading. Please reload this page.
| if (!set.find(elem)) { | ||
| set.insert(elem); | ||
| return true; | ||
| void apply(Map& map, const int arg_idx, const int row_idx, |
There was a problem hiding this comment.
- row_idx better use 64bit instead int.
- const int == int?
There was a problem hiding this comment.
maybe size_t is better ?
xy720
left a comment
There was a problem hiding this comment.
Why calculate multi array union need a map structure? Set is not enough?
amorynan
commented
Sep 14, 2023
because MapImpl can apply const ColumnArrayExecutionDatas params, but SetImpl just apply one ColumnArrayExecutionData |
clang-tidy review says "All clean, LGTM! 👍" |
clang-tidy review says "All clean, LGTM! 👍" |
amorynan
commented
Sep 16, 2023
run buildall |
doris-robot
commented
Sep 16, 2023
TeamCity be ut coverage result: |
doris-robot
commented
Sep 16, 2023
(From new machine)TeamCity pipeline, clickbench performance test result: |
PR approved by at least one committer and no changes requested. |
PR approved by anyone and no changes requested. |
clang-tidy review says "All clean, LGTM! 👍" |
doris-robot
commented
Sep 18, 2023
TeamCity be ut coverage result: |
doris-robot
commented
Sep 18, 2023
(From new machine)TeamCity pipeline, clickbench performance test result: |
xiaokang
commented
Sep 18, 2023
run buildall |
clang-tidy review says "All clean, LGTM! 👍" |
doris-robot
commented
Sep 18, 2023
TeamCity be ut coverage result: |
doris-robot
commented
Sep 18, 2023
(From new machine)TeamCity pipeline, clickbench performance test result: |
amorynan
commented
Sep 19, 2023
run p0 |
amorynan
commented
Sep 19, 2023
run p1 |
amorynan
commented
Sep 20, 2023
PR approved by at least one committer and no changes requested. |
Proposed changes
Issue Number: close #xxx
Now we just support array union with two arrays
Further comments
If this is a relatively large or complex change, kick off the discussion at dev@doris.apache.org by explaining why you chose the solution you did and what alternatives you considered, etc...