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

Fix handling of null values + upstream bug fixes in embulk-base-restclient #47

Merged
merged 6 commits into from
Nov 28, 2017

Conversation

kfitzgerald
Copy link
Contributor

Greetings!

This PR does three things:

  1. It updates the embulk-base-restclient to 0.5.5, which fixes handling of null values for numeric and boolean types. Additionally, it allows proper output of null values (instead of treating nulls as undefined).

  2. It uses proper null handling for output, so null values will be outputed to elasticsearch as null, not undefined (e.g. being omitted entirely). Elasticsearch expects and handles null values, so this should be the default, IMHO.

  3. It adds a set of unit tests, which ensure that proper null value handling is being achieved.

Please review and let me know if there's anything else I can do.

Thanks again,
-Kevin

…to omitting them entirely.

 * Added additional tests to ensure output includes null values as expected
 * Waiting on changes in [embulk-base-restclient#102](embulk/embulk-base-restclient#102) to get merged and deployed before `build.gradle` can get updated
@kfitzgerald
Copy link
Contributor Author

See also: embulk/embulk-base-restclient#102

@muga muga requested a review from sakama November 17, 2017 07:00
@sakama
Copy link
Contributor

sakama commented Nov 21, 2017

@kfitzgerald Thanks for your PR.
I've considered about backward compatibility and added new accept_null_value option and set false as a default. kfitzgerald#1

Could you take a look?

@muga I'm still wondering if we should set true as a default or not.
I think it's possible to change behavior if we announce major update of this plugin.
What do you think about?

Add `accept_null_value` option to keep backward compatibility
@kfitzgerald
Copy link
Contributor Author

@sakama Merged! I agree, having an option to preserve the old functionality would be best.

@sakama @muga I think in the case of elasticsearch, I would be in favor of defaulting to true. Understandably, there may be some users who are affected, but chances are the impact would be low, since when queried, they would be excluded anyway. There's a chance that aggregations could be affected, but I would suspect that overall impact would be low.

I am not strongly advocating this way, since new usages of the output plugin could turn this on, or if exposed through a web interface (e.g. TD's connectors ui) the ui-default could set this to true for new connections, preventing issues with older existing scheduled jobs.

@sakama sakama merged commit f24ec88 into embulk:master Nov 28, 2017
@sakama
Copy link
Contributor

sakama commented Nov 28, 2017

Merged.
I'll release new version soon.

@dmikurube dmikurube mentioned this pull request Jun 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants