Support "_first" and "_last" ordering of missing values in composite aggs by Luegg · Pull Request #76740 · elastic/elasticsearch

Luegg · 2021-08-20T07:40:30Z

Extends the composite value source builders with a new missing_order parameter that additionally allows to always put missing buckets "first" or "last" independently of the sort order.

Example query:

{
  "size": 0,
  "_source": false,
  "aggregations": {
    "groupby": {
      "composite": {
        "size": 1000,
        "sources": [
          {
            "gen": {
              "terms": {
                "field": "gender",
                "missing_bucket": true,
                "missing_order": "first",
                "order": "asc"
              }
            }
          },
          {
            "lang": {
              "terms": {
                "field": "languages",
                "missing_bucket": true,
                "missing_order": "last",
                "order": "asc"
              }
            }
          }
        ]
      }
    }
  }
}

If missing_bucket is false or missing, the missing_order parameter will be ignored.

elasticmachine · 2021-08-31T08:49:21Z

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

nik9000

I think it's right though I'd do the BWC/backport differently.

@ywelsch and @not-napoleon have both recently looked more closely at this code than I have and might want to have a look too.

nik9000 · 2021-08-31T18:21:48Z

...ava/org/elasticsearch/search/aggregations/bucket/composite/CompositeValuesSourceBuilder.java

            this.userValueTypeHint = ValueType.readFromStream(in);
        }
