-
Notifications
You must be signed in to change notification settings - Fork 25.6k
Allow to sort by script value using SemVer semantics #85990
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
Changes from all commits
093e066
71fa269
4663b48
c25793a
2095555
4b14243
2270056
3585a06
1d0219a
6ef4bcd
69c5d93
a39eccd
ab7f3f2
439dc74
87dfaad
53203cc
f1f3cb8
8a530ef
0ecab77
7fae61e
8e88f50
1345751
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| pr: 85990 | ||
| summary: Allow to sort by script value using `SemVer` semantics | ||
| area: Infra/Scripting | ||
| type: bug | ||
| issues: | ||
| - 85989 | ||
| - 82287 |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,18 @@ | ||
| # | ||
| # Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one | ||
| # or more contributor license agreements. Licensed under the Elastic License | ||
| # 2.0 and the Server Side Public License, v 1; you may not use this file except | ||
| # in compliance with, at your election, the Elastic License 2.0 or the Server | ||
| # Side Public License, v 1. | ||
| # | ||
|
|
||
| # The whitelist for the fields api | ||
| # The scripts must be whitelisted for painless to find the classes for the field API | ||
|
|
||
| class org.elasticsearch.script.BytesRefProducer @no_import { | ||
| } | ||
|
|
||
| class org.elasticsearch.script.BytesRefSortScript @no_import { | ||
| } | ||
| class org.elasticsearch.script.BytesRefSortScript$Factory @no_import { | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,20 @@ | ||
| /* | ||
| * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one | ||
| * or more contributor license agreements. Licensed under the Elastic License | ||
| * 2.0 and the Server Side Public License, v 1; you may not use this file except | ||
| * in compliance with, at your election, the Elastic License 2.0 or the Server | ||
| * Side Public License, v 1. | ||
| */ | ||
|
|
||
| package org.elasticsearch.script; | ||
|
|
||
| import org.apache.lucene.util.BytesRef; | ||
|
|
||
| /** | ||
| * used by {@link org.elasticsearch.search.sort.ScriptSortBuilder} to refer to classes in x-pack | ||
| * (eg. org.elasticsearch.xpack.versionfield.Version) that need a custom FieldComparatorSource | ||
| */ | ||
| public interface BytesRefProducer { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you add a line in the Javadoc about how this class is used. |
||
|
|
||
| BytesRef toBytesRef(); | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,38 @@ | ||
| /* | ||
| * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one | ||
| * or more contributor license agreements. Licensed under the Elastic License | ||
| * 2.0 and the Server Side Public License, v 1; you may not use this file except | ||
| * in compliance with, at your election, the Elastic License 2.0 or the Server | ||
| * Side Public License, v 1. | ||
| */ | ||
| package org.elasticsearch.script; | ||
|
|
||
| import java.io.IOException; | ||
| import java.util.Map; | ||
|
|
||
| public abstract class BytesRefSortScript extends AbstractSortScript { | ||
|
|
||
| public static final String[] PARAMETERS = {}; | ||
|
|
||
| public static final ScriptContext<Factory> CONTEXT = new ScriptContext<>("bytesref_sort", Factory.class); | ||
|
|
||
| public BytesRefSortScript(Map<String, Object> params, DocReader docReader) { | ||
| super(params, docReader); | ||
| } | ||
|
|
||
| public abstract Object execute(); | ||
|
|
||
| /** | ||
| * A factory to construct {@link BytesRefSortScript} instances. | ||
| */ | ||
| public interface LeafFactory { | ||
| BytesRefSortScript newInstance(DocReader reader) throws IOException; | ||
| } | ||
|
|
||
| /** | ||
| * A factory to construct stateful {@link BytesRefSortScript} factories for a specific index. | ||
| */ | ||
| public interface Factory extends ScriptFactory { | ||
| LeafFactory newFactory(Map<String, Object> params); | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -7,21 +7,28 @@ | |
|
|
||
| package org.elasticsearch.xpack.versionfield; | ||
|
|
||
| import org.elasticsearch.xcontent.ToXContent; | ||
| import org.apache.lucene.util.BytesRef; | ||
| import org.elasticsearch.script.BytesRefProducer; | ||
| import org.elasticsearch.xcontent.ToXContentFragment; | ||
| import org.elasticsearch.xcontent.XContentBuilder; | ||
|
|
||
| import java.io.IOException; | ||
|
|
||
| /** | ||
| * Script value class. | ||
| * TODO(stu): implement {@code Comparable<Version>} based on {@code VersionEncoder#prefixDigitGroupsWithLength(String, BytesRefBuilder)} | ||
| * See: https://github.com/elastic/elasticsearch/issues/82287 | ||
| */ | ||
| public class Version implements ToXContent { | ||
| public class Version implements ToXContentFragment, BytesRefProducer, Comparable<Version> { | ||
| protected String version; | ||
| protected BytesRef bytes; | ||
|
|
||
| public Version(String version) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wonder if we should have another constructor that takes the encoded version, this String version is mostly for users.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do you mean a
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. From a quick look I think we can have a ctor that only takes the BytesRef and internally decodes it to a String only once, then use one for the xContent/toString part and the other for comparison.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We still need the
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, I'm keeping both (see last commit) |
||
| this.version = version; | ||
| this.bytes = VersionEncoder.encodeVersion(version).bytesRef; | ||
| } | ||
|
|
||
| protected Version(BytesRef bytes) { | ||
| this.version = VersionEncoder.decodeVersion(bytes); | ||
| this.bytes = bytes; | ||
| } | ||
|
|
||
| @Override | ||
|
|
@@ -35,7 +42,12 @@ public XContentBuilder toXContent(XContentBuilder builder, Params params) throws | |
| } | ||
|
|
||
| @Override | ||
| public boolean isFragment() { | ||
| return false; | ||
| public BytesRef toBytesRef() { | ||
| return bytes; | ||
| } | ||
|
|
||
| @Override | ||
| public int compareTo(Version o) { | ||
| return toBytesRef().compareTo(o.toBytesRef()); | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any plans to use this interface on anything other than
Version? Otherwise maybe the instance check this is used here could simple test forVersioninstead. Or is this here so you don't need to referenceVersionfrom within the server module?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The interface is there only to decouple this fix from x-pack, I'd be happy to remove it if we move Verson to server module.
Just for completeness, we could use this interface (and the same fix) for at least another case, that is IP field type, but the interface would still be overkill (an if/else would be more than enough probably)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, thats what I guessed. No worries, I think if moving complicates things right now the interface is fine. Maybe this could benefit from a small class comment why its there then, so the next person doesn't ask again.