Alternative missing value approach to subaggregations

[mmarston]

Update: See #890

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.

This is a draft of an approach that 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.

I've run the changes locally but haven't yet updated any of the tests.

[bsorbo]

I'd sooner maybe expect a collision with NaN than maybe some random 64-bit number high up... but ES does support shorts/bytes/half_floats etc. Potentially we could make this PR string-only and probably still get 80% of the value?

[mmarston]

I agree with the general approach of only taking this approach for data types that have high confidence of avoiding a chance of collision.

For numeric types, the more I look into it, the more confident I am that we can use NaN without collision. JSON doesn't have a way to serialized NaN. And the top Google results say that NaN can't be stored in ElasticSearch. I tested trying to store NaN in a double field in OpenSearch and when I tried "quantityFloat": NaN then OpenSearch returns a failed to parse error with reason Non-standard token 'NaN'. And when I tried "quantityFloat": "NaN" it failed with reason Double value passed as String.

Even if someone solved how to send NaN in JSON and how to store NaN in ElasticSearch or OpenSearch, we could confidently use NaN for ElasticGraph data types Int, JsonSafeLong, and LongString because ElasticGraph would reject NaN when validating those data types.

One risk with NaN is that ElasticSearch or OpenSearch could change to no longer support "NaN" for missing value aggregation. But that would be a breaking change, so seems unlikely that they would do so.

Another risk is that ES/OS could eventually support storing NaN, which would then open up the possibility of collision.

[CLAassistant]

All committers have signed the CLA.

[myronmarston]

I'm a fan of this general direction but I want to be careful with how we do this--we need to be really sure that the placeholder value is safe to use for the field's type (and conflicts are either impossible or so vanishingly unlikely that we consider it effectively impossible). If we're not sure I'd rather continue using the missing subaggregation rather than risk subtle bugs.

To that end, I've left an inline suggestion for an approach that I think will enable this.

When it comes time for the final PR that actually leverages my suggestion to implement this, I'd love to see some before/after examples of Elasticsearch queries so we can see what concrete difference it makes in them.

[myronmarston]

I'm concerned about the approach here: this isn't a complete list of scalar types (for example, Boolean, DateTime, Date, "LongString", etc). Plus there could be user-defined custom scalar types. There's no way for this bit of code to know about all scalar types (nor should it).

I'd prefer that we "invert" this, where scalar types themselves would define what their missing value placeholder is, and this would then just use the type's missing value, if there is one. Here's a sketch of how that could work:

In elasticgraph-schema_definition, we'd update scalar_type to allow missing value placeholders to be defined on them:

schema.scalar_type "JsonSafeLong" do |t|
  # ...
  t.grouping_missing_value_placeholder "NaN"
end

In elasticgraph-schema_artifacts, the ScalarType runtime metadata class would have a grouping_missing_value_placeholder attribute. The scalar type definition in elasticgraph-schema_definition would record the grouping_missing_value_placeholder on runtime metadata when dumping schema artifacts.

In elasticgraph-graphql, we'd wire up the runtime metadata ScalarType to make it available to the ElasticGraph::GraphQL::Schema::Type (you'd probably have to pass it through a couple layers).

Finally, you could read the grouping_missing_value_placeholder value here and return it. Scalar types for which we've defined the grouping_missing_value_placeholder would get this new treatment, while types for which no grouping_missing_value_placeholder has been defined would get the old treatment (a missing subaggregation).

Note: if we go this route, I'd ask that you break it up into multiple PRs as that's going to be way too big to easily review. As a suggested breakdown:

• A PR to add grouping_missing_value_placeholder as an attribute to the elasticgraph-schema_artifacts runtime metadata class. (There are some tests, etc that you'll need to update as part of this PR, so it won't be a one-liner!).
• A PR to update the scalar_type API offered by elasticgraph-schema_definition so that it offers grouping_missing_value_placeholder and records it in the runtime metadata.
  • As part of this PR, you could update the scalar_type definition of all the built-in scalar types to define the grouping_missing_value_placeholder for each.
• A PR to update elasticgraph-graphql to make grouping_missing_value_placeholder available from ElasticGraph::GraphQL::Schema::Type by wiring it up.
• A final PR to use grouping_missing_value_placeholder in the grouping logic as you've done here.

(If some of these PRs seem really small, feel free to combine them.)

[mmarston]

I'd love to see some before/after examples of Elasticsearch queries

Yes, I intend to.

I'd prefer that we "invert" this, where scalar types themselves would define what their missing value placeholder is

Yes, I see that as a better approach. Regardless of how/where we define the missing value placeholder, we'll continue to support data types for which we haven't defined a missing value placeholder.

Before doing all the work to create 4 PRs as you've outlined it, I want to have confidence that making these changes will alleviate the performance/availability issues we're experiencing due to the complex generated queries.

[myronmarston]

Suggested change

	clause_value["missing"] = missing_value_placeholder if handles_missing_values?
	clause_value = clause_value.merge({"missing" => missing_value_placeholder}) if handles_missing_values?

As a general rule, we favor a coding style that avoids mutating objects unless strictly necessary. Re-assigning a local variable to a different object is fine though--local variables, by their nature, are localized.

[mmarston]

I'll watch out for that. There are probably a couple places in this PR that mutate instead of re-assign.

[mmarston]

See #890