Support Custom Comparison in Simple Function API #11136
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Summary: This change extends CustomType in the Simple Function API, templating it on providesCustomComparison, with the idea that Custom Types that provide custom comparison operators will set this value to true when they define their CustomType template overrides. In Simple Functions, this allows us to wrap the value in a view, called CustomTypeWithCustomComparisonView, which overrides the standard comparison operators and hash function to use the operators provided by the Type. It also provides a dereference operator to access the raw underlying value. The only other piece to this diff is adding a VectorReader that constructs the CustomTypeWithCustomComparisonView. This should help address facebookincubator#8713 so we don't have to create special flavors of UDF just for TimestampWithTimezone. On a side note, if we extend this so that if a Custom Type does not provide custom comparison operators we produce a similar view without the custom comparison operators, that would likely solve the issue with custom types and the simple function interface entirely. In the follow up diff, I'll clean up the TimestampWithTimezone UDFs for comparisons and greatest/least. Differential Revision: D63670301
facebook-github-bot
added
the
CLA Signed
✅ Deploy Preview for meta-velox canceled.
|
|
This pull request was exported from Phabricator. Differential Revision: D63670301 |
kevinwilfong
pushed a commit
to kevinwilfong/velox
that referenced
this pull request
Oct 1, 2024
Summary: With the changes to the Simple Function API introduced in facebookincubator#11136 we no longer need to register separate UDFs for TimestampWithTimezone to avoid collisions with int64_t at function resolution time. We also don't need to specialize the comparison logic. Cleaning up the existing special cases in the comparison and greatest/least UDFs. Differential Revision: D63704263
kevinwilfong
pushed a commit
to kevinwilfong/velox
that referenced
this pull request
Oct 1, 2024
facebookincubator#11139) Summary: Pull Request resolved: facebookincubator#11139 With the changes to the Simple Function API introduced in facebookincubator#11136 we no longer need to register separate UDFs for TimestampWithTimezone to avoid collisions with int64_t at function resolution time. We also don't need to specialize the comparison logic. Cleaning up the existing special cases in the comparison and greatest/least UDFs. Differential Revision: D63704263
kevinwilfong
pushed a commit
to kevinwilfong/velox
that referenced
this pull request
Oct 1, 2024
Summary: With facebookincubator#11136 array_min and array_max just work with TimestampWithTimezone. Adding unit tests to verify this and ensure it doesn't break in the future. Differential Revision: D63708110
kevinwilfong
pushed a commit
to kevinwilfong/velox
that referenced
this pull request
Oct 1, 2024
Summary: With facebookincubator#11136 array_sort just works with TimestampWithTimezone. Adding unit tests to verify this and ensure it doesn't break in the future. Differential Revision: D63711984
kevinwilfong
pushed a commit
to kevinwilfong/velox
that referenced
this pull request
Oct 1, 2024
Summary: With facebookincubator#11136 array_sort just works with TimestampWithTimezone. Adding unit tests to verify this and ensure it doesn't break in the future. Differential Revision: D63711984
kevinwilfong
pushed a commit
to kevinwilfong/velox
that referenced
this pull request
Oct 1, 2024
…kincubator#11141) Summary: Pull Request resolved: facebookincubator#11141 With facebookincubator#11136 array_min and array_max just work with TimestampWithTimezone. Adding unit tests to verify this and ensure it doesn't break in the future. Differential Revision: D63708110
kevinwilfong
pushed a commit
to kevinwilfong/velox
that referenced
this pull request
Oct 1, 2024
…kincubator#11141) Summary: Pull Request resolved: facebookincubator#11141 With facebookincubator#11136 array_min and array_max just work with TimestampWithTimezone. Adding unit tests to verify this and ensure it doesn't break in the future. Differential Revision: D63708110
kevinwilfong
pushed a commit
to kevinwilfong/velox
that referenced
this pull request
Oct 1, 2024
…r#11142) Summary: Pull Request resolved: facebookincubator#11142 With facebookincubator#11136 array_sort just works with TimestampWithTimezone. Adding unit tests to verify this and ensure it doesn't break in the future. Differential Revision: D63711984
kevinwilfong
pushed a commit
to kevinwilfong/velox
that referenced
this pull request
Oct 1, 2024
…r#11142) Summary: Pull Request resolved: facebookincubator#11142 With facebookincubator#11136 array_sort just works with TimestampWithTimezone. Adding unit tests to verify this and ensure it doesn't break in the future. Differential Revision: D63711984
|
This pull request has been merged in 822c295. |
facebook-github-bot
pushed a commit
that referenced
this pull request
Oct 3, 2024
#11139) Summary: Pull Request resolved: #11139 With the changes to the Simple Function API introduced in #11136 we no longer need to register separate UDFs for TimestampWithTimezone to avoid collisions with int64_t at function resolution time. We also don't need to specialize the comparison logic. Cleaning up the existing special cases in the comparison and greatest/least UDFs. Reviewed By: bikramSingh91 Differential Revision: D63704263 fbshipit-source-id: 4fb6b60d2ae6675f8b64acb58ecded93f53b65af
kevinwilfong
pushed a commit
to kevinwilfong/velox
that referenced
this pull request
Oct 3, 2024
…kincubator#11141) Summary: With facebookincubator#11136 array_min and array_max just work with TimestampWithTimezone. Adding unit tests to verify this and ensure it doesn't break in the future. Differential Revision: D63708110
kevinwilfong
pushed a commit
to kevinwilfong/velox
that referenced
this pull request
Oct 3, 2024
…r#11142) Summary: With facebookincubator#11136 array_sort just works with TimestampWithTimezone. Adding unit tests to verify this and ensure it doesn't break in the future. Reviewed By: bikramSingh91 Differential Revision: D63711984
kevinwilfong
pushed a commit
to kevinwilfong/velox
that referenced
this pull request
Oct 7, 2024
…kincubator#11141) Summary: With facebookincubator#11136 array_min and array_max just work with TimestampWithTimezone. Adding unit tests to verify this and ensure it doesn't break in the future. Differential Revision: D63708110
kevinwilfong
pushed a commit
to kevinwilfong/velox
that referenced
this pull request
Oct 7, 2024
…kincubator#11141) Summary: With facebookincubator#11136 array_min and array_max just work with TimestampWithTimezone. Adding unit tests to verify this and ensure it doesn't break in the future. Differential Revision: D63708110
kevinwilfong
pushed a commit
to kevinwilfong/velox
that referenced
this pull request
Oct 7, 2024
…r#11142) Summary: With facebookincubator#11136 array_sort just works with TimestampWithTimezone. Adding unit tests to verify this and ensure it doesn't break in the future. Reviewed By: bikramSingh91 Differential Revision: D63711984
kevinwilfong
pushed a commit
to kevinwilfong/velox
that referenced
this pull request
Oct 7, 2024
…kincubator#11141) Summary: With facebookincubator#11136 array_min and array_max just work with TimestampWithTimezone. Adding unit tests to verify this and ensure it doesn't break in the future. Differential Revision: D63708110
kevinwilfong
pushed a commit
to kevinwilfong/velox
that referenced
this pull request
Oct 7, 2024
…r#11142) Summary: With facebookincubator#11136 array_sort just works with TimestampWithTimezone. Adding unit tests to verify this and ensure it doesn't break in the future. Reviewed By: bikramSingh91 Differential Revision: D63711984
facebook-github-bot
pushed a commit
that referenced
this pull request
Oct 8, 2024
Summary: Pull Request resolved: #11141 With #11136 array_min and array_max just work with TimestampWithTimezone. Adding unit tests to verify this and ensure it doesn't break in the future. Reviewed By: kgpai Differential Revision: D63708110 fbshipit-source-id: 4dfd162c0914a5a2ec6304f6aaedd505a3921913
kevinwilfong
pushed a commit
to kevinwilfong/velox
that referenced
this pull request
Oct 10, 2024
Summary: With facebookincubator#11136 array_union and array_remove just work with TimestampWithTimezone. Adding unit tests to verify this and ensure it doesn't break in the future. Differential Revision: D64196032
kevinwilfong
pushed a commit
to kevinwilfong/velox
that referenced
this pull request
Oct 10, 2024
…facebookincubator#11222) Summary: With facebookincubator#11136 array_union and array_remove just work with TimestampWithTimezone. Adding unit tests to verify this and ensure it doesn't break in the future. Differential Revision: D64196032
kevinwilfong
pushed a commit
to kevinwilfong/velox
that referenced
this pull request
Oct 10, 2024
…facebookincubator#11222) Summary: With facebookincubator#11136 array_union and array_remove just work with TimestampWithTimezone. Adding unit tests to verify this and ensure it doesn't break in the future. Differential Revision: D64196032
kevinwilfong
pushed a commit
to kevinwilfong/velox
that referenced
this pull request
Oct 10, 2024
…facebookincubator#11222) Summary: With facebookincubator#11136 array_union and array_remove just work with TimestampWithTimezone. Adding unit tests to verify this and ensure it doesn't break in the future. Differential Revision: D64196032
facebook-github-bot
pushed a commit
that referenced
this pull request
Oct 11, 2024
…#11222) Summary: Pull Request resolved: #11222 With #11136 array_union and array_remove just work with TimestampWithTimezone. Adding unit tests to verify this and ensure it doesn't break in the future. Reviewed By: xiaoxmeng Differential Revision: D64196032 fbshipit-source-id: fd2a98e61f674978a7f1099155d79c3261e46851
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary:
This change extends CustomType in the Simple Function API, templating it on
providesCustomComparison, with the idea that Custom Types that provide custom
comparison operators will set this value to true when they define their CustomType
template overrides.
In Simple Functions, this allows us to wrap the value in a view, called
CustomTypeWithCustomComparisonView, which overrides the standard comparison
operators and hash function to use the operators provided by the Type. It also provides a
dereference operator to access the raw underlying value.
The only other piece to this diff is adding a VectorReader that constructs the
CustomTypeWithCustomComparisonView.
This should help address #8713
so we don't have to create special flavors of UDF just for TimestampWithTimezone.
On a side note, if we extend this so that if a Custom Type does not provide custom
comparison operators we produce a similar view without the custom comparison operators,
that would likely solve the issue with custom types and the simple function interface
entirely.
In the follow up diff, I'll clean up the TimestampWithTimezone UDFs for comparisons and
greatest/least.
Differential Revision: D63670301