Add tests for Presto comparison functions for custom types with custom comparison functions #11019
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
facebook-github-bot
added
the
CLA Signed
✅ Deploy Preview for meta-velox canceled.
|
|
This pull request was exported from Phabricator. Differential Revision: D62893143 |
|
This pull request was exported from Phabricator. Differential Revision: D62893143 |
kevinwilfong
pushed a commit
to kevinwilfong/velox
that referenced
this pull request
Sep 17, 2024
…m comparison functions (facebookincubator#11019) Summary: Pull Request resolved: facebookincubator#11019 facebookincubator#11015 added support for custom types to provide custom comparison functions, motivated by the need to support TimestampWithTimezone's comparison semantics. I validated the UDFs provided in Comparisons.h on top of this change. It does not look like any further changes are needed in those files, but I added explicit tests for TimestampWithTimezone in the various comparison UDFs. I also added tests for the UDFs that support Generic arguments, to ensure that when the Type underlying the Generic provides custom comparison functions, that is respected. I considered adding checks to the UDFs that support templated arguments to ensure the type does not provide custom comparison functions, as these are not supported in any of them (e.g. between, <, >, and the SIMD versions of comparison functions). However, someone would have to explicitly register these functions for the custom type, and I don't think it's unreasonable to expect the user to test these functions work as expected when doing so, I don't think the per-batch check is worth the (admittedly small) overhead. Differential Revision: D62893143
kevinwilfong
force-pushed
the
export-D62893143
branch
from
65d7718
to
c7b46a9
Compare
|
This pull request was exported from Phabricator. Differential Revision: D62893143 |
kevinwilfong
pushed a commit
to kevinwilfong/velox
that referenced
this pull request
Sep 17, 2024
…m comparison functions (facebookincubator#11019) Summary: Pull Request resolved: facebookincubator#11019 facebookincubator#11015 added support for custom types to provide custom comparison functions, motivated by the need to support TimestampWithTimezone's comparison semantics. I validated the UDFs provided in Comparisons.h on top of this change. It does not look like any further changes are needed in those files, but I added explicit tests for TimestampWithTimezone in the various comparison UDFs. I also added tests for the UDFs that support Generic arguments, to ensure that when the Type underlying the Generic provides custom comparison functions, that is respected. I considered adding checks to the UDFs that support templated arguments to ensure the type does not provide custom comparison functions, as these are not supported in any of them (e.g. between, <, >, and the SIMD versions of comparison functions). However, someone would have to explicitly register these functions for the custom type, and I don't think it's unreasonable to expect the user to test these functions work as expected when doing so, I don't think the per-batch check is worth the (admittedly small) overhead. Differential Revision: D62893143
kevinwilfong
force-pushed
the
export-D62893143
branch
from
c7b46a9
to
c0fb0d3
Compare
|
This pull request was exported from Phabricator. Differential Revision: D62893143 |
kevinwilfong
pushed a commit
to kevinwilfong/velox
that referenced
this pull request
Sep 25, 2024
…m comparison functions (facebookincubator#11019) Summary: Pull Request resolved: facebookincubator#11019 facebookincubator#11015 added support for custom types to provide custom comparison functions, motivated by the need to support TimestampWithTimezone's comparison semantics. I validated the UDFs provided in Comparisons.h on top of this change. It does not look like any further changes are needed in those files, but I added explicit tests for TimestampWithTimezone in the various comparison UDFs. I also added tests for the UDFs that support Generic arguments, to ensure that when the Type underlying the Generic provides custom comparison functions, that is respected. I considered adding checks to the UDFs that support templated arguments to ensure the type does not provide custom comparison functions, as these are not supported in any of them (e.g. between, <, >, and the SIMD versions of comparison functions). However, someone would have to explicitly register these functions for the custom type, and I don't think it's unreasonable to expect the user to test these functions work as expected when doing so, I don't think the per-batch check is worth the (admittedly small) overhead. Reviewed By: kgpai Differential Revision: D62893143
kevinwilfong
force-pushed
the
export-D62893143
branch
from
c0fb0d3
to
534a9de
Compare
…m comparison functions (facebookincubator#11019) Summary: Pull Request resolved: facebookincubator#11019 facebookincubator#11015 added support for custom types to provide custom comparison functions, motivated by the need to support TimestampWithTimezone's comparison semantics. I validated the UDFs provided in Comparisons.h on top of this change. It does not look like any further changes are needed in those files, but I added explicit tests for TimestampWithTimezone in the various comparison UDFs. I also added tests for the UDFs that support Generic arguments, to ensure that when the Type underlying the Generic provides custom comparison functions, that is respected. I considered adding checks to the UDFs that support templated arguments to ensure the type does not provide custom comparison functions, as these are not supported in any of them (e.g. between, <, >, and the SIMD versions of comparison functions). However, someone would have to explicitly register these functions for the custom type, and I don't think it's unreasonable to expect the user to test these functions work as expected when doing so, I don't think the per-batch check is worth the (admittedly small) overhead. Reviewed By: kgpai Differential Revision: D62893143
|
This pull request was exported from Phabricator. Differential Revision: D62893143 |
kevinwilfong
force-pushed
the
export-D62893143
branch
from
534a9de
to
29fea07
Compare
|
This pull request has been merged in 52750de. |
|
Conbench analyzed the 1 benchmark run on commit There were no benchmark performance regressions. 🎉 The full Conbench report has more details. |
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:
#11015 added support for custom types to provide
custom comparison functions, motivated by the need to support TimestampWithTimezone's
comparison semantics.
I validated the UDFs provided in Comparisons.h on top of this change.
It does not look like any further changes are needed in those files, but I added explicit tests for
TimestampWithTimezone in the various comparison UDFs. I also added tests for the UDFs that
support Generic arguments, to ensure that when the Type underlying the Generic provides custom
comparison functions, that is respected.
I considered adding checks to the UDFs that support templated arguments to ensure the type does
not provide custom comparison functions, as these are not supported in any of them (e.g. between,
<, >, and the SIMD versions of comparison functions). However, someone would have to explicitly
register these functions for the custom type, and I don't think it's unreasonable to expect the user to
test these functions work as expected when doing so, I don't think the per-batch check is worth the
(admittedly small) overhead.
Differential Revision: D62893143