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

Add json_array_contains Presto Json function #2299

Closed
wants to merge 4 commits into from
Closed

Add json_array_contains Presto Json function #2299

wants to merge 4 commits into from

Conversation

pramodsatya
Copy link
Collaborator

Adds Json function json_array_contains to determine if value exists in json (a string containing a JSON array).

@pramodsatya pramodsatya marked this pull request as draft August 16, 2022 02:27
@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 Aug 16, 2022
@pramodsatya pramodsatya mentioned this pull request Aug 16, 2022
Open
@Yuhta Yuhta requested a review from kagamiori August 16, 2022 13:47
@pramodsatya pramodsatya force-pushed the json_contains branch 3 times, most recently from eb5eff2 to e1a8bfd Compare August 16, 2022 17:23
kagamiori
kagamiori reviewed Aug 16, 2022
View reviewed changes
Copy link
Contributor

@kagamiori kagamiori left a comment

Choose a reason for hiding this comment

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

Hi Pramod, Thank you for adding this function!

velox/docs/functions/json.rst Outdated Show resolved Hide resolved
velox/functions/prestosql/tests/JsonFunctionsTest.cpp Outdated Show resolved Hide resolved
velox/functions/prestosql/JsonFunctions.h Outdated Show resolved Hide resolved
velox/functions/prestosql/tests/JsonFunctionsTest.cpp Outdated Show resolved Hide resolved
velox/functions/prestosql/tests/JsonFunctionsTest.cpp Outdated Show resolved Hide resolved
@pramodsatya pramodsatya force-pushed the json_contains branch 2 times, most recently from 71c614b to 2093a6e Compare August 17, 2022 18:46
aditi-pandit
aditi-pandit reviewed Aug 17, 2022
View reviewed changes
velox/docs/functions.rst Outdated Show resolved Hide resolved
velox/functions/prestosql/JsonFunctions.h Outdated Show resolved Hide resolved
velox/functions/prestosql/JsonFunctions.h Show resolved Hide resolved
@pramodsatya pramodsatya force-pushed the json_contains branch 2 times, most recently from 915dfbd to 6c63db3 Compare August 17, 2022 22:41
aditi-pandit
aditi-pandit approved these changes Aug 18, 2022
View reviewed changes
Copy link
Collaborator

@aditi-pandit aditi-pandit left a comment

Choose a reason for hiding this comment

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

Thanks Pramod for the changes. One comment related to tests.

velox/functions/prestosql/tests/JsonFunctionsTest.cpp Show resolved Hide resolved
@aditi-pandit aditi-pandit marked this pull request as ready for review August 18, 2022 19:28
@netlify
Copy link

netlify bot commented Aug 18, 2022 •
edited
Loading

✅ Deploy Preview for meta-velox canceled.

Name Link
🔨 Latest commit 44bdca9
🔍 Latest deploy log https://app.netlify.com/sites/meta-velox/deploys/62fed01ff2e407000a867abe

kagamiori
kagamiori reviewed Aug 18, 2022
View reviewed changes
velox/functions/prestosql/JsonFunctions.h Outdated
bool valueInt = std::is_same_v<TInput, int64_t>;
bool valueDouble = std::is_same_v<TInput, double>;
for (const auto& v : parsedJson) {
if (valueBool && v.isBool() && v == value) {
Copy link
Contributor

Choose a reason for hiding this comment

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

std::is_same_v<TInput, bool> can be determined at compile time, so it's better not to do runtime checks on it. (Same for int64_t and double.) With the current code, valueBool, valueInt, and valueDouble are computed and checked at runtime. Consider using if constexpr (...) for type checks and do the value comparison inside. The rest looks good to me.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks this is a good suggestion. Agree its best to handle this in compilation. Otherwise choosing to write the templated code while taking these runtime hits is not worth the tradeoff.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks for the suggestion, changed accordingly

kagamiori
kagamiori approved these changes Aug 19, 2022
View reviewed changes
Copy link
Contributor

@kagamiori kagamiori left a comment

Choose a reason for hiding this comment

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

LGTM. Thank you very much for iterating on it! I'm going to import this PR and will let you know if there are further changes needed.

@facebook-github-bot
Copy link
Contributor

@kagamiori has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@pramodsatya pramodsatya deleted the json_contains branch August 22, 2022 14:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aditi-pandit aditi-pandit aditi-pandit approved these changes

@kagamiori kagamiori kagamiori approved these 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
@pramodsatya @facebook-github-bot @kagamiori @aditi-pandit