Skip to content
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.

Sign up for GitHub

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

Jump to bottom

feat: Support array_join(Unknown) #12444

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

feat: Support array_join(Unknown) #12444

wants to merge 1 commit into from

Conversation

xiaodouchen
Copy link
Contributor

@xiaodouchen xiaodouchen commented Feb 25, 2025 •
edited
Loading

Support array_join(Unknown).

Note: array_join() has already supported json type. The CI/CD test
failed and threw an exception "No default Hive type provided for
unsupported Hive type: json" in Presto fuzzer test. For more detailed
information, please refer to #12452.
We fix it by adding this function in skipFunctions and it will lose
fuzzer coverage.

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Feb 25, 2025
@netlify Netlify
Copy link

netlify bot commented Feb 25, 2025 •
edited
Loading

✅ Deploy Preview for meta-velox canceled.

Name Link
🔨 Latest commit 1bff842
🔍 Latest deploy log https://app.netlify.com/sites/meta-velox/deploys/67becab3a4076400089330a5

@xiaodouchen
Copy link
Contributor Author

@kgpai @laithsakka @mbasmanova Please help review this PR when you have time.

mbasmanova
mbasmanova reviewed Feb 25, 2025
View reviewed changes
Copy link
Contributor

@mbasmanova mbasmanova left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@xiaodouchen There are CI failures. Would you take a look?

mbasmanova
mbasmanova requested changes Feb 25, 2025
View reviewed changes
velox/functions/prestosql/tests/ArrayJoinTest.cpp
TEST_F(ArrayJoinTest, unknownTest) {
testArrayJoinNoReplacement<UnknownValue>(
{std::nullopt, std::nullopt, std::nullopt}, ","_sv, ""_sv);
testArrayJoinReplacement<UnknownValue>(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test case doesn't seem right. I'd expect all NULL values to be replaced with 'apple'.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, you're right. Fixed.

@xiaodouchen xiaodouchen force-pushed the array_join_unknown branch 2 times, most recently from 409ea83 to 8172ddc Compare February 26, 2025 03:14
@xiaodouchen
Copy link
Contributor Author

xiaodouchen commented Feb 26, 2025 •
edited
Loading

@xiaodouchen There are CI failures. Would you take a look?

@mbasmanova array_join supports json type and throws exception "No default Hive type provided for unsupported Hive type: json" in Presto fuzzer test. It is a known issue in #12286. We fix it by adding this function in skipFunctions.

mbasmanova
mbasmanova reviewed Feb 26, 2025
View reviewed changes
velox/expression/fuzzer/ExpressionFuzzerTest.cpp Outdated
@@ -198,6 +198,8 @@ int main(int argc, char** argv) {
"current_date", // Non-deterministic
"xxhash64_internal",
"combine_hash_internal",
"array_join", // No default Hive type provided for unsupported Hive
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would you add a link to GitHub issue that provides more details?

It seems that with this change we lose Fuzzer coverage... is there a way to keep the coverage? If not, let's at least call this out in PR description.

CC: @kgpai @kagamiori

Copy link
Contributor Author

@xiaodouchen xiaodouchen Feb 26, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mbasmanova

  1. Yes, added. ( array_join: Expression Fuzzer throws exception for the json type #12452)
  2. Sorry, I don't know how to keep the coverage. I have mentioned this limitation in the PR description.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bikramSingh91 @amitkdutta @kagamiori @kgpai Krishna, Wei, Bikram, would you comment on whether we are Ok with loosing Fuzzer coverage for array_join.

kgpai
kgpai reviewed Feb 26, 2025
View reviewed changes
velox/expression/fuzzer/ExpressionFuzzerTest.cpp
@@ -200,6 +200,7 @@ int main(int argc, char** argv) {
"combine_hash_internal",
"map_keys_by_top_n_values", // requires
// https://github.com/prestodb/presto/pull/24570
"array_join", // https://github.com/facebookincubator/velox/issues/12452
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be separate PR and not part of this change since it would be good to ensure that your changes are being tested in the fuzzer.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kgpai kgpai kgpai left review comments

@mbasmanova mbasmanova mbasmanova requested changes

Assignees
No one assigned
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@xiaodouchen @kgpai @mbasmanova @facebook-github-bot