Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add an option to force the numeric type of a field sort #38095

Merged
merged 9 commits into from
Mar 15, 2019

Conversation

jimczi
Copy link
Contributor

@jimczi jimczi commented Jan 31, 2019

This change adds an option to the FieldSortBuilder that allows to transform the type
of a numeric field into another. Possible values for this option are long that transforms
the source field into an integer and double that transforms the source field into a floating point.
This new option is useful for cross-index search when the sort field is mapped differently on some
indices. For instance if a field is mapped as a floating point in one index and as an integer in another
it is possible to align the type for both indices using the numeric_type option:

{
   "sort": {
    "field": "my_field",
    "numeric_type": "double" <1>
   }
}

<1> Ensure that values for this field are transformed to a floating point if needed.

Only long and double are supported at the moment but the goal is to also handle date and date_nanos when #32601 is merged.

Naming should also be improved, I choose numeric_type to start with something but I am not happy with it. Propositions are welcome ;).

This change adds an option to the `FieldSortBuilder` that allows to transform the type
of a numeric field into another. Possible values for this option are `long` that transforms
the source field into an integer and `double` that transforms the source field into a floating point.
This new option is useful for cross-index search when the sort field is mapped differently on some
indices. For instance if a field is mapped as a floating point in one index and as an integer in another
it is possible to align the type for both indices using the `numeric_type` option:

```
{
   "sort": {
    "field": "my_field",
    "numeric_type": "double" <1>
   }
}
```

<1> Ensure that values for this field are transformed to a floating point if needed.

Only `long` and `double` are supported at the moment but the goal is to also handle `date` and `date_nanos`
when elastic#32601 is merged.
@jimczi jimczi added >feature :Search/Search Search-related issues that do not fall into other categories v7.0.0 v6.7.0 labels Jan 31, 2019
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-search

@jasontedor jasontedor added v8.0.0 and removed v7.0.0 labels Feb 6, 2019
Copy link
Contributor

@mayya-sharipova mayya-sharipova left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Jim, makes sense!

One thing I was wondering (not related to your PR) if it is correct that we return 503 for wrong sorting (using float and double and not setting numeric_type)? Should it be 400 instead?

{
  "error": {
    "root_cause": [],
    "type": "search_phase_execution_exception",
    "reason": "",
    "phase": "fetch",
    "grouped": true,
    "failed_shards": [],
    "caused_by": {
      "type": "class_cast_exception",
      "reason": "class java.lang.Float cannot be cast to class java.lang.Double (java.lang.Float and java.lang.Double are in module java.base of loader 'bootstrap')"
    }
  },
  "status": 503
}


Since `field` is mapped as a `double` in the first index and as a `long`
in the second index, it is not possible to use this field to sort requests
that query both indices bu default. However you can force the type to one
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bu default -> by default

@jpountz jpountz self-requested a review March 11, 2019 09:51
Copy link
Contributor

@jpountz jpountz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left some comments but the idea makes sense to me.

It is also possible to transform a floating point field into a `long`
but note that in this case floating points are replaced by the largest
value that is less than or equal to the argument and is equal to a mathematical
integer.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is not correct for negative values

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

right, thanks

SortField field = fieldData.sortField(missing, localSortMode, nested, reverse);
final SortField field;
if (numericType != null) {
if (fieldData instanceof SortedNumericDVIndexFieldData == false) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why do you check against this class rather than IndexNumericFieldData?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a mistake, I pushed 24c961a

@jimczi jimczi requested a review from jpountz March 12, 2019 16:05
@jimczi jimczi merged commit ddd34b1 into elastic:master Mar 15, 2019
@jimczi jimczi deleted the sort_mix_numeric branch March 15, 2019 09:25
jimczi added a commit to jimczi/elasticsearch that referenced this pull request Mar 15, 2019
This change adds an option to the `FieldSortBuilder` that allows to transform the type
of a numeric field into another. Possible values for this option are `long` that transforms
the source field into an integer and `double` that transforms the source field into a floating point.
This new option is useful for cross-index search when the sort field is mapped differently on some
indices. For instance if a field is mapped as a floating point in one index and as an integer in another
it is possible to align the type for both indices using the `numeric_type` option:

```
{
   "sort": {
    "field": "my_field",
    "numeric_type": "double" <1>
   }
}
```

<1> Ensure that values for this field are transformed to a floating point if needed.
jimczi added a commit to jimczi/elasticsearch that referenced this pull request Mar 15, 2019
This change adapts the bwc version check for the new numeric_type option of the FieldSortBuilder.
Since this change needs to be backported to 7x, all bwc tests are temporary disabled
until elastic#40084 is merged.

Relates elastic#38095
jimczi added a commit that referenced this pull request Mar 18, 2019
)

This change adds an option to the `FieldSortBuilder` that allows to transform the type
of a numeric field into another. Possible values for this option are `long` that transforms
the source field into an integer and `double` that transforms the source field into a floating point.
This new option is useful for cross-index search when the sort field is mapped differently on some
indices. For instance if a field is mapped as a floating point in one index and as an integer in another
it is possible to align the type for both indices using the `numeric_type` option:

```
{
   "sort": {
    "field": "my_field",
    "numeric_type": "double" <1>
   }
}
```

<1> Ensure that values for this field are transformed to a floating point if needed.
jimczi added a commit that referenced this pull request Mar 18, 2019
This change adapts the bwc version check for the new numeric_type option of the FieldSortBuilder.
Since this change needs to be backported to 7x, all bwc tests are temporary disabled
until #40084 is merged.

Relates #38095
@jakelandis jakelandis added v7.3.0 and removed v7.2.0 labels Jun 17, 2019
@jpountz
Copy link
Contributor

jpountz commented Jul 5, 2019

@jimczi I'm assuming there is nothing left to backport and removed the backport pending label.

@russcam
Copy link
Contributor

russcam commented Aug 30, 2019

Looks like this went into v7.2.0?

public FieldSortBuilder setNumericType(String numericType) {
String lowerCase = numericType.toLowerCase(Locale.ENGLISH);
switch (lowerCase) {
case "long":
case "double":
case "date":
case "date_nanos":
break;
default:
throw new IllegalArgumentException("invalid value for [numeric_type], " +
"must be [long, double, date, date_nanos], got " + lowerCase);
}
this.numericType = lowerCase;
return this;
}

@jpountz jpountz added v7.2.0 and removed v7.3.0 labels Sep 4, 2019
jpountz added a commit that referenced this pull request Sep 5, 2019
jpountz added a commit that referenced this pull request Sep 5, 2019
jpountz added a commit that referenced this pull request Sep 5, 2019
jpountz added a commit that referenced this pull request Sep 5, 2019
@jpountz
Copy link
Contributor

jpountz commented Sep 5, 2019

@russcam Indeed, I just fixed the version tag and the release notes. Thanks for spotting!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>feature :Search/Search Search-related issues that do not fall into other categories v7.2.0 v8.0.0-alpha1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants