EQL: Length function implementation#54209
Conversation
|
Pinging @elastic/es-ql |
| def(Substring.class, Substring::new, "substring"), | ||
| }, | ||
| def(Length.class, Length::new, "length"), | ||
| def(Substring.class, Substring::new, "substring") |
There was a problem hiding this comment.
honest question: what's our convention for trailing commas after the last line in a list?
just noticed the changes here and wondering which I should do in my PRs
There was a problem hiding this comment.
I think that was a leftover. I don't see a point in having a comma if there is nothing left afterwards. The compiler doesn't complain, as it's ignoring it, but imo it shouldn't be there.
There was a problem hiding this comment.
I'm used to Go approach with trailing comma and like the reasoning for it:
https://dave.cheney.net/2014/10/04/that-trailing-comma
| import static org.elasticsearch.xpack.ql.expression.gen.script.ParamsBuilder.paramsBuilder; | ||
|
|
||
| /** | ||
| * EQL specific length function acting on every type of field, not only strings. |
There was a problem hiding this comment.
I think that documentation is outdated and before EQL had a type system.
Here's the current definition. It accepts only strings and arrays. I think it's okay if we drop array support for now, like we have by igoring other functions
https://github.com/endgameinc/eql/blob/master/eql/functions.py#L427-L441
I'll fix the EQL documentation quick.
There was a problem hiding this comment.
That's unfortunate. I need to remove some tests and adjust the logic...
It would be good if other functions' documentation would be double-checked to make sure we implement the right functionality.
There was a problem hiding this comment.
Yeah. I hope that didn't waste too much of your time, and I'm sorry about that. I'll do a quick double check over the EQL documentation and add a type signature to the functions issue
|
|
||
| public Length(Source source, Expression src) { | ||
| super(source, Arrays.asList(src)); | ||
| this.source = src; |
There was a problem hiding this comment.
the name mismatch is slightly confusing
There was a problem hiding this comment.
it is the same in many other places, so ... it's consistent.
| if (source == null) { | ||
| return null; | ||
| } | ||
| if (source instanceof String == false && source instanceof Character == false) { |
There was a problem hiding this comment.
I was wondering about this after our discussion today and when thinking of edge cases for wildcard #53999.
What are the conditions that would cause us to hit this path?
There was a problem hiding this comment.
That's a good question. I cannot think of a scenario where a Character is being received. Maybe the JSON XContent parser would (in a future implementation) consider a single character coming over the wire to be a Character instead of a String.
| VerificationException e = expectThrows(VerificationException.class, | ||
| () -> plan("process where length(plain_text) > 0")); | ||
| String msg = e.getMessage(); | ||
| assertEquals("Found 1 problem\nline 1:15: [length(plain_text)] cannot operate on field of data type [text]: No keyword/multi-field " |
There was a problem hiding this comment.
If this is always done in painless anyway, why can't this work? It would just retrieve the doc value right?
also if this is just a matter of scoping the function to the more cut-and-dry cases, that's fine too
There was a problem hiding this comment.
Text fields cannot use doc_values and the only way for Painless to load a text field is through _source. For one, _source is expensive to load. Also, it will load the entire _source, not only the source for that specific field. Performance wise it will be painfull (pun intended).
|
@elastic/es-ql |
|
@elasticmachine update branch |
(cherry picked from commit 1849346)
Addresses #53853.