-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
Fixing return type for IPV4 #15916
Fixing return type for IPV4 #15916
Conversation
@@ -49,7 +49,7 @@ public class IPv4AddressMatchOperatorConversion extends DirectOperatorConversion | |||
SUBNET_OPERAND | |||
) | |||
) | |||
.returnTypeInference(ReturnTypes.BOOLEAN_NULLABLE) | |||
.returnTypeNullable(SqlTypeName.BOOLEAN) |
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.
should we add a test for this one too?
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.
actually, why is this wrong? does the method also return null if the subnet argument is not a valid address too?
if it does indeed need to change, does ipv6_match also need updated? https://github.com/apache/druid/blob/master/sql/src/main/java/org/apache/druid/sql/calcite/expression/builtin/IPv6AddressMatchOperatorConversion.java#L50
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.
This does not need to be changed as ipv4_match even if 2nd arg is null throws a validation exception and does not return null in anyway. So I think it can be left out
@somu-imply Can we rebase and merge this ? |
As shown here, the following query with ipv4_parse('1.2.3') returns a Calcite assertion instead of null when the string literal ‘1.2.3' can’t be parsed as an IP address.
The reason was the return type was not set correctly. This PR fixes it.
This PR has: