Handle views in ResolveIndexAction#143561
Conversation
|
Pinging @elastic/es-analytical-engine (Team:Analytics) |
jfreden
left a comment
There was a problem hiding this comment.
Thanks for working on a fix for this!
I think this should be handled earlier and more generally so other methods could pick this up too, ideally in the IndexNameExpressionResolver::ensureAliasOrIndexExists. Views should be hidden unless explicit index options to fetch them are used. Unfortunately there is only the IndicesOptions.wildcardOptions.resolveViews to check that now, there isn't one specifically for concrete view references.
|
Possibly we want to return views from |
| List<ResolvedDataStream> dataStreams | ||
| ) { | ||
| resolveIndices(names, indicesOptions, projectState, resolver, indices, aliases, dataStreams, Collections.emptySet()); | ||
| } |
There was a problem hiding this comment.
Inlined method used only in test. We should not have test only code in production
| throw infe; | ||
| } | ||
| } | ||
| if (indexAbstraction.getType() == Type.VIEW && context.getOptions().wildcardOptions().resolveViews() == false) { |
There was a problem hiding this comment.
This skips view when targeted directly by name.
Ideally we should not rely on a wildcard option for this.
For now I would like to merge fix as is to fix affected api.
Eventually (but before it is released) we should replace options.wildcardOptions.resolveViews with something related to concrete views, not just patterns.
cc @n1v0lg
There was a problem hiding this comment.
Something to note is that resolveAliases also only exists in the WildcardOptions, so if we add some new IndicesOptions subcategory it should probably be migrated there too. Unclear if it's worth the effort, but that's for a different investigation.
| if (names.length == 1 && (Metadata.ALL.equals(names[0]) || Regex.isMatchAllPattern(names[0]))) { | ||
| names = new String[] { "**" }; | ||
| } | ||
| assert indicesOptions.wildcardOptions().resolveViews() == false : "Views are not supported in ResolveIndexAction"; |
| index: myview | ||
| body: { query: { match_all: {} } } | ||
|
|
||
| - length: { hits.hits: 0 } |
There was a problem hiding this comment.
This change also causes a _search query to fail rather than pass with 0 results.
I think this is less surprising, but maybe @craigtaverner could confirm if this is desired.
There was a problem hiding this comment.
I think the change in behaviour makes sense.
craigtaverner
left a comment
There was a problem hiding this comment.
LGTM. I think we might want to add a test that gets the old error message (write to a view), but that could easily be another PR. It is missing scope from the original work, not really relevant to this PR.
| emptyIterable() | ||
| ); | ||
|
|
||
| // matched view is not returned, but it does not fail request |
There was a problem hiding this comment.
Should this comment move a few lines down and a new comment be here, since we are indeed failing the request when an explicit view name is used.
| index: myview | ||
| body: { query: { match_all: {} } } | ||
|
|
||
| - length: { hits.hits: 0 } |
There was a problem hiding this comment.
I think the change in behaviour makes sense.
|
|
||
| - do: | ||
| catch: /an ESQL view \[myview\] may not be the target of an index operation/ | ||
| catch: /no such index \[myview\]/ |
There was a problem hiding this comment.
I think the change in behaviour makes sense. But I also note that we should get the old error message if we write to write to a view, and there is no test for that. We could add such a test in a separate PR, or here.
|
Important Review skippedAuto reviews are limited based on label configuration. 🏷️ Required labels (at least one) (2)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). Comment |
…locations * upstream/main: (126 commits) Update KnnIndexTester to use more settings from datasets (elastic#143869) fix: dynamic template vector array is overridden by automatic dense_vector mapping (elastic#143733) ES|QL: Don't reuse the same alias for _fork column (elastic#143909) Close and initialize clients after each node upgrade in logsdb rolling upgrade tests. (elastic#143823) ESQL: Added GroupedTopNOperator for LIMIT BY, compute only (elastic#143476) Handle views in ResolveIndexAction (elastic#143561) Improve reindex rethrottle API in stateless (elastic#143771) Use a copy of the SearchExecutionContext for each Percolator execution (elastic#142765) Log the stacktrace when we encounter a deprecation warning for `default_metric` (elastic#143929) ESQL: evaluate ReferenceAttributes to potentially FieldAttributes for full-text functions restriction (elastic#143893) Add ClusterStateSerializationStats Serializatation Tests (elastic#142703) Adds Coordination Diagnostics Tests (elastic#142709) Upgrade Elasticsearch to Apache Lucene 10.4 (elastic#141882) ESQL: Add configurable bracket-based multi-value support for CSV reader (elastic#143890) time series es819 binary dv use up to a 1mb block size (elastic#143049) Dynamically enable / disable plugins in correspondence to stateless mode. (elastic#142147) ES|QL: Implement first/last_over_time for tdigest (elastic#143832) Document CHANGE_POINT limitation (elastic#143877) Fix OperationsOnSeqNoDisabledIndicesIT (elastic#143892) [Test] Test that sequence numbers are not pruned with retention lease (elastic#143825) ...
There has been several discussions on this topic since I opened #141050 and @idegtiarenko also mentioned this in #143561 (comment). The issue is that both `resolveViews` and `resolveAliases` are in `WildcardOptions` but are used when resolving both concrete index pattern targets and wildcard targets. This PR moves `resolveViews` from `IndicesOptions.WildcardOptions` into a new `IndicesOptions.IndexAbstractionOptions` record to address this confusing inconsistency. This is in preparation for sending `resolveViews` as a parameter to field caps for remote view resolving where more work will be done to serialize `resolveViews` and not having this in place would make for confusing code. See #143384 for more information on the upcoming serialization of `resolveViews`. **_Note_**: `resolveAliases` is moved in a follow up PR since it's decoupled from the resolveViews changes. #143953 **_Note_**: `IndicesRequest.includeDataStreams()` is the same type of flag as `resolveAliases` and `resolveViews`, it controls whether data streams participate in index resolution. It's a candidate for `IndexAbstractionOptions` but currently lives on `IndicesRequest` (not `IndicesOptions`) and is threaded separately through `IndexNameExpressionResolver.Context` as a constructor parameter. Moving it would touch 60+ files and change the `IndicesRequest` interface. Because of how big that change would be, I have not considered doing that in this PR (or at all).
This change ensures that
GET /_resolve/index/{pattern}is not failing if it matches one or several views