Phase 1: Add grouping_missing_value_placeholder runtime metadata

[mmarston]

This is phase 1 in a series of new pull requests that are working toward a new approach to handling missing values in subaggregations as described in #882. This PR is the first step outlined by @myronmarston here. It only includes adding grouping_missing_value_placeholder as an attribute of ScalarType and Enum::Type. For scalar types, the placeholder value can be specified when the type is defined. For enum types there is no need to make schema authors define a placeholder value. We use a placeholder value of **missing** that can't conflict with enum name (since GraphQL enums may only contain letters and underscores). If this value conflicts with a datastore_value then we fallback to not using a placeholder value.

Context

Due to limitations in OpenSearch and ElasticSearch, composite aggregations cannot be used as subaggregations with in a composite aggregation. When grouping by multiple fields in a subaggregation, ElasticGraph works around this limitation by creating a hierarchy of subaggregations. To handle missing values, each group by field results in a terms subaggregation and a missing subaggregation. When there are many fields to group by this results in an exponential explosion in the number of subaggregations. For some of our queries this is resulting in poor performance, or worse, causing queries to fail because they exceed max clause count on the OpenSearch cluster.

The new approach still relies on a hierarchy of subaggregations, but at each level of the hierarchy it only uses one child subaggregation instead of two, so the number of subaggregations is linear with respect to the number of group by fields, instead of being exponential.

This approach relies on providing a the terms aggregations a missing value to use as a placeholder. This isn't ideal but all workarounds involve a balance of tradeoffs.

Other PRs

Phase 2: Infer grouping_missing_value_placeholder based on mapping_type
Phase 3: Wire up grouping_missing_value_placeholder in GraphQL layer
Phase 4: Use grouping_missing_value_placeholder in aggregation logic

[CLAassistant]

All committers have signed the CLA.

[myronmarston]

This is coming along nicely! Left some feedback.

[mmarston]

@myronmarston I'm asking a couple high level questions here to make it easier to follow rather than spread out through all the stacked PRs

1. Do we want the placeholder value persisted in the runtime metadata artifacts?
   • it feels a bit redundant including the same placeholder value over and over again but there isn't really any harm. A slight alternative is to use an enum that defines which placeholder constant to use. The enum could have 3 values corresponding to string placeholder, numeric placeholder, and no placeholder (or that could be implied by nil).
   • if the placeholder is only used when it is in the generated artifacts, then metadata artifacts have to be generated after upgrading before seeing any benefit. That is probably a good thing (and based on other comments, sounds like it may actually always be required in order for the artifacts to match the server version).
   • Is it worth including any runtime metadata related to enums, given that the missing value can never conflict (as noted in this comment.
2. Should the schema author have a way to specify the placeholder value?
   • I'd only think that is necessary if we're concerned about the risk of collision with our default placeholders.
   • If we don't offer the option to specify the value in the schema definition, I think we should at least offer a way to opt out.
   • Perhaps opt-out is better as a higher level option (e.g. in datastore config) rather than needing to opt out every field (if that is what the user wants).

[myronmarston]

1. Do we want the placeholder value persisted in the runtime metadata artifacts?

My instinct is "yes". One of the core ideas of ElasticGraph is to "shift" as much computation as possible to schema artifact generation time so that the GraphQL engine can be as dumb and agnostic as possible, often by reading and using runtime metadata values. Storing the placeholder value in runtime metadata artifacts aligns with that. At GraphQL query time we're dealing with GraphQL types and having the desired placeholder value available on the GraphQL type by reading it from runtime metadata feels very straightforward and aligned with ElasticGraph's existing patterns.

• it feels a bit redundant including the same placeholder value over and over again but there isn't really any harm. A slight alternative is to use an enum that defines which placeholder constant to use. The enum could have 3 values corresponding to string placeholder, numeric placeholder, and no placeholder (or that could be implied by nil).

It feels simplest to me to just have the placeholder be a direct field in the runtime metadata, rather than it specifying an enum value which has to be dereferenced to get the actual placeholder value. Plus it's more flexible--it enables the placeholder to be anything. While we don't have any plans to use values beyond the 3 you mentioned, in my book it's a win-win when the simplest solution affords the most flexibility.

I'm also not concerned about the duplication/redundancy of the same value showing up multiple times in runtime_metadata.yaml. There's not any maintenance cost associated with that that I can think of.

• if the placeholder is only used when it is in the generated artifacts, then metadata artifacts have to be generated after upgrading before seeing any benefit. That is probably a good thing (and based on other comments, sounds like it may actually always be required in order for the artifacts to match the server version).

Yep, the expectation/requirement is that all schema artifacts are fully up-to-date. Besides the "were these artifacts dumped by the same version of ElasticGraph?" check I mentioned before, there's also a standard be rake check task that's designed to be used in CI which fails if any artifacts are out-of-date. When used in a CI build that is required to pass before merging, it guarantees that schema artifacts (including runtime metadata) are always up to date.

• Is it worth including any runtime metadata related to enums, given that the missing value can never conflict (as noted in this comment.

Now that we've realized a conflict is impossible for enums, I don't think it's worth including it in the runtime metadata. Instead, the MISSING_STRING_PLACEHOLDER constant can be defined centrally and then that constant can be used in elasticgraph-graphql when exposing grouping_missing_value_placeholder from ElasticGraph::GraphQL::Schema::Type when it's an enum type.

2. Should the schema author have a way to specify the placeholder value?

I don't think we have to offer a way to control this (I can't really think of a reason a schema author would need to) but if it's easy/low-effort to enable this customization with the path we're taking, then I think it's great to offer the option--maybe there's a use case we haven't thought of.

• If we don't offer the option to specify the value in the schema definition, I think we should at least offer a way to opt out.
• Perhaps opt-out is better as a higher level option (e.g. in datastore config) rather than needing to opt out every field (if that is what the user wants).

I think the runtime metadata system we're talking about offers a pretty easy way to make this customizable, where opting out on a type is as easy as setting it to nil. Here's a sketch for how that could work:

diff --git a/elasticgraph-schema_definition/lib/elastic_graph/schema_definition/schema_elements/scalar_type.rb b/elasticgraph-schema_definition/lib/elastic_graph/schema_definition/schema_elements/scalar_type.rb
index 1769059..4eb44b2 100644
--- a/elasticgraph-schema_definition/lib/elastic_graph/schema_definition/schema_elements/scalar_type.rb
+++ b/elasticgraph-schema_definition/lib/elastic_graph/schema_definition/schema_elements/scalar_type.rb
@@ -44,6 +44,7 @@ module ElasticGraph
       class ScalarType < Struct.new(
         :schema_def_state,
         :type_ref,
+        :grouping_missing_value_placeholder_overridden,
         :mapping_type,
         :runtime_metadata,
         :aggregated_values_customizations,
@@ -66,12 +67,13 @@ module ElasticGraph
 
         # @private
         def initialize(schema_def_state, name)
-          super(schema_def_state, schema_def_state.type_ref(name).to_final_form)
+          super(schema_def_state, schema_def_state.type_ref(name).to_final_form, false)
 
           # Default the runtime metadata before yielding, so it can be overridden as needed.
           self.runtime_metadata = SchemaArtifacts::RuntimeMetadata::ScalarType.new(
             coercion_adapter_ref: SchemaArtifacts::RuntimeMetadata::ScalarType::DEFAULT_COERCION_ADAPTER_REF,
-            indexing_preparer_ref: SchemaArtifacts::RuntimeMetadata::ScalarType::DEFAULT_INDEXING_PREPARER_REF
+            indexing_preparer_ref: SchemaArtifacts::RuntimeMetadata::ScalarType::DEFAULT_INDEXING_PREPARER_REF,
+            grouping_missing_value_placeholder: nil
           )
 
           yield self
@@ -91,12 +93,26 @@ module ElasticGraph
           type_ref.name
         end
 
+        def grouping_missing_value_placeholder(placeholder)
+          self.grouping_missing_value_placeholder_overridden = true
+          self.runtime_metadata = runtime_metadata.with(grouping_missing_value_placeholder: placeholder)
+        end
+
         # (see Mixins::HasTypeInfo#mapping)
         def mapping(**options)
           self.mapping_type = options.fetch(:type) do
             raise Errors::SchemaError, "Must specify a mapping `type:` on custom scalars but was missing on the `#{name}` type."
           end
 
+          unless grouping_missing_value_placeholder_overridden
+            case mapping_type
+            when *NUMERIC_TYPES
+              self.runtime_metadata = runtime_metadata.with(grouping_missing_value_placeholder: MISSING_NUMERIC_PLACEHOLDER)
+            when *STRING_TYPES
+              self.runtime_metadata = runtime_metadata.with(grouping_missing_value_placeholder: MISSING_STRING_PLACEHOLDER)
+            end
+          end
+
           super
         end
 
@@ -313,6 +329,7 @@ module ElasticGraph
         # https://www.elastic.co/guide/en/elasticsearch/reference/current/mapping-types.html
         # https://www.elastic.co/guide/en/elasticsearch/reference/7.13/number.html#number
         NUMERIC_TYPES = %w[long integer short byte double float half_float scaled_float unsigned_long].to_set
+        STRING_TYPES = %w[keyword text]
         DATE_TYPES = %w[date date_nanos].to_set
         # The Elasticsearch/OpenSearch docs do not exhaustively give a list of types on which range queries are efficient,
         # but the docs are clear that it is efficient on numeric and date types, and is inefficient on string

Essentially, expose a grouping_missing_value_placeholder API from schema.scalar_type that lets users customize the placeholder value or set it to nil to opt-out. We can still infer the placeholder value when the mapping type is configured, being careful to not override what a user has overridden.

That approach has the properties I think we want:

• Defaults to doing what we think is the best thing for all scalar types
• Offers the ability for a user to opt out and/or customize the value
• Relatively simple implementation
• Aligns with existing ElasticGraph patterns

How does that plan sound?

[mmarston]

Thanks for responding to my latest questions. They aligned with the direction I was already thinking but I wanted to put a few options out there.

[mmarston]

I've updated phase 1 to address all comments.

[myronmarston]

Almost ready to merge!

[myronmarston]

LGTM!