ESQL: Implement network_direction function
#136133
Conversation
elasticsearchmachine
added
the
Team:Analytics
|
Pinging @elastic/es-analytical-engine (Team:Analytics) |
|
Pinging @elastic/kibana-esql (ES|QL-ui) |
|
Hi @MattAlp, I've created a changelog YAML for you. |
🔍 Preview links for changed docs |
...t/java/org/elasticsearch/xpack/esql/expression/function/scalar/ip/NetworkDirectionTests.java
Outdated
Show resolved
Hide resolved
...c/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/ip/NetworkDirection.java
Show resolved
Hide resolved
|
I don't know if there is such a thing as aliases in ESQL scalar functions, but if there was, would be nice to only have to write |
| @Override | ||
| public String paramName(boolean blockStyle) { | ||
| return name + (blockStyle ? "Block" : "Vector"); | ||
| return name + "Block"; |
There was a problem hiding this comment.
Are you sure this is needed?
I ran ./gradlew :x-pack:plugin:esql:compileJava locally on this repo, with and without this change, and I don't see any generated classes change. I think it isn't needed.
There was a problem hiding this comment.
I believe that's because I've since changed the signature and annotations on the process method, which no longer triggers the faulty codegen.
In order to reproduce, you should be able to check out the earlier commit 3129e57, revert this change, and see what happens when you attempt to compile.
Nik should be able to confirm when he's back - as I understand, BlockArguments can't be represented as Vectors by definition, which is why the reproducer will create valid control flow but mistakenly refer to a ${name}Vector on one line while referencing the ${name}Block earlier.
There was a problem hiding this comment.
I made this change in my local copy, then ran the gradle command from earlier. The result is captured in the screenshot.
Looks like the semantics are broken since addRawVector method is called and passed a valuesBlock instead of valuesVector. Not that it matters a lot, I mean compiler is happy, and method is no-op anyways so param can be called anything. It just breaks the semantics, that's all. You're right that vectors don't mean much for the block arg class, but at least the current way both the method and its param name are in sync.
There was a problem hiding this comment.
That screenshot looks fine - though I had just asked @mouhc1ne about removing this method entirely because it's not called. Let me have a look at the rest of it.
There was a problem hiding this comment.
OK! I understand now. @MattAlp does indeed need this change. We don't have any other functions that'd bump into this. But, it does, indeed, make a lame parameter name on those methods. But, like 20 minutes ago, I requested skipping generating these methods anyway. So I guess that's fine.
It looks like there is, https://github.com/mattalp/elasticsearch/blob/24d8f307acd7ed2e249774899a94cd0135464b16/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/EsqlFunctionRegistry.java#L457 |
...in/java/org/elasticsearch/xpack/esql/expression/function/scalar/ScalarFunctionWritables.java
Show resolved
Hide resolved
...c/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/ip/NetworkDirection.java
Show resolved
Hide resolved
| import java.util.List; | ||
| import java.util.function.Supplier; | ||
|
|
||
| import static org.hamcrest.core.IsEqual.equalTo; |
There was a problem hiding this comment.
I believe it's more idiomatic to import Matchers instead of IsEqual. Not that it matters a ton though.
| @Override | ||
| public String paramName(boolean blockStyle) { | ||
| return name + (blockStyle ? "Block" : "Vector"); | ||
| return name + "Block"; |
There was a problem hiding this comment.
OK! I understand now. @MattAlp does indeed need this change. We don't have any other functions that'd bump into this. But, it does, indeed, make a lame parameter name on those methods. But, like 20 minutes ago, I requested skipping generating these methods anyway. So I guess that's fine.
5 participants
network_direction(source_ip, destination_ip, networks)function based on the processor of the same nameBlockArgument.javain order to create a functioning evaluator.