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.
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.
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
Add IPAddress Presto type #10596
Add IPAddress Presto type #10596
Changes from all commits
16d69ff
5df77ca
0541476
a8dd6fb
f56b527
b7e4836
fadf329
6aaf937
44845a8
6c39388
f31614a
a941c93
e8897a2
0519e94
afc20ab
3bbc2fa
7ad4fcd
63d9ad2
2047cca
af963ee
5eea4c6
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this an orderable type? If so, does OrderBy produce correct results if it treats values of this type the same as values of BIGINT type? If not, we may have a problem similar to #10338
CC: @pedroerp @bikramSingh91
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mbasmanova
I believe that it is working somewhat correctly. To confirm I do have an E2E test that was added for my complete PR of IPADDRESS/IPPREFIX/functions.
prestodb/presto#23282
I believe that the reason for this is due to the 1 to 1 mapping of IPADDRESS to BIGINT and how they are stored.
For example the ipaddress
1.1.1.1
in hex is00000000000000000000ffff01010101
and which has a binary value of281470698586369
1.1.1.2
in hex is00000000000000000000ffff01010102
which has a binary value of281470698586370
which is one larger.1.1.2.1
in hex is00000000000000000000ffff01010201
which is281470698586625
which is larger than both1.1.1.1
and1.1.1.2
as it should be.Similar with IPV6.
That being said that does point out two issues. One that I don't think presto even takes care of.
::FFFF:FFFF:FFFF
This one should exist in presto.So the ordering will be for example
For example
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for looking into this. I'll need more time to understand.
I see that compare in Presto for IPADDRESS is not the same as for BIGINT (or maybe they just look different). This suggest that IPADDRESS type is affected by #10338 and therefore we won't be able to use it (without causing correctness issues) until that issue is resolved or we find a way to block code paths that may be affected.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No problem. Simplest fix would be to switch to varbinary but that is not very efficient. Other option would be to override the operator similar to how we override cast? Not sure if this is possible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not yet. @bikramSingh91 has been looking into this as part of #10338
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mohsaka One option might be to proceed to land this change and continue landing functions that use that type, but document that it is not "ready to be used" and add a plan validator to Presto to reject plans with that type when running native engine (we have that for TS_w_TS type).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mohsaka We should add the plan validator with a config like it was done for timestamp with timezone in Presto coordiantor first before merging this change in velox.
Example:
prestodb/presto#23200
prestodb/presto#23374
It will allow us to test it, but disable for production or correctness critical use cases. If we merge this and worker start executing queries with IPAddress type unconditonally, we will face correctness problems.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@amitkdutta Thank you for the pointers. Will work on adding the plan changes before merging in these changes.