Value type hint as string #80838

not-napoleon · 2021-11-18T14:30:12Z

Resolves #53424

This stores user value type hints internally as strings, converting them to ValuesSourceTypes at config resolution time.

The goal of this change is to let numeric script values sources know if they are floating point or not. Previously, this was done by plumbing the ValueType deep into the ValuesSource. Now, instead, pass in a boolean, but that in turn requires the VST to know if it's floating point or not. Thus this split.

not-napoleon · 2021-11-19T15:45:41Z

...ternalClusterTest/java/org/elasticsearch/search/aggregations/bucket/terms/StringTermsIT.java

@@ -1389,16 +1388,20 @@ public void testScriptWithValueType() throws Exception {
            assertThat(terms.getBuckets().size(), equalTo(1));
        }

-        String invalidValueType = source.replaceAll("\"value_type\":\"n.*\"", "\"value_type\":\"foobar\"");
+        String invalidValueType = source.replaceAll("\"value_type\":\"n.*?\"", "\"value_type\":\"foobar\"");


Regexes are hard. This test has always produced invalid json here, but apparently in the old code path we failed on value type validation first. Stream parsing I guess.

not-napoleon · 2021-11-19T15:46:43Z

...ternalClusterTest/java/org/elasticsearch/search/aggregations/bucket/terms/StringTermsIT.java

