Add a cluster setting configuring `TDigestExecutionHint` #96943

kkrik-es · 2023-06-20T09:19:58Z

Provide a Setting for TDigestExecutionHint that's configured through ClusterSettings. Wire ClusterSettings through SearchExecutionContext and AggregationContext, to access the setting in agg builders that use percentiles. Add a helper factory method for SearchExecutionContext to simplify initialization in tests.

Any hint set directly inside the tdigest spec of a percentile query takes precedence over the one from ClusterSettings.

Related to #95903

More work needed for TDigestPercentile*Tests and the TDigestTest (and the rest of the tests) in the tdigest lib to pass.

Remove wrong asserts from tests and MergingDigest.

Remove redundant serializing interfaces from the library.

These tests don't address compatibility issues in mixed cluster tests as the latter contain a mix of older and newer nodes, so the output depends on which node is picked as a data node since the forked TDigest library is not backwards compatible (produces slightly different results).

# Conflicts: # libs/tdigest/src/main/java/org/elasticsearch/tdigest/SortingDigest.java # server/src/main/java/org/elasticsearch/TransportVersion.java # server/src/main/java/org/elasticsearch/index/query/SearchExecutionContext.java # server/src/main/java/org/elasticsearch/search/aggregations/metrics/AbstractTDigestPercentilesAggregator.java # server/src/main/java/org/elasticsearch/search/aggregations/metrics/MedianAbsoluteDeviationAggregationBuilder.java # server/src/main/java/org/elasticsearch/search/aggregations/metrics/MedianAbsoluteDeviationAggregator.java # server/src/main/java/org/elasticsearch/search/aggregations/metrics/MedianAbsoluteDeviationAggregatorFactory.java # server/src/main/java/org/elasticsearch/search/aggregations/metrics/PercentilesConfig.java # server/src/main/java/org/elasticsearch/search/aggregations/metrics/TDigestState.java # server/src/test/java/org/elasticsearch/search/aggregations/metrics/MedianAbsoluteDeviationAggregatorTests.java # server/src/test/java/org/elasticsearch/search/aggregations/metrics/MedianAbsoluteDeviationTests.java # server/src/test/java/org/elasticsearch/search/aggregations/metrics/TDigestExecutionHintTests.java # server/src/test/java/org/elasticsearch/search/aggregations/metrics/TDigestPercentileRanksAggregatorTests.java # server/src/test/java/org/elasticsearch/search/aggregations/metrics/TDigestPercentilesAggregatorTests.java # server/src/test/java/org/elasticsearch/search/aggregations/metrics/TDigestStateTests.java # x-pack/plugin/analytics/src/main/java/org/elasticsearch/xpack/analytics/aggregations/metrics/AbstractHistoBackedTDigestPercentilesAggregator.java # x-pack/plugin/analytics/src/main/java/org/elasticsearch/xpack/analytics/boxplot/BoxplotAggregationBuilder.java # x-pack/plugin/analytics/src/main/java/org/elasticsearch/xpack/analytics/boxplot/BoxplotAggregatorFactory.java # x-pack/plugin/analytics/src/test/java/org/elasticsearch/xpack/analytics/aggregations/metrics/TDigestPreAggregatedPercentileRanksAggregatorTests.java # x-pack/plugin/analytics/src/test/java/org/elasticsearch/xpack/analytics/aggregations/metrics/TDigestPreAggregatedPercentilesAggregatorTests.java # x-pack/plugin/analytics/src/test/java/org/elasticsearch/xpack/analytics/boxplot/BoxplotAggregationBuilderTests.java

elasticsearchmachine · 2023-06-20T09:20:46Z

Hi @kkrik-es, I've created a changelog YAML for you.

…o fix/95903-clustersettings

elasticsearchmachine · 2023-06-20T09:57:29Z

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

martijnvg

I think we should try to rely on the default defined in the new cluster setting and make execution hint an optional field in all the builders.

martijnvg · 2023-06-20T10:04:53Z

server/src/main/java/org/elasticsearch/search/aggregations/metrics/TDigestExecutionHint.java

