-
Notifications
You must be signed in to change notification settings - Fork 3.7k
[Feature](function) Support function cross_product of DuckDB #59223
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:
|
...-test/suites/query_p0/sql_functions/array_functions/test_array_cross_product_function.groovy
Outdated
Show resolved
Hide resolved
...-test/suites/query_p0/sql_functions/array_functions/test_array_cross_product_function.groovy
Show resolved
Hide resolved
.../src/main/java/org/apache/doris/nereids/trees/expressions/functions/scalar/CrossProduct.java
Outdated
Show resolved
Hide resolved
| const auto& arg1 = block.get_by_position(arguments[0]); | ||
| const auto& arg2 = block.get_by_position(arguments[1]); | ||
|
|
||
| auto col1 = arg1.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.
use vector_const and const_vector to deal constancy
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.
I want to confirm my understanding:
should I explicitly handle ColumnConst instead of calling convert_to_full_column_if_const()?
If my understanding is incorrect, I would appreciate your guidance on how to handle this.
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.
yes. convert_to_full_column_if_const will lead to performance problem
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.
done
| get_name(), size1, size2); | ||
| } | ||
|
|
||
| if (size1 != 3 || size2 != 3) { |
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.
L156 and L162 are same check?
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.
I don't think so. One ensures length consistency between the two inputs, while the other enforces a fixed-size 3 requirement.
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.
I mean, if if (size1 != 3 || size2 != 3) get false, then if (size1 != size2) must be false, right? they are entailment relationship. so just remove the first.
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.
done
|
and remember to format your code ~ |
.../src/main/java/org/apache/doris/nereids/trees/expressions/functions/scalar/CrossProduct.java
Outdated
Show resolved
Hide resolved
.../src/main/java/org/apache/doris/nereids/trees/expressions/functions/scalar/CrossProduct.java
Outdated
Show resolved
Hide resolved
3e5e023 to
58fb654
Compare
zclllyybb
left a comment
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.
Hi, there's still some problem. besides some reply of previous comments
| ssize_t size2 = offset2->get_data()[row] - prev_offset2; | ||
|
|
||
| if (size1 == 0 || size2 == 0) { | ||
| current_offset += 3; |
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.
nested_data.resize didn't write the default value. so I think you should also set nested_data here
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.
done
|
|
||
| if (arr1->get_data_ptr()->is_nullable()) { | ||
| if (arr1->get_data_ptr()->has_null()) { | ||
| throw doris::Exception(ErrorCode::INVALID_ARGUMENT, |
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.
replace all throw with return status in this function
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.
done
| get_name(), size1, size2); | ||
| } | ||
|
|
||
| if (size1 != 3 || size2 != 3) { |
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.
I mean, if if (size1 != 3 || size2 != 3) get false, then if (size1 != size2) must be false, right? they are entailment relationship. so just remove the first.
58fb654 to
c0b84d3
Compare
c0b84d3 to
402901b
Compare
What problem does this PR solve?
Issue Number: #48203
Related PR: #xxx
Problem Summary:
Release note
doc: apache/doris-website#3209
The CROSS_PRODUCT function is used to compute the cross product of two arrays of size 3.
Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)