Add source fallback for keyword fields using operation #88735

jdconrad · 2022-07-22T15:56:12Z

This is a follow up PR to (#88040) to contrast a parameter passed to fielddataBuilder versus separate methods of fielddataBuilder and scriptFielddataBuilder. The parameter is an enum named MappedFieldType.FielddataOperation where the operation specifies what data structures should be used to generate fielddata. Right now it only has SEARCH and SCRIPT, but in the future this could extended to AGGS, SORT, etc. to allow fine-grained tuning where doc values are pulled from.

Note that something like DOC_VALUES_ONLY or SOURCE_FALLBACK doesn't work for scripting here because with exceptions like text field scripting can't know if it wants doc values primarily or source primarily so it has to be determined via the MappedType per operation.

jdconrad · 2022-07-27T16:37:55Z

@romseygeek Would you please take another pass when you have the chance? Thank you!

romseygeek

This looks great! I left a few comments but I think we're nearly there.

romseygeek · 2022-07-28T09:21:06Z

server/src/main/java/org/elasticsearch/index/IndexService.java

@@ -206,7 +206,10 @@ public IndexService(
                this.indexSortSupplier = () -> indexSettings.getIndexSortConfig()
                    .buildIndexSort(
                        mapperService::fieldType,
-                        (fieldType, searchLookup) -> indexFieldData.getForField(fieldType, FieldDataContext.noRuntimeFields("index sort"))
+                        (fieldType, searchLookup, fielddataOperation) -> indexFieldData.getForField(


I don't think we need to pass this extra fielddataOperation parameter here?

Fixed. Also, for my understanding, is this because we never want sort to fallback so there's no reason to build a different operation type into an index sort?

Yes, exactly

romseygeek · 2022-07-28T09:22:04Z

server/src/main/java/org/elasticsearch/index/fielddata/FieldDataContext.java

@@ -30,7 +40,9 @@ public record FieldDataContext(String fullyQualifiedIndexName, Supplier<SearchLo
    public static FieldDataContext noRuntimeFields(String reason) {
        return new FieldDataContext(
            "",
-            () -> { throw new UnsupportedOperationException("Runtime fields not supported for [" + reason + "]"); }
+            () -> { throw new UnsupportedOperationException("Runtime fields not supported for [" + reason + "]"); },
+            null,


It might be safer to have a default sourcePathsLookup of s -> Set.of(s) here?

Yes! Fixed.

romseygeek · 2022-07-28T09:48:05Z

server/src/test/java/org/elasticsearch/index/mapper/AbstractScriptFieldTypeTestCase.java

-    protected static FieldDataContext mockFielddataContext() {
-        return new FieldDataContext("test", mockContext()::lookup);
+    protected static FieldDataContext mockFielddataContext(MappedFieldType.FielddataOperation fielddataOperation) {
+        return new FieldDataContext("test", mockContext()::lookup, mockContext()::sourcePath, fielddataOperation);
    }


Given that these are script tests, I think we can just always use FielddataOperation.SCRIPT here?

This is an interesting case for runtime fields. I don't think it's actually possible to hit source fallback for a runtime field at all, but they will likely be executed at a top level as a search operation, so I think for these tests it would make the most sense to switch it to SEARCH rather than SCRIPT.

I don't think it's actually possible to hit source fallback for a runtime field at all

I'm confused, runtime fields explicitly use scripts so I think they're quite likely to use this path? Or am I misunderstanding what's going on here?

My understanding is when a runtime field is first executed as a script its part of a search query where the runtime field looks like a standard field. At that first execution the runtime field should always be found as the doc values portion of a field that it's emulating, right? So I don't think the operation actually matters for that "top-level" runtime field. However, for fields referenced from that runtime field then the operation will be SCRIPT when we do the lookup from LeafDocLookup.

For the tests you're right, this should be SCRIPT. I'm thinking about this incorrectly.

jdconrad · 2022-07-28T15:50:57Z

@romseygeek Thank you for the continued review iterations. This is ready for another pass.

romseygeek

LGTM!

jdconrad · 2022-07-28T16:28:16Z

@romseygeek Thank you! Will commit as soon as CI passes.

elasticsearchmachine · 2022-07-28T16:45:07Z

Hi @jdconrad, I've created a changelog YAML for you.

* upstream/main: Add 8.5 migration docs (elastic#88923) Script: Reindex & UpdateByQuery Metadata (elastic#88665) Remove unused plugins dir var from server CLI (elastic#88917) Use tracing API in TaskManager (elastic#88885) Add source fallback for keyword fields using operation (elastic#88735) Prune changelogs after 8.3.3 release Bump versions after 8.3.3 release Add a test for checking for misspelled "dry_run" parameters for Desired Nodes API (elastic#88898) Speedup BalanceUnbalancedClusterTests (elastic#88794) Preventing exceptions on node shutdown in integration tests (elastic#88827) Do not trigger check part3 for test mute and docs PRs (elastic#88895) Add troubleshooting docs about data corruption (elastic#88760) Mute RollupActionSingleNodeTests#testRollupDatastream (elastic#88891) [DOCS] Domain splitting impacts API keys (elastic#88677) Fix SqlSearchIT testAllTypesWithRequestToOldNodes (elastic#88866) (elastic#88883) Update synthetic-source.asciidoc (elastic#88880) Log more details in TaskAssertions (elastic#88864) Make Tuple a record (elastic#88280)

Add source fallback operation when looking up a the factor field added in elastic#88735 Resolves elastic#88985

Add source fallback operation when looking up a the factor field added in #88735 Resolves #88985

javanna

I had a quick look as well, this change seemed important enough for me to look at even if it's already merged. I left some questions. An additional question I have is around the scope: the title mentions keyword fields, but I was under the impression that the fallback has been added also for numbers and geo_points?

javanna · 2022-08-11T12:21:43Z

server/src/main/java/org/elasticsearch/index/mapper/MappedFieldType.java

+     */
+    public enum FielddataOperation {
+        SEARCH,
+        SCRIPT


Should we add docs to clarify how these two options differ from each other?

javanna · 2022-08-11T12:23:47Z

server/src/main/java/org/elasticsearch/index/mapper/GeoPointFieldMapper.java

+                failIfNoDocValues();
+            }
+
+            if ((operation == FielddataOperation.SEARCH || operation == FielddataOperation.SCRIPT) && hasDocValues()) {


is the first part of the if necessary? It's maybe a protection mechanism for potential new operations that may be added in the future? Isn't it more practical to just say if hasDocValues() load from docvalues?

javanna · 2022-08-11T12:24:21Z

server/src/main/java/org/elasticsearch/index/mapper/KeywordFieldMapper.java

+                failIfNoDocValues();
+            }
+
+            if ((operation == FielddataOperation.SEARCH || operation == FielddataOperation.SCRIPT) && hasDocValues()) {


same comment as above

javanna · 2022-08-11T12:26:09Z

server/src/main/java/org/elasticsearch/index/mapper/NumberFieldMapper.java

+                failIfNoDocValues();
+            }
+
+            if ((operation == FielddataOperation.SEARCH || operation == FielddataOperation.SCRIPT) && hasDocValues()) {


here too, same as above

javanna · 2022-08-11T12:27:57Z

server/src/main/java/org/elasticsearch/search/aggregations/support/AggregationContext.java

@@ -435,7 +435,7 @@ public Analyzer buildCustomAnalyzer(

        @Override
        protected IndexFieldData<?> buildFieldData(MappedFieldType ft) {
-            return context.getForField(ft);
+            return context.getForField(ft, MappedFieldType.FielddataOperation.SEARCH);


given all these related changes, too late now but have we considered having a separate getForField only for scripting and leaving the existing one alone, so that less changes are required in existing code?

jdconrad added 27 commits June 24, 2022 15:58

Update SearchLookup to include source paths for each field.

9227535

Add source fallback for keyword field.

ad63384

Update some tests for keyword source fallback.

d110c7d

separate script source fallback into its own method

892b27a

Update docs/changelog/88040.yaml

30fc69b

Merge branch 'master' into sourcefallback

4c522c4

update change log

27f7d80

Merge branch 'master' into sourcefallback

47e8955

Merge branch 'master' into sourcefallback

d105a93

Delete docs/changelog/88040.yaml

60d4d0e

Merge branch 'master' into sourcefallback

661d2c1

Merge branch 'master' into sourcefallback

1d0ffd1

fix tests with master merge

39cde22

add keyword source fallback for scripting using value fetcher

0693ac0

replace tuple return with marker interface

9062de7

Merge branch 'master' into sourcefallback

c56371e

Merge branch 'master' into sourcefallback

6a3aa33

Merge branch 'master' into sourcefallback

6a03351

Split off value fetcher index field data into base classes.

170b9af

Add integer field to source fallback w/ value fetcher strategy.

50beaea

Add script source fallback for geopoint field.

c91f730

Merge branch 'master' into sourcefallback

a8854c3

Merge branch 'master' into sourcefallback

38df037

add fielddatatype

32d8ace

Fix package

f039525

Merge branch 'master' into sfoptions

e5de7f3

change name from type to operation

e8f813a

jdconrad added >enhancement :Search Foundations/Mapping Index mappings, including merging and defining field types Team:Core/Infra Meta label for core/infra team labels Jul 22, 2022

jdconrad added 2 commits July 26, 2022 12:21

Merge branch 'main' into sfoptions

266b231

Merge branch 'main' into sfoptions

69cd948

mark-vieira added v8.5.0 and removed v8.4.0 labels Jul 27, 2022

jdconrad added 2 commits July 27, 2022 09:08

Merge branch 'main' into sfoptions

5c11166

add sourceValueFetcher methods for keyword and number field types

f341afa

jdconrad added 2 commits July 27, 2022 11:22

Merge branch 'main' into sfoptions

4a6efd9

fix bad merge

b5258ad

romseygeek reviewed Jul 28, 2022

View reviewed changes

jdconrad added 2 commits July 28, 2022 07:49

Merge branch 'main' into sfoptions

d5d83d7

response to pr comments

8055416

fix tests

5b41bb2

romseygeek approved these changes Jul 28, 2022

View reviewed changes

Merge branch 'main' into sfoptions

758e2c9

jdconrad removed the WIP label Jul 28, 2022

elasticsearchmachine removed Team:Core/Infra Meta label for core/infra team Team:Clients Meta label for clients team labels Jul 28, 2022

Update docs/changelog/88735.yaml

2dd57a7

jdconrad merged commit 5e0701f into elastic:main Jul 28, 2022

jdconrad mentioned this pull request Jul 28, 2022

Improve support of source fallback #88913

Closed

hendrikmuhs mentioned this pull request Aug 1, 2022

Example plugin test compile error #88985

Closed

arteam added a commit to arteam/elasticsearch that referenced this pull request Aug 1, 2022

Fix compilation in the rescore plugin

d3ed689

Add source fallback operation when looking up a the factor field added in elastic#88735 Resolves elastic#88985

arteam mentioned this pull request Aug 1, 2022

Fix compilation in the rescore example plugin #89004

Merged

arteam added a commit that referenced this pull request Aug 1, 2022

Fix compilation in the rescore plugin (#89004)

2a03ac3

Add source fallback operation when looking up a the factor field added in #88735 Resolves #88985

javanna reviewed Aug 11, 2022

View reviewed changes

Add source fallback for keyword fields using operation #88735

Add source fallback for keyword fields using operation #88735

Uh oh!

Conversation

jdconrad commented Jul 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jdconrad commented Jul 27, 2022

Uh oh!

romseygeek left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jdconrad commented Jul 28, 2022

Uh oh!

romseygeek left a comment

Choose a reason for hiding this comment

Uh oh!

jdconrad commented Jul 28, 2022

Uh oh!

elasticsearchmachine commented Jul 28, 2022

Uh oh!

javanna left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jdconrad commented Jul 22, 2022 •

edited

Loading