-
Notifications
You must be signed in to change notification settings - Fork 24.7k
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
SCRIPTING: Move sort Context to its Own Class #33717
SCRIPTING: Move sort Context to its Own Class #33717
Conversation
* SCRIPTING: Move sort Context to its Own Class * Extracted SortScript * Kept mechanics close to what they were with SearchScript * `execute` appears to return `String` as well as `Number` in certain scenarios so I had to keep the `Object` return here
Pinging @elastic/es-core-infra |
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 wonder if we could have 2 different script classes for String/Number, so we can have type safety? The compilation of the sort script happens at the beginning of ScriptSortBuilder.build, but I think it could be moved inside the switch on type, so the factories/script classes are specific to each type.
@rjernst sure, we can do that :) Will do soon. |
@rjernst done, split the sort context up into 2 contexts for strings and numbers now :) |
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 left a few comments. Don't expressions need to be updated to support the numeric version?
return execute().doubleValue(); | ||
} | ||
|
||
public abstract Number execute(); |
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.
Why does this need to return number? We only ever seem to convert it to double, so I think it should return that?
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.
@rjernst 👍 jup
super(params, lookup, leafContext); | ||
} | ||
|
||
public double runAsDouble() { |
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 don't think we need this, the callers should just call execute().
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.
Right, my bad didn't double check after splitting this => removing :)
} | ||
|
||
/** Return the score of the current document. */ | ||
public double getScore() { |
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.
This will cause a variable score
to be made available to the script, not _score
. It needs to be get_score()
.
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.
right 👍 renaming :)
import org.elasticsearch.search.lookup.LeafSearchLookup; | ||
import org.elasticsearch.search.lookup.SearchLookup; | ||
|
||
abstract class AbstractSortScript implements ScorerAware { |
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 don't think this can be package private, as painless would not be able to access the methods from it?
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'm also not sure there is really enough here to justify a super class. Can we just have the few lines duplicated and abstract this when we see more need?
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.
@rjernst I mainly made the abstract
class because I figured there's quite a bit of magic with the way the lookups work and such and it wasn't so tricky to make this.
Also, it's fine for this to be package private. Painless uses Class#getMethods()
which is documented as:
* Returns an array containing {@code Method} objects reflecting all the
* public methods of the class or interface represented by this {@code
* Class} object, including those declared by the class or interface and
* those inherited from superclasses and superinterfaces.
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.
Thanks, that is good to know about the lookups, I didn't realize that. :)
I would at least rename this as I don't think it has to do with "sorting" at all, it is about making score accessible?
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.
@rjernst true, AbstractScoreAwareScript
? :)
Now that you mention it ... didn't we already forget this in #33602 as well? (we needed to add logic to |
just FYI, I'm back to fixing this now the question above has answered itself in the other PRs :) |
@rjernst this should be good for another review. I made it work for expressions now for numeric sorts :) |
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.
Thanks! LGTM, just a couple minor nits
if (variable.equals("_score")) { | ||
bindings.add(new SortField("_score", SortField.Type.SCORE)); | ||
needsScores = true; | ||
} else if (variable.equals("_value")) { |
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.
_value is only for agg scripts, we shouldn't have it for anything else
} | ||
}; | ||
return context.factoryClazz.cast(factory); | ||
} if (context.instanceClazz.equals(IngestScript.class)) { |
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.
missing "else" here for the else if?
@rjernst thanks! |
* SCRIPTING: Move sort Context to its own Class
execute
appears to returnString
as well asNumber
in certain scenarios so I had to keep the
Object
return hererelates #33602