Skip to content

Add IPAddress Presto type #10596

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

Closed
mohsaka wants to merge 21 commits into from
Closed

Add IPAddress Presto type #10596

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
16d69ff
Add varbinary
mohsaka Jul 26, 2024
5df77ca
All formatted
mohsaka Jul 26, 2024
0541476
Address comments
mohsaka Aug 2, 2024
a8dd6fb
comments
mohsaka Aug 2, 2024
f56b527
Comments
mohsaka Aug 2, 2024
b7e4836
Format
mohsaka Aug 2, 2024
fadf329
Add IPAddress to docs
mohsaka Aug 6, 2024
6aaf937
Test case changes
mohsaka Aug 7, 2024
44845a8
Some documentation fixes
mohsaka Aug 8, 2024
6c39388
Address more comments
mohsaka Aug 8, 2024
f31614a
From ipaddress casts
mohsaka Aug 8, 2024
a941c93
Address comments
mohsaka Aug 8, 2024
e8897a2
format
mohsaka Aug 8, 2024
0519e94
Address more comments
mohsaka Aug 8, 2024
afc20ab
Format
mohsaka Aug 8, 2024
3bbc2fa
add return
mohsaka Aug 8, 2024
7ad4fcd
Add type
mohsaka Aug 8, 2024
63d9ad2
address comments
mohsaka Aug 9, 2024
2047cca
Allocate entire buffer
mohsaka Aug 9, 2024
af963ee
Fix documentation
mohsaka Aug 12, 2024
5eea4c6
Remove buffer allocation
mohsaka Aug 12, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions velox/docs/develop/types.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -136,6 +136,7 @@ HYPERLOGLOG VARBINARY
JSON VARCHAR
TIMESTAMP WITH TIME ZONE BIGINT
UUID HUGEINT
IPADDRESS HUGEINT
======================== =====================

TIMESTAMP WITH TIME ZONE represents a time point in milliseconds precision
Expand All @@ -146,6 +147,14 @@ Supported range of milliseconds is [0xFFF8000000000000L, 0x7FFFFFFFFFFFF]
store timezone ID. Supported range of timezone ID is [1, 1680].
The definition of timezone IDs can be found in ``TimeZoneDatabase.cpp``.

IPADDRESS represents an IPV6 or IPV4 formatted IPV6 address. Its physical
type is HUGEINT. The format that the address is stored in is defined as part of `(RFC 4291#section-2.5.5.2) <https://datatracker.ietf.org/doc/html/rfc4291.html#section-2.5.5.2>`_
As Velox is run on Little Endian systems and the standard is network byte(Big Endian)
order, we reverse the bytes to allow for masking and other bit operations
used in IPADDRESS/IPPREFIX related functions. This type can be used to
Copy link
Copy Markdown
Contributor

@mbasmanova mbasmanova Aug 8, 2024

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

Copy link
Copy Markdown
Collaborator Author

@mohsaka mohsaka Aug 8, 2024

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 is 00000000000000000000ffff01010101 and which has a binary value of 281470698586369

1.1.1.2 in hex is 00000000000000000000ffff01010102 which has a binary value of 281470698586370 which is one larger.

1.1.2.1 in hex is 00000000000000000000ffff01010201 which is 281470698586625 which is larger than both 1.1.1.1 and 1.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.

  1. When we sort a mixture of IPV4 and IPV6 there will be a split between IPV6 and IPV4 addresses at ::FFFF:FFFF:FFFF This one should exist in presto.
    So the ordering will be for example
::FFFE:FFFF:FFFF
0.0.0.0
....
255.255.255.255
::1:0000:0000:0000
  1. The other problem being that any IPV6 IPADDRESS with the MSB set will be placed at the start instead of at the end. This one shouldn't exist in presto as its doing a compareUnsigned.
    For example
FFFF:FFFF:FFFF:FFFF:FFFF:FFFF:FFFF:FFFF
::
::1

Copy link
Copy Markdown
Contributor

@mbasmanova mbasmanova Aug 9, 2024

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.

    @Override
    public int compareTo(Block leftBlock, int leftPosition, Block rightBlock, int rightPosition)
    {
        int compare = Long.compareUnsigned(reverseBytes(leftBlock.getLong(leftPosition, 0)), reverseBytes(rightBlock.getLong(rightPosition, 0)));
        if (compare != 0) {
            return compare;
        }
        return Long.compareUnsigned(reverseBytes(leftBlock.getLong(leftPosition, SIZE_OF_LONG)), reverseBytes(rightBlock.getLong(rightPosition, SIZE_OF_LONG)));
    }

Copy link
Copy Markdown
Collaborator Author

@mohsaka mohsaka Aug 9, 2024

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.

Copy link
Copy Markdown
Contributor

@mbasmanova mbasmanova Aug 9, 2024

Choose a reason for hiding this comment

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

Not sure if this is possible

Not yet. @bikramSingh91 has been looking into this as part of #10338

Copy link
Copy Markdown
Contributor

@mbasmanova mbasmanova Aug 9, 2024

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).

Copy link
Copy Markdown
Contributor

@amitkdutta amitkdutta Aug 9, 2024

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.

Copy link
Copy Markdown
Collaborator Author

@mohsaka mohsaka Aug 9, 2024

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.

create IPPREFIX networks as well as to check IPADDRESS validity within
IPPREFIX networks.

Spark Types
~~~~~~~~~~~~
The `data types <https://spark.apache.org/docs/latest/sql-ref-datatypes.html>`_ in Spark have some semantic differences compared to those in
Expand Down
Loading