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

Support binary field type in script values #21484

Merged
merged 2 commits into from Nov 22, 2016

Conversation

Projects
None yet
4 participants
@umeshdangat
Contributor

umeshdangat commented Nov 11, 2016

Resolves #14469

Indexed docs with binary doc value type as mentioned in the issue: https://gist.github.com/umeshdangat/43b10532dc52c5d92c1a2d7aaf8c0d8c

Painless script to access binary type as script values:
https://gist.github.com/umeshdangat/d236b8f70ddfda739df8b01c672413b5

Result: No more unsupported operations exception: https://gist.github.com/umeshdangat/c137c5d426bc23c5ff02b087d3a3684c

Java Plugin which reads the binary field via ScriptDocValues
https://gist.github.com/umeshdangat/882caf25fdfd6090855a1252f210067c

I decided to add BytesRefs to ScriptDocValues because it seems more appropriate to return a BytesRef and then use it to extract byte[] like so:
byte[] arr = ((ScriptDocValues.BytesRefs) doc().get("qa_data")).get(0).bytes;

fixes issue:14469, support binary field type in script values
add ScriptDocValues.BytesRefs for reading binary fieldtype
@nik9000

This comment has been minimized.

Show comment
Hide comment
@nik9000

nik9000 Nov 15, 2016

Contributor

Makes sense to me. Could you add a test? Maybe a test case in ScriptQuerySearchIT?

Contributor

nik9000 commented Nov 15, 2016

Makes sense to me. Could you add a test? Maybe a test case in ScriptQuerySearchIT?

@umeshdangat

This comment has been minimized.

Show comment
Hide comment
@umeshdangat

umeshdangat Nov 16, 2016

Contributor

@nik9000 added tests, thanks for pointing to the ScriptQuerySearchIT, I tried to follow the same test pattern. Although, I had to post my own mapping in the tests and turn the doc_values on explicitly.

Seems like they are disabled by default for binary fields: https://gist.github.com/umeshdangat/3a831942e354099f8d9bc5b2af1327b4

Although the documentation says otherwise:
https://www.elastic.co/guide/en/elasticsearch/reference/current/binary.html

Contributor

umeshdangat commented Nov 16, 2016

@nik9000 added tests, thanks for pointing to the ScriptQuerySearchIT, I tried to follow the same test pattern. Although, I had to post my own mapping in the tests and turn the doc_values on explicitly.

Seems like they are disabled by default for binary fields: https://gist.github.com/umeshdangat/3a831942e354099f8d9bc5b2af1327b4

Although the documentation says otherwise:
https://www.elastic.co/guide/en/elasticsearch/reference/current/binary.html

@clintongormley clintongormley changed the title from fixes issue:14469, support binary field type in script values to Support binary field type in script values Nov 19, 2016

@umeshdangat

This comment has been minimized.

Show comment
Hide comment
@umeshdangat

umeshdangat Nov 22, 2016

Contributor

@nik9000 do the tests look good to you?

Contributor

umeshdangat commented Nov 22, 2016

@nik9000 do the tests look good to you?

@nik9000

This comment has been minimized.

Show comment
Hide comment
@nik9000

nik9000 Nov 22, 2016

Contributor

@nik9000 do the tests look good to you?

Looks great. Sorry I didn't notice earlier. I'll see if I can get this into 5.1.

Contributor

nik9000 commented Nov 22, 2016

@nik9000 do the tests look good to you?

Looks great. Sorry I didn't notice earlier. I'll see if I can get this into 5.1.

SearchResponse response = client().prepareSearch()
.setQuery(scriptQuery(
new Script(ScriptType.INLINE, CustomScriptPlugin.NAME, "doc['binaryData'].get(0).length > 15", Collections.emptyMap())))

This comment has been minimized.

@nik9000

nik9000 Nov 22, 2016

Contributor

This line is too long, it is causing checkstyle to fail. We have a line limit of 140 characters that we started enforcing a few months ago but only on files that didn't have any violations. We're super super slowly fixing the violations as we go.

I'll fix this before I merge.

@nik9000

nik9000 Nov 22, 2016

Contributor

This line is too long, it is causing checkstyle to fail. We have a line limit of 140 characters that we started enforcing a few months ago but only on files that didn't have any violations. We're super super slowly fixing the violations as we go.

I'll fix this before I merge.

private byte[] getRandomBytes(int len) {
final byte[] randomBytes = new byte[len];
new Random().nextBytes(randomBytes);

This comment has been minimized.

@nik9000

nik9000 Nov 22, 2016

Contributor

This API call is forbidden and fails the build. random().nextBytes(randomBytes); is fine though. I'll fix it before I merge.

@nik9000

nik9000 Nov 22, 2016

Contributor

This API call is forbidden and fails the build. random().nextBytes(randomBytes); is fine though. I'll fix it before I merge.

@nik9000 nik9000 merged commit f37db2f into elastic:master Nov 22, 2016

1 check passed

CLA Commit author has signed the CLA
Details

nik9000 added a commit that referenced this pull request Nov 22, 2016

Clean up ScriptQuerySearchIT
Shorten line and remove forbidden API.

Relates to #21484
@nik9000

This comment has been minimized.

Show comment
Hide comment
@nik9000

nik9000 Nov 22, 2016

Contributor

Merged! Thanks @umeshdangat!

Master: f37db2f + 29e6832
5.x: a0fdc29 + 6f86c81

Contributor

nik9000 commented Nov 22, 2016

Merged! Thanks @umeshdangat!

Master: f37db2f + 29e6832
5.x: a0fdc29 + 6f86c81

nik9000 added a commit that referenced this pull request Nov 22, 2016

Support binary field type in script values (#21484)
Add ScriptDocValues.BytesRefs for reading binary fieldtype

nik9000 added a commit that referenced this pull request Nov 22, 2016

Clean up ScriptQuerySearchIT
Shorten line and remove forbidden API.

Relates to #21484
@tianyayoucao

This comment has been minimized.

Show comment
Hide comment
@tianyayoucao

tianyayoucao Aug 7, 2017

I‘ve read your great article “Moving Yelp's Core Business Search to Elasticsearch” which really attracts me,could you open the source code giving me an advice,thanks a lot!@umeshdangat

tianyayoucao commented Aug 7, 2017

I‘ve read your great article “Moving Yelp's Core Business Search to Elasticsearch” which really attracts me,could you open the source code giving me an advice,thanks a lot!@umeshdangat

@umeshdangat

This comment has been minimized.

Show comment
Hide comment
@umeshdangat

umeshdangat Aug 7, 2017

Contributor

@tianyayoucao, Thanks for reading the article! We have considered the possibility of open sourcing the hot reload of code within an elasticsearch plugin, but we have no ETA or a fixed timeline on this as of today.
However, the code snippets mentioned in Moving Yelp's Core Business Search to Elasticsearch should help you give building blocks needed for this.

Contributor

umeshdangat commented Aug 7, 2017

@tianyayoucao, Thanks for reading the article! We have considered the possibility of open sourcing the hot reload of code within an elasticsearch plugin, but we have no ETA or a fixed timeline on this as of today.
However, the code snippets mentioned in Moving Yelp's Core Business Search to Elasticsearch should help you give building blocks needed for this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment