ingest convert processor add long and double type.#23423
ingest convert processor add long and double type.#23423fishioon wants to merge 5 commits intoelastic:masterfrom
Conversation
|
brilliant! We try to have tests on all of our PRs before merging them in -- think you are up for adding appropriate tests in this file: ConvertProcessorTests.java? I can also follow-up with updating the documentation |
|
Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually? |
|
jenkins please test this |
1 similar comment
|
jenkins please test this |
|
I am sorry for forgot this. It's my mistake and fix now. |
|
no worries, I'll re-run the test and then do a final review. thanks for following up! |
|
jenkins please test this |
|
jenkins please test this |
1 similar comment
|
jenkins please test this |
talevy
left a comment
There was a problem hiding this comment.
Overall looks good, Left a note about the overlapped matching that can go on between floats and doubles.
I think I am OK with that, maybe we should change it to give Double higher precedence
| try { | ||
| return FLOAT.convert(value); | ||
| } catch (IllegalArgumentException e) {} | ||
| try { |
There was a problem hiding this comment.
Problem here is that FLOAT.convert(value) would succeed and produce potential rounding errors instead of failing and dropping down to here.
Looks like setting AUTO would potentially never generate a FLOAT if we re-order these
| assertThat(convertedValue, equalTo(randomFloat)); | ||
| } | ||
|
|
||
| public void testAutoConvertMatchDouble() throws Exception { |
There was a problem hiding this comment.
This test fails because of my previous comment, Floats match before Doubles.
| long randomLong = randomLong(); | ||
| randomValue = randomLong; | ||
| break; | ||
| case 4: |
There was a problem hiding this comment.
There is a tab (\t) here, should be converted to spaces 😄
|
Hi @fishioon. would love to see these changes go through, but there hasn't been a response in a while. I am going to go ahead and close this for now, but feel free to re-open the PR to continue! |
issue around this enhancement here #23085