-        this.missingBucket = in.readBoolean();
+        if (in.getVersion().onOrAfter(Version.V_7_16_0)) {


I tend to set this to 8_0_0 when I introduce these changes so that CI runs the bwc tests before I merge. I'll prepare the backport PR and a separate PR to disable bwc. Once both pass i merged the disabling first, wait a bit, and merge the backport. Then merge a final thing to master to undo it. It's a bunch of work but it keeps the BWC tests running as long as possible.

👍 The fact that all the bwc tests passed despite that I've been using 7.16 here also made me think that maybe I'm missing some tests. So I added some more rest tests and adjusted the versions.

elasticsearchmachine · 2021-09-13T12:23:39Z

💔 Backport failed

Status	Branch	Result
❌	7.x	Commit could not be cherrypicked due to conflicts

You can use sqren/backport to manually backport by running backport --upstream elastic/elasticsearch --pr 76740

…aggs (elastic#76740) * Support "first" and "last" ordering of missing values in composite aggs * skip API compat tests for 7.16 * use proper versions for bwc * use "missing_order" parameter instead of "missing" * address review comments # Conflicts: # server/src/main/java/org/elasticsearch/search/aggregations/bucket/composite/CompositeValuesSourceBuilder.java

…osite aggs (#76740) (#77620) * Support "_first" and "_last" ordering of missing values in composite aggs (#76740) * Support "first" and "last" ordering of missing values in composite aggs * skip API compat tests for 7.16 * use proper versions for bwc * use "missing_order" parameter instead of "missing" * address review comments # Conflicts: # server/src/main/java/org/elasticsearch/search/aggregations/bucket/composite/CompositeValuesSourceBuilder.java * adjust versions for BWC

They have started failing about 0.3% of the time each. And there are a couple dozen of them so yay. Relates to #77650 and probably #76740

Follow up of #77650. Looks like two issues caused occasional failures: missingOrder has not been considered in InternalComposite.InternalBucket#compareKey reverseMul has been ignored in one case in OrdinalValuesSource#compareInternal The first problem has clearly been introduced by #76740 but the second point is more interesting. As far as I understand, this might have caused problems before but I couldn't find any according test failures in Gradle before yesterday. In order to preclude further rare test failures I've run the whole CompositeAggregatorTests 3000 times and all tests turned green.

…77691) Follow up of elastic#77650. Looks like two issues caused occasional failures: missingOrder has not been considered in InternalComposite.InternalBucket#compareKey reverseMul has been ignored in one case in OrdinalValuesSource#compareInternal The first problem has clearly been introduced by elastic#76740 but the second point is more interesting. As far as I understand, this might have caused problems before but I couldn't find any according test failures in Gradle before yesterday. In order to preclude further rare test failures I've run the whole CompositeAggregatorTests 3000 times and all tests turned green. # Conflicts: # server/src/main/java/org/elasticsearch/search/aggregations/bucket/composite/InternalComposite.java

Follow up of #77650. Looks like two issues caused occasional failures: missingOrder has not been considered in InternalComposite.InternalBucket#compareKey reverseMul has been ignored in one case in OrdinalValuesSource#compareInternal The first problem has clearly been introduced by #76740 but the second point is more interesting. As far as I understand, this might have caused problems before but I couldn't find any according test failures in Gradle before yesterday. In order to preclude further rare test failures I've run the whole CompositeAggregatorTests 3000 times and all tests turned green. # Conflicts: # server/src/main/java/org/elasticsearch/search/aggregations/bucket/composite/InternalComposite.java

Luegg · 2021-09-16T06:26:04Z

Closes #34221

Documents the missing_order parameter for composite aggregations introduced in #76740

Documents the missing_order parameter for composite aggregations introduced in elastic#76740

) Documents the missing_order parameter for composite aggregations introduced in #76740

elasticsearchmachine added the v8.0.0 label Aug 20, 2021

Luegg changed the title ~~Support _first and _last ordering for missing values in composite aggs~~ Support "first" and "last" ordering of missing values in composite aggs Aug 20, 2021

Luegg force-pushed the enhance/missingFirstLastInCompositeAggs branch 4 times, most recently from b937542 to 1d77b12 Compare August 20, 2021 14:35

Luegg force-pushed the enhance/missingFirstLastInCompositeAggs branch from 1d77b12 to cfc69a6 Compare August 30, 2021 12:57

Merge origin/main into enhance/missingFirstLastInCompositeAggs

5d548ab

Luegg force-pushed the enhance/missingFirstLastInCompositeAggs branch 2 times, most recently from 827f69f to e7db340 Compare August 30, 2021 14:48

Merge origin/main into enhance/missingFirstLastInCompositeAggs

b4e4d9b

Luegg force-pushed the enhance/missingFirstLastInCompositeAggs branch 2 times, most recently from a13c500 to 53eecca Compare August 30, 2021 15:11

Luegg added v7.16.0 :Analytics/CompositeAggs labels Aug 30, 2021

Luegg force-pushed the enhance/missingFirstLastInCompositeAggs branch from 53eecca to 29d8b48 Compare August 30, 2021 16:04

Support "first" and "last" ordering of missing values in composite aggs

3eabe92

Luegg force-pushed the enhance/missingFirstLastInCompositeAggs branch from 29d8b48 to 3eabe92 Compare August 30, 2021 16:11

Luegg added 2 commits August 31, 2021 09:05

Merge origin/master into enhance/missingFirstLastInCompositeAggs

bf1baa3

skip API compat tests for 7.16

904e93a

Luegg requested a review from nik9000 August 31, 2021 08:49

Luegg marked this pull request as ready for review August 31, 2021 08:49

elasticmachine added the Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) label Aug 31, 2021

Luegg changed the title ~~Support "first" and "last" ordering of missing values in composite aggs~~ Support "_first" and "_last" ordering of missing values in composite aggs Aug 31, 2021

Luegg mentioned this pull request Aug 31, 2021

SQL: Inconsistent default ordering of NULL values with and without GROUP BY #77068

Open

nik9000 reviewed Aug 31, 2021

View reviewed changes

not-napoleon self-requested a review August 31, 2021 18:55

Luegg added 2 commits September 1, 2021 17:30

Merge origin/main into enhance/missingFirstLastInCompositeAggs

aea7fe1

Merge origin/main into enhance/missingFirstLastInCompositeAggs

729f2b2

Luegg added the auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) label Sep 13, 2021

elasticsearchmachine merged commit c62d8e2 into elastic:master Sep 13, 2021

