Skip to content

Conversation

pquentin
Copy link
Member

@pquentin pquentin commented Oct 3, 2025

Should fix the following test:

// Test file: /test/free/aggregations/range.yml
// Test name: Range aggregation on date field

import { expectAssignable } from 'tsd'
import * as T from '../types'

expectAssignable<T.SearchRequest>({
  "body": {
    "aggs": {
      "date_range": {
        "range": {
          "field": "date",
          "ranges": [
            {
              "from": "2021-05-01T00:00:00Z",
              "to": "2021-05-05T00:00:00Z"
            }
          ]
        }
      }
    },
    "size": 0
  },
  "index": "date_range_test",
  "typed_keys": true
})

@pquentin pquentin added the skip-backport This pull request should not be backported label Oct 3, 2025
@github-actions
Copy link
Contributor

github-actions bot commented Oct 3, 2025

Following you can find the validation changes against the target branch for the APIs.

API Status Request Response
get 🟢 317/317 → 321/321 314/314 → 318/318
index 🟢 1440/1440 → 1445/1445 1442/1442 → 1447/1447
indices.create 🔴 1373/1397 → 1378/1402 1397/1397 → 1402/1402
indices.get_mapping 🔴 198/198 → 202/202 187/198 → 191/202
search 🔴 2584/2611 → 2586/2612 2611/2611 → 2612/2612

You can validate these APIs yourself by using the make validate target.

@flobernd
Copy link
Member

Let me check the consequences of this change for the .NET client please, before you merge it.

FieldValue does not look like the best candidate here at a first glance. Would DateTime work? If I remember correctly this type alles string and timestamp representation. However, not sure why the range was a double before. AFAIK we do have double timestamp types as well, but not yet a combined version like DateTime.

We should probably introduce a new type of DateTime does not fit, to allow statically typed languages to use their native DateTime primitives like we do in all other cases that deal with date/time values.

@flobernd
Copy link
Member

Ok, so I checked this in detail and I think the test is simply not valid.

"aggs": {
  "date_range": {
    "range": {

This part makes no sense. date_range does not support a range field. It supports ranges which is correctly typed as ranges?: DateRangeExpression[] and therefore supports both representations (double and string).

The AggregationRange type you changed in this PR is used by GeoDistanceAggregation and the regular RangeAggregation but does not affect DateRangeAggregation.

WDYT @l-trotta @Anaethelion ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

skip-backport This pull request should not be backported specification

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants