test(native): Add ipprefix and ipaddress tests to native-tests#26905
Merged
aditi-pandit merged 3 commits intoprestodb:masterfrom Feb 4, 2026
Merged
test(native): Add ipprefix and ipaddress tests to native-tests#26905aditi-pandit merged 3 commits intoprestodb:masterfrom
aditi-pandit merged 3 commits intoprestodb:masterfrom
Conversation
wongquijote
requested review from
a team,
elharo,
feilong-liu,
jaystarshot and
vivek-bharathan
as code owners
prestodb-ci
requested review from
a team,
bibith4 and
wanglinsong
and removed request for
a team
Contributor
Reviewer's GuideAdds shared ipprefix/ipaddress test suite and native engine coverage, while refactoring IP type locations and dependencies to use common type classes and support IPPREFIX in test clients. Class diagram for shared IP types, operators, functions, and testsclassDiagram
class Type
class AbstractType
class IpAddressType {
}
class IpPrefixType {
}
class BuiltInTypeAndFunctionNamespaceManager {
}
class IpAddressOperators {
}
class IpPrefixOperators {
}
class IpPrefixFunctions {
}
class AbstractTestFunctions {
}
class AbstractTestNativeFunctions {
}
class AbstractTestIpPrefix {
}
class TestIpPrefixFunctions {
}
class TestingPrestoClient {
}
class CheckUnsupportedPrestissimoTypes {
}
Type <|-- AbstractType
AbstractType <|-- IpAddressType
AbstractType <|-- IpPrefixType
BuiltInTypeAndFunctionNamespaceManager --> IpAddressType
BuiltInTypeAndFunctionNamespaceManager --> IpPrefixType
IpAddressOperators --> IpAddressType
IpPrefixOperators --> IpPrefixType
IpPrefixFunctions --> IpPrefixType
AbstractTestFunctions <|-- AbstractTestNativeFunctions
AbstractTestFunctions <|-- AbstractTestIpPrefix
AbstractTestIpPrefix --> IpPrefixFunctions
AbstractTestIpPrefix --> IpPrefixType
AbstractTestNativeFunctions <|-- TestIpPrefixFunctions
TestIpPrefixFunctions --> AbstractTestIpPrefix
TestingPrestoClient --> IpPrefixType
CheckUnsupportedPrestissimoTypes --> Type
CheckUnsupportedPrestissimoTypes ..> IpAddressType
class IpAddressType{
<<moved_to_presto_common_type>>
}
class IpPrefixType{
<<moved_to_presto_common_type>>
}
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- In CheckUnsupportedPrestissimoTypes, instead of commenting out the IPADDRESS check, either remove the dead code and configuration flag or reintroduce it behind a clearly named feature toggle so the behavior is explicit and testable.
- The new AbstractTestIpPrefix is an interface with default @test methods; consider renaming it (e.g., IpPrefixTestMixin) or adding a brief comment to clarify its intended use as a shared test mixin rather than an abstract base class.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In CheckUnsupportedPrestissimoTypes, instead of commenting out the IPADDRESS check, either remove the dead code and configuration flag or reintroduce it behind a clearly named feature toggle so the behavior is explicit and testable.
- The new AbstractTestIpPrefix is an interface with default @Test methods; consider renaming it (e.g., IpPrefixTestMixin) or adding a brief comment to clarify its intended use as a shared test mixin rather than an abstract base class.
## Individual Comments
### Comment 1
<location> `presto-main-tests/src/main/java/com/facebook/presto/tests/operator/scalar/AbstractTestIpPrefix.java:81-82` </location>
<code_context>
- };
- }
-
@Test
- public void testIpAddressIpPrefix()
+ public void testInvalidIpAddressIpPrefix()
</code_context>
<issue_to_address>
**suggestion (testing):** Consider adding null-handling tests for IP_PREFIX to cover SQL null semantics
Current `IP_PREFIX` tests cover various IPv4/IPv6 and prefix-length combinations, but not null inputs. Please add tests for cases like `IP_PREFIX(CAST(NULL AS IPADDRESS), 24)` and `IP_PREFIX(IPADDRESS '1.2.3.4', CAST(NULL AS INTEGER))` so null semantics are verified consistently for both engine and native implementations.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
pramodsatya
reviewed
Jan 6, 2026
Contributor
pramodsatya
left a comment
pramodsatya
left a comment
There was a problem hiding this comment.
Thanks @wongquijote, LGTM overall % few comments.
| if (type.equals(IPADDRESS) && config.isDisableIPAddressForNative()) { | ||
| return Optional.of(ipAddressErrorMessage); | ||
| } | ||
| // if (type.equals(IPADDRESS) && config.isDisableIPAddressForNative()) { |
Contributor
There was a problem hiding this comment.
Why is this change needed? For the ipaddress native-tests, can we not set this config to false?
| @Test | ||
| public void testInvalidIpAddressIpPrefix() | ||
| { | ||
| assertInvalidFunction("IP_PREFIX(IPADDRESS '::ffff:1.2.3.4', -1)", "IPv4 subnet size must be in range [0, 32]"); |
Contributor
There was a problem hiding this comment.
These error messages seem to be the same as Presto java error messages? Why are these tests not a part of AbstractTestIpPrefix interface?
Contributor
Author
There was a problem hiding this comment.
The methods assertInvalidCast and assertInvalidFunction are protected utility methods defined within the AbstractTestNativeFunctions base class.
I wasn't sure if I should change these methods to public, but I'll update in another PR.
Initially, I took this design from AbstractTestFunctions which I believe designed it this way because these methods are specific implementation details of the testing framework that shouldn't be exposed rather than public behavioral specifications, therefore they are not included in the AbstractTestIpPrefix interface. Consequently, tests that rely on these protected helpers must reside in the concrete class (TestIpPrefixFunctions) which has access to the base class's internal methods via inheritance.
Contributor
Author
There was a problem hiding this comment.
I added another commit addressing these comments.
| * limitations under the License. | ||
| */ | ||
| package com.facebook.presto.type; | ||
| package com.facebook.presto.common.type; |
Contributor
There was a problem hiding this comment.
Could you separate the changes moving IpAddress and IpPrefix types to a separate commit/PR?
Contributor
Author
There was a problem hiding this comment.
It's updated now as two separate commits
Contributor
|
Please sign the Presto CLA as mentioned in this comment. Please add Release Notesin the PR description. |
wongquijote
force-pushed
the
ipprefix_tests
branch
from
b37e220 to
a633165
Compare
wongquijote
force-pushed
the
ipprefix_tests
branch
from
5796659 to
36d91fd
Compare
Contributor
|
Thanks @wongquijote for this PR. Want to check if Tim is comfortable with the refactor for IPAddressType module you have done. @tdcmeehan : Do you have any concerns with moving IPAddress/IPPrefix types from presto-common to presto-main-base to consolidate all the tests as proposed here ? |
aditi-pandit
approved these changes
Feb 4, 2026
Contributor
aditi-pandit
left a comment
aditi-pandit
left a comment
There was a problem hiding this comment.
Thanks @wongquijote
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
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:
Ported the IpPrefix and IpAddress tests in https://github.com/prestodb/presto/blob/master/presto-main-base/src/test/java/com/facebook/presto/operator/scalar/TestIpPrefixFunctions.java to run with Presto Native engine in presto-native-tests.
This is a continuation of the work to refactor scalar function tests from
presto-main-basetopresto-main-testsfrom this PR: #26013Also moved IpPrefixType and IpAddressType into
presto-commonfrompresto-main-basedue to some dependency cycles that appeared after refactoring.== NO RELEASE NOTE ==