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

Changed script to work with Elastic 6.0.0. #17

Open
wants to merge 27 commits into
base: master
Choose a base branch
from

Conversation

Zakkery
Copy link

@Zakkery Zakkery commented Jan 30, 2019

Hi,

This should be working on Elastic 6.0.0. They really simplified design of scoring scripts so it is now only one file, pretty much.
Inline scripts are now depreciated so using 'source' field instead.
I had to also alter test files since Elastic 6.0.0 uses Netty4Plugin now and some settings are now disabled.
I tested it on real data and it seems to work, but let me know if you have any concern.

Best,
Zakkery

@lior-k
Copy link
Owner

lior-k commented Feb 2, 2019

Hi Zakkery.
Thanks for the PR.
I'll add my comments in-line
But 1 in general - several files instead of 1 helps me keep the clutter of Plugin boiler plate away & focus only on the 2 important methods: run & compile.
I would really appreciate it If you can move the clutter to separate files & keep indentation to a minimum

ran22 and others added 6 commits February 10, 2019 16:27
Note that all vectors should be float instead of doubles
This introduces ~ 50% performance boost. so it's worth it
…ncodedVector field, added runAsLong(), changed searchVector to be a primitive type, made SCRIPT_SOURCE private static class member
Fixed an issue with the go example code
@Zakkery
Copy link
Author

Zakkery commented Apr 4, 2019

I broke it back into multiple files, like you suggested and fixed code according to your comments.

@lior-k
Copy link
Owner

lior-k commented Apr 23, 2019

Hi Zakkery. note that master was changed to use float vectors instead of doubles.
please review your changes against master.
thanks

@Zakkery
Copy link
Author

Zakkery commented Apr 23, 2019

Hi Zakkery. note that master was changed to use float vectors instead of doubles.
please review your changes against master.
thanks

I just did a rebase, I guess I forgot to do it, now should be all floats and merged properly.


final int len = input.readVInt();
// in case vector is of different size
if (len != inputVector.length * Double.BYTES) {
Copy link
Owner

Choose a reason for hiding this comment

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

this will always be true since we moved to floats.
I would like the scoring to fail if we have a problem with the vectors instead of hiding it by returning 0.
It helped us find bugs in our vector creation code. So please remove this check & the try..catch on the entire function

Copy link
Author

@Zakkery Zakkery Apr 25, 2019

Choose a reason for hiding this comment

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

I cannot remove try/catch - binaryEmbeddingReader.binaryValue() can throw an IOException:
public abstract BytesRef binaryValue() throws IOException

I did change it to be * Float.BYTES

I think it is possible to move binaryEmbeddingReader.binaryValue().bytes into setDocument and handle exception there but isn't it a bit messy?

Iakov Gorelik and others added 5 commits April 25, 2019 13:32
…ncodedVector field, added runAsLong(), changed searchVector to be a primitive type, made SCRIPT_SOURCE private static class member
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants