-
Notifications
You must be signed in to change notification settings - Fork 24.9k
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
Scripted keyword field #58939
Scripted keyword field #58939
Conversation
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.
LGTM, TODOs and al.
private final LeafReaderContext ctx; | ||
private final SourceLookup source; | ||
private final LeafDocLookup fieldData; | ||
private final LeafSearchLookup leafSearchLookup; |
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.
Ok with me.
public AbstractScriptFieldScript(Map<String, Object> params, SourceLookup source, DocLookup fieldData, LeafReaderContext ctx) { | ||
public AbstractScriptFieldScript(Map<String, Object> params, SearchLookup searchLookup, LeafReaderContext ctx) { | ||
this.leafSearchLookup = searchLookup.getLeafSearchLookup(ctx); | ||
//TODO how do other scripts get stored fields exposed? Through asMap? I don't see any getters for them. |
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.
Yeah, they have them exposed in weird ways that I don't feel comfortable copying. I'd prefer to make a getter for them.
IndexNameExpressionResolver indexNameExpressionResolver, | ||
Supplier<RepositoriesService> repositoriesServiceSupplier) { | ||
//looks like createComponents gets called after getMappers | ||
this.scriptTypeParser.setScriptService(scriptService); |
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.
That is kind of nasty. Not a blocker for the PR, and maybe not a blocker ever, but a little sad!
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.
yea this caught me by surprise. Though it was not hard to work around.
return null; | ||
} | ||
// keywords are internally stored as utf8 bytes | ||
BytesRef binaryValue = (BytesRef) 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.
I think our keywords come out as strings though, right? When does this get called?
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.
no idea, this is copied from the keyword field....
|
||
@Override | ||
protected void parseCreateField(ParseContext context) { | ||
//there is no field! |
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'll get called if the source contains the field. I wonder if we should do anything about that? Like, not now, but maybe sometime? I dunno.
Pinging @elastic/es-search (:Search/Search) |
@romseygeek given all you are doing with mappings, would you like to have a look too and give us some feedback? |
This is the first draft implementation of scripted field of type keyword. Still lots of TODOs to address, this PR targets a feature branch.
Relates to #59332