EQL: Allow sample queries without a timestamp field#90629
Conversation
|
Pinging @elastic/es-ql (Team:QL) |
| ); | ||
| Filter joinKeyNotNull = new Filter(join.source(), k.child(), constraint); | ||
| filters.set(i, new KeyedFilter(k.source(), joinKeyNotNull, k.keys(), k.timestamp(), k.timestamp())); | ||
| filters.set(i, new KeyedFilter(k.source(), joinKeyNotNull, k.keys(), k.timestamp(), k.tiebreaker())); |
There was a problem hiding this comment.
this looks like an oversight in the original implementation, not specifically related to this fix (I just took the chance to fix it)
astefan
left a comment
There was a problem hiding this comment.
I agree with the solution but not with the tests. Also, the solution is fixing the timestamp_field part, but extrapolating this a bit, it doesn't cover the tiebreaker field as well.
At the moment, a very similar test, but specifying a "tiebreaker_field" results in the same error as the one for the timestamp_field.
Removing the date fields from the sample*.data files is not actually testing the fix. To make it more diverse, instead, you could keep sample1 and sample2 as is and remove the timestamp fields from sample3. Still, this doesn't test the fix, but it makes the IT tests more complex.
To test the fix you could probably test the verifier outcome for timestamp_field and tiebreaker_field presence.
| List<NamedExpression> out = new ArrayList<>(); | ||
|
|
||
| out.add(timestamp); | ||
| if (Expressions.isPresent(timestamp)) { |
There was a problem hiding this comment.
Add a comment here for the scenario when the timestamp is not actually present, even though is a required request parameter.
Also, adjust the class level comment of this class which says "Used inside Join or Sequence".
and add one more test
|
Thanks for the comments @astefan, adding a few notes below:
Not sure if this case should be considered a user mistake (explicitly specify a tiebreaker field that does not exist), but the fix is easy, so I added it as well.
It's the opposite actually: if at least one index has the |
|
@elasticmachine run elasticsearch-ci/rest-compatibility |
Sample queries do not rely on time sequences, so they don't need a timestamp field to execute.
Since Samples were implemented within current EQL framework, they inherited all the checks for the presence of a timestamp field in the target index(es), even though it was not actually used.
This PR removes this limitation.