Extract references tracking from SearchLookup #68202
Conversation
romseygeek
changed the title
| throw new IllegalArgumentException("Field requires resolving too many dependent fields: " + String.join(" -> ", newFieldChain)); | ||
| } | ||
| return new SearchLookup(this, newFieldChain); | ||
| public SearchLookup forkAndTrackReferences(String field) { |
There was a problem hiding this comment.
I had hoped to be able to remove this method entirely, but we need it in runtime field queries if we want the root of the field cycle to be reported accurately. In other words, without this method we still get errors thrown on cycles, but instead of being "field -> a -> b -> field" the cycle is reported as "a -> b -> field -> a". I'm not sure if this is a real problem or not - it's a perfectly good cycle detection wherever it starts from. I lean towards removing it anyway, as I think the additional API surface isn't worth the different error message.
There was a problem hiding this comment.
I kind of like the error message being accurate. Could we drop the API and have the queries make TrackingMappedFieldsLookup and immediate call the method to add themselves to its list?
| protected final void checkLoop(BiConsumer<MappedFieldType, SearchExecutionContext> queryBuilder) { | ||
| Exception e = expectThrows(IllegalArgumentException.class, () -> queryBuilder.accept(loopFieldType(), mockContext())); | ||
| protected final void checkLoop(BiFunction<MappedFieldType, SearchExecutionContext, Query> queryBuilder) throws IOException { | ||
| Query query = queryBuilder.apply(loopFieldType(), mockContext(true, loopFieldType())); |
There was a problem hiding this comment.
This wasn't really checking an actual production code path before. We now run a search, which forces the script to look up values triggering the loop handling.
| return ft.rangeQuery(randomLong(), randomLong(), randomBoolean(), randomBoolean(), null, null, null, ctx); | ||
| long lower = randomLongBetween(0, 1000); | ||
| long upper = lower + randomLongBetween(5, 5000); | ||
| return ft.rangeQuery(lower, upper, randomBoolean(), randomBoolean(), null, null, null, ctx); | ||
| } |
There was a problem hiding this comment.
Because we're actually executing the random query now, we need to ensure its parameter constraints are met.
romseygeek
added
:Search/Search
|
Pinging @elastic/es-search (Team:Search) |
| } | ||
| } | ||
|
|
||
| public TrackingMappedFieldsLookup excludingField(String field) { |
There was a problem hiding this comment.
It doesn't really exclude the field, right? It tracks it?
| throw new IllegalArgumentException("Field requires resolving too many dependent fields: " + String.join(" -> ", newFieldChain)); | ||
| } | ||
| return new SearchLookup(this, newFieldChain); | ||
| public SearchLookup forkAndTrackReferences(String field) { |
There was a problem hiding this comment.
I kind of like the error message being accurate. Could we drop the API and have the queries make TrackingMappedFieldsLookup and immediate call the method to add themselves to its list?
Reference tracking for field lookups during a script call will be needed for
calculated fields, and may also be useful for things like alias lookups in
future. This commit extracts it from SearchLookup into a dedicated
TrackingMappedFieldLookup class.