-
Notifications
You must be signed in to change notification settings - Fork 138
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 CIDR function to PPL (#3036) #3110
base: main
Are you sure you want to change the base?
Conversation
- Update Lexer and Parser. - Add `DSL::cidr` and BuiltinFunctionName.CIDR`. - Add `BinaryPredicateOperator::cidr`. - Add `IPUtils` with stub implementation of `isAddressInRange`. Signed-off-by: currantw <[email protected]>
Signed-off-by: currantw <[email protected]>
…nge` implementation to return `false` rather than raising an exception. Signed-off-by: currantw <[email protected]>
Signed-off-by: currantw <[email protected]>
Signed-off-by: currantw <[email protected]>
Signed-off-by: currantw <[email protected]>
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.
There should be an integration test as well.
Please check if there is an explain test needed and others such as AstBuiderTest, AstExpressionBuliderTest
@@ -0,0 +1,33 @@ | |||
package org.opensearch.sql.utils; |
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.
Please add the license header.
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.
Thanks. Done. ✅
|
||
return ExprBooleanValue.of(Arrays.equals(addressBytes, 0, prefixLengthBytes, rangeBytes, 0, prefixLengthBytes)); | ||
} catch (Exception e) { | ||
return ExprBooleanValue.of(false); |
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 this return ExprNullValue instead?
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 these parameters are inputs from the user PPL (and cannot be field references), is that correct? Would it make sense for the parser to resolve this instead? And throw a syntax exception if it is a bad format?
byte[] rangeBytes = Arrays.copyOfRange(InetAddresses.forString(rangeFields[0]).getAddress(), 0, prefixLengthBytes); | ||
|
||
return ExprBooleanValue.of(Arrays.equals(addressBytes, 0, prefixLengthBytes, rangeBytes, 0, prefixLengthBytes)); | ||
} catch (Exception e) { |
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.
I think we should explicitly check that the string is one of the permitted formats rather than throw/catch potential exceptions like index out of bounds exceptions.
@ParameterizedTest(name = "and({0}, {1})") | ||
@MethodSource("binaryPredicateArguments") | ||
public void test_and(Boolean v1, Boolean v2) { | ||
FunctionExpression and = DSL.and(DSL.literal(booleanValue(v1)), DSL.literal(booleanValue(v2))); | ||
assertEquals(BOOLEAN, and.type()); | ||
assertEquals(v1 && v2, ExprValueUtils.getBooleanValue(and.valueOf(valueEnv()))); | ||
assertEquals(String.format("and(%s, %s)", v1.toString(), v2.toString()), and.toString()); | ||
assertEquals(String.format("and(%s, %s)", v1, v2.toString()), and.toString()); |
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 and the adding of final are good changes, but we should keep changes in this PR specific to the issue you're working on. Let's have the clean-up improvements in a separate Issue and PR.
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.
Sounds good. I've reverted the cleanup changes to final
and toString
. ✅
Signed-off-by: currantw <[email protected]>
Signed-off-by: currantw <[email protected]>
Description
Adds the
cidr
function to PPL, which returns whether an IP address is within an IP address range specified in CIDR notation.Related Issues
Resolves #3036.
Check List
--signoff
.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.