fix: Address incompatibility of constant folding for ipaddress #12065
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:
Presto may perform constant folding on queries before sending the fragment to velox workers. However, when the workers receive the fragments, the fragments may contain types which had a different implementation than how velox implemented the type. This incompatibility results in incorrect results.
For example, this PR fixes the type incompatibility between Java coordinator and C++ worker for `ipaddress` types.
- Java coordinator, ipaddress is represented as a slice of 16 bytes which if represented as a number, would be big endian.
- C++ worker, ipaddress is represented as an int128_t, in little endian form.
The discrepancy between these two can be see with on native engine, the result set will be `::ffff:1.2.3.4` represented in reverse byte order
```
SELECT
CAST(ip AS ipaddress) as casted_ip
FROM
(
VALUES
('::ffff:1.2.3.4')
) AS t (ip)
```
To address this issue, we can reverse the byte order of the ipaddress type sent from and to Java.
**Note**:
- This issue is not exclusive to ipaddrss, and other custom types in velox which have different underlying type/implementation than Java may suffer from this issue as well.
- We can likely enhance the fuzzer to help catch cases like this at diff time once custom fuzzer inputs are landed (facebookincubator#11466)
Differential Revision: D68039050
facebook-github-bot
added
the
CLA Signed
|
This pull request was exported from Phabricator. Differential Revision: D68039050 |
✅ Deploy Preview for meta-velox canceled.
|
|
cc @mohsaka as well (not sure why your name doesn't show up when I try to add you as a reviewer ... ) |
|
Created issue for IPPrefix here: #12070 |
|
will open a new diff to avoid rebase hell |
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:
Presto may perform constant folding on queries before sending the fragment to velox workers. However, when the workers receive the fragments, the fragments may contain types which had a different implementation than how velox implemented the type. This incompatibility results in incorrect results.
For example, this PR fixes the type incompatibility between Java coordinator and C++ worker for
ipaddresstypes.The discrepancy between these two can be see with on native engine, the result set will be
::ffff:1.2.3.4represented in reverse byte orderTo address this issue, we can reverse the byte order of the ipaddress type sent from and to Java.
Note:
This issue is not exclusive to ipaddrss, and other custom types in velox which have different underlying type/implementation than Java may suffer from this issue as well.
We can likely enhance the fuzzer to help catch cases like this at diff time once custom fuzzer inputs are landed (feat(fuzzer): Support custom input generator in VectorFuzzer #11466)
Differential Revision: D68039050