-
Notifications
You must be signed in to change notification settings - Fork 988
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
add scripted upsert feature #1454
Conversation
💚 CLA has been signed |
441cfd5
to
9b64aa6
Compare
@elasticmachine update branch |
I think there might be some additional problems to work out with scripted upserts and es-spark. When I try this (using the scripted upsert example from https://www.elastic.co/guide/en/elasticsearch/reference/current/docs-update.html#scripted_upsert)
I get
It looks like
But with that I get:
It doesn't seem to like the empty map being passed in. |
Oh the first one was just a mistake on my part -- there is special syntax for constants: https://www.elastic.co/guide/en/elasticsearch/hadoop/current/configuration.html#cfg-update. After using the correct syntax I run into the same |
@@ -38,7 +39,9 @@ | |||
@Override | |||
protected void doWriteObject(Object object, BytesArray storage, ValueWriter<?> writer) { | |||
if (ConfigurationOptions.ES_OPERATION_UPSERT.equals(settings.getOperation())) { | |||
super.doWriteObject(object, storage, writer); | |||
if (settings.hasScriptUpsert()) { | |||
super.doWriteObject(Collections.emptyMap(), storage, writer); |
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.
DataFrameValueWriter::write can't handle this being a Collection rather than a Tuple2. But if I change this line to
storage.add("{}");
then it seems to work (at least for my one test case). There might be a cleaner way to do 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.
Maybe something like this instead:
FastByteArrayOutputStream bos = new FastByteArrayOutputStream(storage);
JacksonJsonGenerator generator = new JacksonJsonGenerator(bos);
generator.writeBeginObject();
generator.writeEndObject();
generator.close();
@lucebert I branched off of your branch and put some changes (including a new integration test) here: https://github.com/masseyke/elasticsearch-hadoop/tree/feature/add-script-upsert-2. If you have a chance, I'd appreciate hearing if that works for your case. |
@masseyke Unfortunatly, I no longer have access to the case because I work now for another client However, in my memory it was something quite similar to this test:
I tried with your branch and it works well |
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.
Looks good to me now (I merged in my branch).
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
resolves #1449
Thank you for submitting a pull request!
Please make sure you have signed our Contributor License Agreement (CLA).
We are not asking you to assign copyright to us, but to give us the right to distribute your code without restriction. We ask this of all contributors in order to assure our users of the origin and continuing existence of the code.
You only need to sign the CLA once.