ESQL: Forbid "load" unmapped_fields for certain commands#144115
ESQL: Forbid "load" unmapped_fields for certain commands#144115elasticsearchmachine merged 10 commits intoelastic:mainfrom
Conversation
…forbid_unmapped_fields_load
…fan/elasticsearch into forbid_unmapped_fields_load
|
Pinging @elastic/es-analytical-engine (Team:Analytics) |
alex-spies
left a comment
There was a problem hiding this comment.
Thanks @astefan ! Generally LGTM. However, one error message triggers wrongly, namely subqueries trigger the FORK error message, too (see below for more details). Also, some of the positive tests appear to be duplicated.
Please, go ahead and merge at your own discretion after addressing comments :)
| ); | ||
|
|
||
| assumeFalse( | ||
| "FORK is not supported with unmapped_fields=\"load\"", |
There was a problem hiding this comment.
nit: We could add a comment pointing to #142033?
Also applies elsewhere where this PR disables testing of some queries with load.
| public void testForkDisallowedWithUnmappedFieldsLoad() { | ||
| assumeTrue("Requires OPTIONAL_FIELDS_V2", EsqlCapabilities.Cap.OPTIONAL_FIELDS_V2.isEnabled()); |
There was a problem hiding this comment.
nit: that could be moved to the AnalyzerUnmappedTests. They have utilities that auto-disable tests that require load, namely setUnmappedLoad which prepends a query with SET unmapped_fields="load" and skips the test if that's disabled.
|
|
||
| /** | ||
| * {@code unmapped_fields="load"} does not yet support branching commands (FORK, LOOKUP JOIN, subqueries/views). | ||
| * See https://github.com/elastic/elasticsearch/issues/142367 |
There was a problem hiding this comment.
nit: implementing support is a separate follow-up issue, namely #142033
| private static void checkLoadModeDisallowedCommands(LogicalPlan plan, Failures failures) { | ||
| plan.forEachDown(p -> { | ||
| if (p instanceof Fork) { | ||
| failures.add(fail(p, "FORK is not supported with unmapped_fields=\"load\"")); |
There was a problem hiding this comment.
I think for actual subqueries, we'll get the FORK is not supported message, too, because subqueries use UnionAll which is a subclass of Fork.
| if (right instanceof Filter filter) { | ||
| right = filter.child(); | ||
| } | ||
| return right instanceof EsRelation relation && relation.indexMode() == IndexMode.LOOKUP; |
There was a problem hiding this comment.
nit: this is correct, but we may have more complex right branches in the near future.
A simpler approach is to just check the whole plan for EsRelation instances with IndexMode.LOOKUP, as these relations are used exclusively for indices we perform lookups from.
| assertThat(pukesf2.getName(), is("does_not_exist2")); | ||
| } | ||
| } | ||
| """), "Subqueries and views are not supported with unmapped_fields=\"load\""); |
There was a problem hiding this comment.
verificationFailure just checks that the error message is contained, not that other error messages aren't also triggered.
Looking at the actual failures while debugging this test, I get this:
Found 5 problems
line 1:34: FORK is not supported with unmapped_fields="load"
line 2:5: Subqueries and views are not supported with unmapped_fields="load"
line 5:5: Subqueries and views are not supported with unmapped_fields="load"
line 7:5: Subqueries and views are not supported with unmapped_fields="load"
line 9:7: LOOKUP JOIN is not supported with unmapped_fields="load"
The first failure does not apply here and is wrongly triggered, see my comment above.
| assumeTrue("Requires subquery in FROM command support", EsqlCapabilities.Cap.SUBQUERY_IN_FROM_COMMAND.isEnabled()); | ||
|
|
||
| // A single subquery without a main index is merged into the main query during analysis, | ||
| // so there is no Subquery node in the plan and no branching — this is allowed. |
| FROM test | ||
| | EVAL language_code = languages | ||
| | LOOKUP JOIN languages_lookup ON language_code | ||
| | FORK (WHERE emp_no > 1) (WHERE emp_no < 100) |
There was a problem hiding this comment.
The query is duplicated. Let's extract that into a String that is re-used.
It's correct that we assert both failures, though.
| FROM test | ||
| | INLINE STATS c = COUNT(*) BY emp_no | ||
| """)); | ||
| } |
There was a problem hiding this comment.
nit: asserting the plan structure generally makes sense if we already add a positive test. And/or leaving a comment TODO: golden tests so we pick this up later.
| public void testLoadModeAllowsInlineStats() { | ||
| analyzeStatement(setUnmappedLoad(""" | ||
| FROM test | ||
| | INLINE STATS c = COUNT(*) BY emp_no |
There was a problem hiding this comment.
not-so nit: we already have inline stats + nullify tests above, e.g. testInlineStats. The same is true for fork. Let's double check which of the positive tests with nullify already exist and remove them from this PR.
…forbid_unmapped_fields_load
…elocations * upstream/main: (72 commits) [Test] Randomly disable sequence numbers in CcrTimeSeriesDataStreamsIT (elastic#143930) Fix AsyncSearchIndexServiceTests.testCircuitBreaker failure (elastic#144058) Refine GenerativeIT some more, this time with accounting for some added (elastic#144220) ESQL: Physical Planning on the Lookup Node (elastic#143707) Mute org.elasticsearch.xpack.esql.CsvIT test {csv-spec:approximation.Approximate stats by with zero variance} elastic#144240 Trigger counter metrics in test for delta temporality measurements (elastic#144193) fix capabiltiy approximation_v3 (elastic#144230) [ci] Add PR pipeline for testing ipv6 and fix tests not working with ipv6 (elastic#140473) update (elastic#144095) Make from/to optional in TBUCKET when Kibana timestamp filter is present (elastic#144057) Extract reroute behavior from create-index request classes (elastic#144140) ESQL: Fix release build only failures (elastic#144122) ES|QL query approximation: move sample correction to data node (elastic#144005) Add indexing pressure tracking to OTLP endpoints (elastic#144009) Fix replica writes after _seq_no doc values are pruned (elastic#144180) allow tests to configure supportsLoadingConfig (elastic#144061) [ES|QL] Unmute testGiantTextFieldInSubqueryIntermediateResultsWithSort (elastic#144126) [ESQL][DOCS] Add CPS page (unpublished for moment) (elastic#144206) ESQL: Forbid "load" unmapped_fields for certain commands (elastic#144115) Add CCS Remote Views Detection (elastic#143384) ...
) Addresses elastic#142367. AI-assisted PR.
Addresses #142367. AI-assisted PR.