Null string is encoded as "null" in incremental index#2765
Null string is encoded as "null" in incremental index#2765gianm merged 1 commit intoapache:masterfrom
Conversation
|
Is it possible to include a test that would have caught this? |
There was a problem hiding this comment.
can you explain in your PR description why this line is not enough to fix the problem. What is the purpose of the remaining changes?
|
Can we remove the changes not related to the String null handling? The code being refactored is being redone in #2760, e.g. those value transformers in IncrementalIndex are removed. |
|
@jon-wei sure |
navis
force-pushed
the
invalid-encode-nullstring
branch
from
0f5a845 to
f0e55f5
Compare
|
Is this a bug that affects 0.9.0, or just 0.9.1? |
|
@drcrallen it's from #2263, which is not included in any released versions. |
|
@drcrallen it'll just be for 0.9.1, the code being changed was from #2263, which isn't in 0.9.0 |
|
👍 |
| public String apply(final Object o) | ||
| { | ||
| return String.valueOf(o); | ||
| return o == null ? null : String.valueOf(o); |
There was a problem hiding this comment.
Should this be converting "" to null or is something else doing that?
There was a problem hiding this comment.
I think it's fine, null/empty dim values will end up as "" via a nullToEmpty() call when added to the dictionary:
There was a problem hiding this comment.
ah ok, this looks good then. thanks
|
👍 |
| String s = (String) o; | ||
| try { | ||
| return Long.valueOf((String) o); | ||
| return s.isEmpty() ? null : Long.valueOf(s); |
There was a problem hiding this comment.
does this change any current behavior that might map empty strings to 0 for numeric values?
There was a problem hiding this comment.
I don't think it could, since with the old code, Long.valueOf("") would have thrown an exception. The new code would return null.
5 participants
I've found null values for string type is encoded as "null", which seemed not intended behavior.