@@ -21,6 +22,14 @@ public enum TDigestExecutionHint implements Writeable {
    DEFAULT(0),        // Use a TDigest that is optimized for performance, with a small penalty in accuracy.
    HIGH_ACCURACY(1);  // Use a TDigest that is optimize for accuracy, at the expense of performance.

+    public static final Setting<String> SETTING = Setting.simpleString(
+        "search.aggs.tdigest_execution_hint",
+        "",


maybe define the default here?

Done - empty string was already mapped to DEFAULT, but made it more explicit.

martijnvg · 2023-06-20T10:06:49Z

server/src/main/java/org/elasticsearch/search/aggregations/metrics/PercentilesConfig.java

@@ -165,7 +165,7 @@ public void parseExecutionHint(String executionHint) {
        }

        public TDigestExecutionHint getExecutionHint() {
-            return executionHint;
+            return executionHint == null ? TDigestExecutionHint.DEFAULT : executionHint;


I think we should allow executionHint to be null here so that we can rely on the default defined in the new cluster setting.

Right, this should only apply in cases where the executionHint hasn't been set through a call to a method that accesses the AggregationContext. In general, I think it's better to guarantee that this won't be used as null, otherwise we'd better use an Optional and modify all consumers accordingly. This does feel like overengineering, though - we have a default hint for this exact reason..

Discussed offline, fixed as suggested to cover for the case where the object is written without a hint but read when one is set in ClusterSettings.

martijnvg · 2023-06-20T10:07:45Z

server/src/main/java/org/elasticsearch/search/aggregations/metrics/PercentilesConfig.java

        }

        @Override
        public void writeTo(StreamOutput out) throws IOException {
            super.writeTo(out);
            out.writeDouble(compression);
            if (out.getTransportVersion().onOrAfter(TransportVersion.V_8_500_014)) {
-                executionHint.writeTo(out);
+                getExecutionHint().writeTo(out);


Maybe use writeOptionalWriteable(...) here? That way we can allow exection hint to be null.

This unfortunately means we do need to increase transport version.

What's the benefit of leaving this as null at this point? Do we expect mutations after an object gets serialized?

Used a writeBoolean explicitly to combine with TDigestExecutionHint.writeTo

martijnvg

I left a few more comments. I think it is almost ready.
Mainly around using: readOptionalWriteable() and writeOptionalWriteable().

...org/elasticsearch/search/aggregations/metrics/MedianAbsoluteDeviationAggregationBuilder.java

martijnvg · 2023-06-20T15:43:31Z

server/src/main/java/org/elasticsearch/search/aggregations/metrics/TDigestExecutionHint.java

@@ -21,6 +22,14 @@ public enum TDigestExecutionHint implements Writeable {
    DEFAULT(0),        // Use a TDigest that is optimized for performance, with a small penalty in accuracy.
    HIGH_ACCURACY(1);  // Use a TDigest that is optimize for accuracy, at the expense of performance.

+    public static final Setting<String> SETTING = Setting.simpleString(


I think Setting#enumSetting(...) should be used here?

That one is case sensitive - setting to HIGH_ACCURACY works but high_accuracy doesn't. That's why I think using ::parse is preferable here?

Right, I see this method always uses Enum.valueOf(...) which is case sensitive and only allows values as is defined in the enum itself. (and in ExecutionHint all enums are defined in uppercase).

Ok, let's keep it this way.

server/src/main/java/org/elasticsearch/search/aggregations/metrics/PercentilesConfig.java

martijnvg · 2023-06-20T15:49:17Z

server/src/main/java/org/elasticsearch/search/aggregations/metrics/PercentilesConfig.java

-            if (out.getTransportVersion().onOrAfter(TransportVersion.V_8_500_014)) {
-                executionHint.writeTo(out);
+            if (out.getTransportVersion().onOrAfter(TransportVersion.V_8_500_017)) {
+                if (executionHint != null) {


writeOptional?

martijnvg · 2023-06-20T15:50:52Z

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

@@ -650,6 +651,7 @@ public SearchExecutionContext newSearchExecutionContext(
            shardId,
            shardRequestIndex,
            indexSettings,
+            clusterService != null ? clusterService.getClusterSettings() : ClusterSettings.createBuiltInClusterSettings(),


Just curios, when is this null?

Couldn't spot any, I was just defensive here. Removed the check.

martijnvg · 2023-06-20T15:52:51Z

server/src/main/java/org/elasticsearch/index/query/SearchExecutionContext.java

+    /**
+     * Factory method generating a dummy context with partial functionality. Strictly test-only.
+     */
+    public static SearchExecutionContext newDummyForTests(


This is test code, but gets included in the release. Can we move it to test/framework/main/java ? Maybe as factory method in a class in org.elasticsearch.search package in the test framework?

...ytics/src/main/java/org/elasticsearch/xpack/analytics/boxplot/BoxplotAggregationBuilder.java

Move test-only SearchExecutionContext method in helper class under test.

# Conflicts: # server/src/main/java/org/elasticsearch/TransportVersion.java

martijnvg

LGTM

test/framework/src/main/java/org/elasticsearch/index/query/SearchExecutionContextHelper.java

martijnvg · 2023-06-20T17:27:23Z

server/src/main/java/org/elasticsearch/search/aggregations/metrics/TDigestExecutionHint.java

@@ -21,6 +22,14 @@ public enum TDigestExecutionHint implements Writeable {
    DEFAULT(0),        // Use a TDigest that is optimized for performance, with a small penalty in accuracy.
    HIGH_ACCURACY(1);  // Use a TDigest that is optimize for accuracy, at the expense of performance.

+    public static final Setting<String> SETTING = Setting.simpleString(


Right, I see this method always uses Enum.valueOf(...) which is case sensitive and only allows values as is defined in the enum itself. (and in ExecutionHint all enums are defined in uppercase).

Ok, let's keep it this way.

This was replaced by optional serialization in version V_8_500_018 (elastic#96943), breaking backwards compatibility for serverless. Related to elastic#95903

This was replaced by optional serialization in version V_8_500_018 (#96943), breaking backwards compatibility for serverless. Related to #95903

kkrik-es and others added 30 commits May 9, 2023 10:23

Initial import for TDigest forking.

3c6c424

Fix MedianTest.

d719767

More work needed for TDigestPercentile*Tests and the TDigestTest (and the rest of the tests) in the tdigest lib to pass.

Fix Dist.

e816b57

Fix AVLTreeDigest.quantile to match Dist for uniform centroids.

0079943

Update docs/changelog/96086.yaml

0a18e70

Fix MergingDigest.quantile to match Dist on uniform distribution.

96ea462

Add merging to TDigestState.hashCode and .equals.

56350ee

Remove wrong asserts from tests and MergingDigest.

Fix style violations for tdigest library.

d731aee

Fix typo.

18c1cd5

Fix more style violations.

f96b4d2

Fix more style violations.

6d0201b

Fix remaining style violations in tdigest library.

9d48856

Update results in docs based on the forked tdigest.

68e489f

Fix YAML tests in aggs module.

2b3b463

Fix YAML tests in x-pack/plugin.

0d6ea1e

Skip failing V7 compat tests in modules/aggregations.

98c1146

Fix TDigest library unittests.

d49ed67

Remove redundant serializing interfaces from the library.

Fix test failures in docs and mixed cluster.

5d9ae8e

Reduce buffer sizes in MergingDigest to avoid oom.

1ad14f8

Exclude more failing V7 compatibility tests.

af93c26

Update results for JdbcCsvSpecIT tests.

d82ec3e

Update results for JdbcDocCsvSpecIT tests.

c9ab354

Revert unrelated change.

c19de44

More test fixes.

9753468

Use version skips instead of blacklisting in mixed cluster tests.

a23ae32

Switch TDigestState back to AVLTreeDigest.

054cd90

Update docs and tests with AVLTreeDigest output.

2ccefde

Update flaky test.

3801358

Remove dead code, esp around tracking of incoming data.

52ae1ff

kkrik-es added 2 commits June 20, 2023 11:06

Init executionHint to null, set before using.

674694f

kkrik-es added >enhancement :Analytics/Aggregations Aggregations Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) labels Jun 20, 2023

elasticsearchmachine added the v8.9.0 label Jun 20, 2023

Update docs/changelog/96943.yaml

981a462

kkrik-es changed the title ~~Add cluster setting to SearchExecutionContext to configure TDigestExecutionHint~~ Add a cluster setting configuring TDigestExecutionHint Jun 20, 2023

kkrik-es added 2 commits June 20, 2023 12:55

Pass initialized executionHint to createEmptyPercentileRanksAggregator.

aff9044

Merge remote-tracking branch 'upstream/fix/95903-clustersettings' int…

60ecb28

…o fix/95903-clustersettings

kkrik-es requested a review from martijnvg June 20, 2023 09:57

kkrik-es marked this pull request as ready for review June 20, 2023 09:57

martijnvg reviewed Jun 20, 2023

View reviewed changes

kkrik-es added 4 commits June 20, 2023 14:07

Initialize TDigestExecutionHint.SETTING to "DEFAULT".

34a1266

Merge branch 'main' into fix/95903-clustersettings

12be72c

Merge branch 'main' into fix/95903-clustersettings

dc956f8

Initialize TDigestExecutionHint to null.

a3633be

martijnvg reviewed Jun 20, 2023

View reviewed changes

kkrik-es added 3 commits June 20, 2023 20:00

Use readOptionalWriteable/writeOptionalWriteable.

9689ab4

Move test-only SearchExecutionContext method in helper class under test.

Merge branch 'main' into fix/95903-clustersettings

f69ba70

# Conflicts: # server/src/main/java/org/elasticsearch/TransportVersion.java

Bump up TransportVersion.

b4827a7

martijnvg approved these changes Jun 20, 2023

View reviewed changes

Small fixes.

2cad8ba

kkrik-es merged commit 32bdd3b into elastic:main Jun 20, 2023

kkrik-es deleted the fix/95903-clustersettings branch June 20, 2023 20:22

kkrik-es mentioned this pull request Jun 20, 2023

Restore executionHint serialization from version V_8_500_014 #96965

Merged

kkrik-es added a commit that referenced this pull request Jun 20, 2023

Restore V_8_500_014 executionHint serialization. (#96965)

d6ff43b

This was replaced by optional serialization in version V_8_500_018 (#96943), breaking backwards compatibility for serverless. Related to #95903

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a cluster setting configuring `TDigestExecutionHint` #96943

Add a cluster setting configuring `TDigestExecutionHint` #96943

kkrik-es commented Jun 20, 2023 •

edited

Loading

elasticsearchmachine commented Jun 20, 2023

elasticsearchmachine commented Jun 20, 2023

martijnvg left a comment

martijnvg Jun 20, 2023

kkrik-es Jun 20, 2023

martijnvg Jun 20, 2023

kkrik-es Jun 20, 2023

kkrik-es Jun 20, 2023

martijnvg Jun 20, 2023

martijnvg Jun 20, 2023

kkrik-es Jun 20, 2023

kkrik-es Jun 20, 2023

martijnvg left a comment

martijnvg Jun 20, 2023

kkrik-es Jun 20, 2023

martijnvg Jun 20, 2023

martijnvg Jun 20, 2023

martijnvg Jun 20, 2023

kkrik-es Jun 20, 2023

martijnvg Jun 20, 2023

martijnvg left a comment

martijnvg Jun 20, 2023

Add a cluster setting configuring TDigestExecutionHint #96943

Add a cluster setting configuring TDigestExecutionHint #96943

Conversation

kkrik-es commented Jun 20, 2023 • edited Loading

elasticsearchmachine commented Jun 20, 2023

elasticsearchmachine commented Jun 20, 2023

martijnvg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martijnvg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martijnvg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Add a cluster setting configuring `TDigestExecutionHint` #96943

Add a cluster setting configuring `TDigestExecutionHint` #96943

kkrik-es commented Jun 20, 2023 •

edited

Loading