Luegg mentioned this pull request Sep 13, 2021

[7.x] Support "_first" and "_last" ordering of missing values in composite aggs (#76740) #77620

Merged

Luegg deleted the enhance/missingFirstLastInCompositeAggs branch September 13, 2021 12:39

Luegg mentioned this pull request Sep 13, 2021

Adjust versions after backporting 76740 to 7.x #77640

Merged

nik9000 mentioned this pull request Sep 13, 2021

Composite test failing after #76740 #77650

Closed

nik9000 added a commit that referenced this pull request Sep 13, 2021

Disable some composite tests

ee42d29

They have started failing about 0.3% of the time each. And there are a couple dozen of them so yay. Relates to #77650 and probably #76740

nik9000 added a commit that referenced this pull request Sep 13, 2021

Disable some composite tests

4ec48a0

They have started failing about 0.3% of the time each. And there are a couple dozen of them so yay. Relates to #77650 and probably #76740

This was referenced Sep 14, 2021

fix composite aggregation tests failing after #76740 #77691

Merged

SQL: fix NULLS FIRST/LAST for aggregations #77750

Merged

Luegg mentioned this pull request Sep 15, 2021

[7.x] fix composite aggregation tests failing after #76740 (#77691) #77779

Merged

Luegg pushed a commit to Luegg/elasticsearch that referenced this pull request Sep 15, 2021

disable bwc tests for backporting elastic#76740

00bff68

Luegg mentioned this pull request Sep 15, 2021

disable bwc tests for backporting #76740 #77781

Merged

elasticsearchmachine pushed a commit that referenced this pull request Sep 15, 2021

disable bwc tests for backporting #76740 (#77781)

4c23260

Luegg linked an issue Sep 16, 2021 that may be closed by this pull request

Enhance position of nulls inside composite aggs sorting #34221

Closed

Luegg mentioned this pull request Sep 16, 2021

Document missing_order param for composite aggregations #77839

Merged

Luegg added a commit that referenced this pull request Sep 27, 2021

Document missing_order param for composite aggregations (#77839)

421b3e8

Documents the missing_order parameter for composite aggregations introduced in #76740

Luegg added a commit to Luegg/elasticsearch that referenced this pull request Sep 27, 2021

Document missing_order param for composite aggregations (elastic#77839)

b4ddb12

Documents the missing_order parameter for composite aggregations introduced in elastic#76740

elasticsearchmachine pushed a commit that referenced this pull request Sep 27, 2021

Document missing_order param for composite aggregations (#77839) (#78302

9800df6

) Documents the missing_order parameter for composite aggregations introduced in #76740

jakelandis added v8.0.0-beta1 and removed v8.0.0 labels Oct 27, 2021

danhermann added the >enhancement label Dec 3, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support "_first" and "_last" ordering of missing values in composite aggs#76740

Support "_first" and "_last" ordering of missing values in composite aggs#76740
elasticsearchmachine merged 14 commits intoelastic:masterfrom
Luegg:enhance/missingFirstLastInCompositeAggs

Luegg commented Aug 20, 2021 •

edited

Loading

Uh oh!

elasticmachine commented Aug 31, 2021

Uh oh!

nik9000 left a comment

Uh oh!

nik9000 Aug 31, 2021

Uh oh!

Luegg Sep 2, 2021 •

edited

Loading

Uh oh!

elasticsearchmachine commented Sep 13, 2021

Uh oh!

Luegg commented Sep 16, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

Conversation

Luegg commented Aug 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elasticmachine commented Aug 31, 2021

Uh oh!

nik9000 left a comment

Choose a reason for hiding this comment

Uh oh!

nik9000 Aug 31, 2021

Choose a reason for hiding this comment

Uh oh!

Luegg Sep 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

elasticsearchmachine commented Sep 13, 2021

💔 Backport failed

Uh oh!

Luegg commented Sep 16, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

Luegg commented Aug 20, 2021 •

edited

Loading

Luegg Sep 2, 2021 •

edited

Loading