-            XContentParseException ex = expectThrows(
-                XContentParseException.class,
+            SearchPhaseExecutionException ex = expectThrows(
+                SearchPhaseExecutionException.class,


We no longer catch invalid value types at parse time. We do still catch them before aggregation execution though (at ValuesSourceConfig#Resolve time).

imotov

The change look reasonable to me. Since it is pretty big, I might have missed something. So, it might make sense for @nik9000 to have another look as well.

imotov · 2022-01-05T18:51:58Z

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

    ) {
        this.aggregatorRegistry = copyMap(aggregatorRegistry);
        this.usageService = usageService;
+        // TODO: Make an immutable copy blah blah blah


Is this planned for a follow-up?

yeah, I skipped it for time when doing the initial draft, but I'll add that in before merging. Thanks for the reminder. Probably a few other small clean ups I need to do too (like fixing the merge conflicts...).

nik9000

I think there are two outwards facing changes.

Aggs can declare different support for long vs double. I wonder if we can drop isFloatingPoint in a follow up change and have aggs declare support for long and double separately instead of checking that.
Plugins can now declare new values source types. Aggs won't support them out of the box, but the plugin could declare those too. That's neat.

I think I found a NOCOMMIT that snuck in. I left a few small requests otherwise.

nik9000 · 2022-01-05T21:12:22Z

...ution/archives/integ-test-zip/src/test/java/org/elasticsearch/test/rest/NodeRestUsageIT.java

        assertDiff(beforeCombinedAggsUsage, afterCombinedAggsUsage, "terms", "date", 1L);
        assertDiff(beforeCombinedAggsUsage, afterCombinedAggsUsage, "terms", "keyword", 2L);
-        assertDiff(beforeCombinedAggsUsage, afterCombinedAggsUsage, "avg", "numeric", 3L);
+        assertDiff(beforeCombinedAggsUsage, afterCombinedAggsUsage, "avg", "long", 3L);


Does this one also want to have double here?

nik9000 · 2022-01-05T21:13:45Z

...tats/src/main/java/org/elasticsearch/search/aggregations/matrix/ArrayValuesSourceParser.java

        protected NumericValuesSourceParser(boolean formattable) {
-            super(formattable, CoreValuesSourceType.NUMERIC, ValueType.NUMERIC);
+            super(formattable, CoreValuesSourceType.DOUBLE, ValueType.NUMERIC);


It's probably best not to drop support for anything in a big refactoring PR. Maybe make sure we have a test that targets a long field?

nik9000 · 2022-01-05T21:18:10Z

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

@@ -59,6 +61,8 @@ public int hashCode() {
    public static class Builder {
        private final AggregationUsageService.Builder usageServiceBuilder;
        private Map<RegistryKey<?>, List<Map.Entry<ValuesSourceType, ?>>> aggregatorRegistry = new HashMap<>();
+        private Map<String, ValuesSourceType> valueTypeLookup = new HashMap<>();
+        private Set<ValuesSourceType> duplicateRegistrationCheck = new HashSet<>();


Maybe just registeredValuesSourceTypes. It isn't just for dupes.

nik9000 · 2022-01-05T21:19:56Z

test/framework/src/main/java/org/elasticsearch/search/aggregations/AggregatorTestCase.java

@@ -981,7 +981,8 @@ private void writeTestDoc(MappedFieldType fieldType, String fieldName, RandomInd
        Document doc = new Document();
        String json;

-        if (vst.equals(CoreValuesSourceType.NUMERIC)) {
+        // NOCOMMI: Break out long and double logic?


nik9000 · 2022-01-06T14:35:07Z

...test/java/org/elasticsearch/xpack/analytics/topmetrics/TopMetricsAggregatorMetricsTests.java

@@ -203,7 +207,8 @@ private ValuesSourceConfig toConfig(SortedNumericDocValues values) throws IOExce
        when(source.isFloatingPoint()).thenReturn(false);
        when(source.longValues(null)).thenReturn(values);
        if (randomBoolean()) {
-            return toConfig(source, CoreValuesSourceType.NUMERIC, randomWholeNumberDocValuesFormat(), true);
+            // TODO: should we have a case for CVST.LONG here?
+            return toConfig(source, CoreValuesSourceType.DOUBLE, randomWholeNumberDocValuesFormat(), true);


It's probably worth it now that they are separate types. But it's not a blocker for this at all.

elasticsearchmachine · 2022-08-16T13:43:06Z

Pinging @elastic/es-analytics-geo (Team:Analytics)

elasticsearchmachine · 2024-02-14T18:10:29Z

Pinging @elastic/es-analytical-engine (Team:Analytics)

not-napoleon · 2024-02-14T18:19:48Z

there's no path to merging this at this point.

not-napoleon added 2 commits November 18, 2021 08:35

VSAB now has string for hint

91cd7f8

Use strings for hints everywhere

d4a818a

elasticsearchmachine added the v8.1.0 label Nov 18, 2021

not-napoleon added 6 commits November 18, 2021 09:32

VSConfig#resolve takes a VST

f642826

Register Values Sources

669381f

Change remaining nocommits todos so tests will run

188cad9

spotless apply

c3d4bb6

Fix MatrixStats when hint is null

fb42a7c

not-napoleon mentioned this pull request Nov 18, 2021

Parse user type hints directly into values source types #80747

Closed

not-napoleon added 10 commits November 18, 2021 14:03

actually register the core types

33bf6ff

Register values sources for top metrics tests

d9a991f

More tests that need to register stuff

c26b1a8

Merge branch 'master' into ValueTypeHint-as-String

9f202a9

Clean up GeoShapeValuesSource

5811cff

some test fixes

d7ea0d5

fix usage tracking test

659e0aa

Throw on unknown value types

aca10ba

spotless apply

1f9fe33

Fix NPE

78fdad5

not-napoleon commented Nov 19, 2021

View reviewed changes

not-napoleon added 3 commits November 19, 2021 11:26

Fix another NPE

099f086

version output in composite sources correctly

f4fa34d

Merge branch 'master' into ValueTypeHint-as-String

62cdfdf

not-napoleon requested review from imotov and nik9000 January 4, 2022 21:32

imotov reviewed Jan 5, 2022

View reviewed changes

nik9000 requested changes Jan 6, 2022

View reviewed changes

mark-vieira removed the v8.1.0 label Feb 2, 2022

elasticsearchmachine added the needs:triage Requires assignment of a team area label label Jul 27, 2022

nik9000 removed the needs:triage Requires assignment of a team area label label Aug 2, 2022

elasticsearchmachine added the needs:triage Requires assignment of a team area label label Aug 2, 2022

nik9000 removed the needs:triage Requires assignment of a team area label label Aug 2, 2022

elasticsearchmachine added the needs:triage Requires assignment of a team area label label Aug 2, 2022

romseygeek added :Analytics/Aggregations Aggregations and removed needs:triage Requires assignment of a team area label labels Aug 16, 2022

elasticsearchmachine added the Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) label Aug 16, 2022

csoulios added v8.6.0 and removed v8.5.0 labels Sep 21, 2022

kingherc added v8.7.0 and removed v8.6.0 labels Nov 16, 2022

rjernst added v8.8.0 and removed v8.7.0 labels Feb 8, 2023

gmarouli added v8.9.0 and removed v8.8.0 labels Apr 26, 2023

pugnascotia added v8.10.0 and removed v8.9.0 labels Jun 22, 2023

quux00 added v8.11.0 and removed v8.10.0 labels Aug 16, 2023

mattc58 added v8.12.0 and removed v8.11.0 labels Oct 4, 2023

brianseeders added v8.13.0 and removed v8.12.0 labels Dec 6, 2023

elasticsearchmachine added v8.14.0 and removed v8.13.0 labels Feb 14, 2024

not-napoleon closed this Feb 14, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Value type hint as string #80838

Value type hint as string #80838

Uh oh!

not-napoleon commented Nov 18, 2021

Uh oh!

not-napoleon Nov 19, 2021

Uh oh!

not-napoleon Nov 19, 2021

Uh oh!

imotov left a comment

Uh oh!

imotov Jan 5, 2022

Uh oh!

not-napoleon Jan 5, 2022

Uh oh!

nik9000 left a comment

Uh oh!

nik9000 Jan 5, 2022

Uh oh!

nik9000 Jan 5, 2022

Uh oh!

nik9000 Jan 5, 2022

Uh oh!

nik9000 Jan 5, 2022

Uh oh!

nik9000 Jan 6, 2022

Uh oh!

elasticsearchmachine commented Aug 16, 2022

Uh oh!

elasticsearchmachine commented Feb 14, 2024

Uh oh!

not-napoleon commented Feb 14, 2024

Uh oh!

Uh oh!

Value type hint as string #80838

Value type hint as string #80838

Uh oh!

Conversation

not-napoleon commented Nov 18, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

imotov 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!

nik9000 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!

elasticsearchmachine commented Aug 16, 2022

Uh oh!

elasticsearchmachine commented Feb 14, 2024

Uh oh!

not-napoleon commented Feb 14, 2024

Uh oh!

Uh oh!