Phase 2: Infer grouping_missing_value_placeholder based on mapping_type

[mmarston]

This is 2nd in a series of new pull requests that are working toward a new approach to handling missing values in subaggregations as described in #882. For an outline of the steps see the comment from @myronmarston here. For context see the first PR in the sequence, #890

This replaces the Phase 2, mattmarston/elasticgraph#1 PR that was in my forked repo. This PR addresses the comments there.

Now the grouping_missing_value_placeholder method on ScalarType is inferred by the mapping type but can still be override the inferred placeholder value. Enums are handled in the GraphQL layer in the phase 3 PR. There is no runtime metadata or schema definition updates related to enums.

Other PRs

Phase 1: Add grouping_missing_value_placeholder runtime metadata (merged)
Phase 2: Infer grouping_missing_value_placeholder based on mapping_type (this PR)
Phase 3: Wire up grouping_missing_value_placeholder in GraphQL layer
Phase 4: Use grouping_missing_value_placeholder in aggregation logic

[myronmarston]

Looking good! Left some suggestions.

[myronmarston]

Two things here:

• How did you this out? Is it documented somewhere? (If so, can you link to those docs here?)
• It's worth noting that the presence of the JSON schema min/max do not guarantee that all indexed values are within the specified range. It guarantees that newly indexed values will be in that range, but it's possible that the JSON schema min/max were added after data started being indexed, and some values could already be in the index which are outside the specified range.
  • However, this is very much an edge case. My position is that if someone runs into this case, that's a time when they can override the grouping_missing_value_placeholder (maybe the the first concrete use case I've thought of for doing so).
  • In practice, I don't expect ElasticGraph users to be defining custom long types--EG already offers JsonSafeLong and LongString and most users are likely to use those, which already have and enforce the JSON schema min/max.
  • Regardless, maybe worth acknowledging via a comment that this can happen and that grouping_missing_value_placeholder can be forced to nil in that case? That way there's a record that we considered it.
  • ...I am curious what you think about this.

[mmarston]

How did you this out? Is it documented somewhere? (If so, can you link to those docs here?)

Unfortunately I'm not aware of any ES/OS documentation for the behavior. I know that using "NaN" works for integer types and I've tested that end-to-end. I have examples documented here in the phase 4 PR.

I'm now not certain about the bucket keys being coerced to floats, so it is possible that the range checks are unnecessary. However, I'd rather keep the range checks in for now to be safe. The alternative is that I completely remove missing value placeholder support for all integer mapping types (including long).

I first came to the inference that ints were getting coerced to floats when I was working on phase 4 and found that to get some tests to pass I needed to change some bucket keys to add .0, e.g. from 2022 to 2022.0. See this comment.

But when I've tested directly calling OpenSearch/ElasticSearch and when I've queried using GraphQL (in phase 4) I see integer output, not floats. So I'm not sure what is going on with the integration tests I noted above.

It's worth noting that the presence of the JSON schema min/max do not guarantee that all indexed values are within the specified range....
... maybe worth acknowledging via a comment that this can happen

I've noted this in the code comment.

[myronmarston]

Thanks, that's helpful context.

One suggestion: to help see what's happening, you can run tests with DEBUG_QUERY=1. When that ENV var is present it'll print the ES/OS request and response so you can more easily see what's going on.

[mmarston]

I've addressed all the feedback except I need to:

• add validation and tests around the data type of the placeholder
• document and provide examples that show that NaN works on integer types

[myronmarston]

Just about ready to merge--left a few more comments.

[myronmarston]

It's great to see this but it's not obvious to me if it'll necessarily work end-to-end since you've only been dealing with strings so far. Before releasing with this, I'd like the elasticgraph-graphql acceptance specs to include an example of using a numeric placeholder. This can be done in a future phase 5 PR:

• Add a PositiveInt custom scalar type in the example test schema, and define -1 as its grouping placeholder.
• Add a PositiveInt field to a type like Widget
• Update the aggregations acceptance specs to run an aggregations query which groups on the PositiveInt value. Verify the missing value comes through as expected.

[mmarston]

I haven't tested end-to-end with a number yet, but I have tested ES/OS queries directly using numeric values (see this comment on the phase 4 PR). That includes an example using -1. And while I didn't include it in the comment, I've confirmed that floating numbers (e.g. 3.5) also work, but for string fields (the bucket key comes back as "3.5") and int fields (bucket returned as a number, 3.5).

I'll leave this comment thread open to add the suggested testing in a future PR.

[myronmarston]

I'll leave this comment thread open to add the suggested testing in a future PR.

This repo is configured to block merges until every comment is marked as "resolved" just as a forcing function to not miss any, so we won't be able to merge this PR with this comment left open. You can track this work via a separate issue if you like.

[myronmarston]

👍 Looks great to me.