use object[] instead of string[] for vector expressions to be consistent with vector object selectors #13209
Conversation
…ent with vector object selectors
There was a problem hiding this comment.
I think it looks fine. I left a few comments where I think we could maybe do things to be a bit more defensive. In the worst case, though, these changes won't break new things, they just might not fix all of the things that they wanted to (i.e. in some cases, the exception might've just moved to be whack-a-moled a different day).
| @@ -118,7 +119,7 @@ public int getCurrentVectorSize() | |||
| */ | |||
| private static final class StringLookupVectorInputBindings implements Expr.VectorInputBinding | |||
| { | |||
| private final String[] currentValue = new String[1]; | |||
| private final Object[] currentValue = new Object[1]; | |||
There was a problem hiding this comment.
Nitish: The naming of this private static class doesn't really line up with its implementation anymore?
There was a problem hiding this comment.
I guess it is still used exclusively for 'deferred' single string expressions, so maybe is ok?
| { | ||
| // in sql compatible mode, nulls are handled by super class and never make it here... | ||
| return leftVal + rightVal; | ||
| return leftVal + (String) rightVal; |
There was a problem hiding this comment.
You could call .toString() and avoid ClassCastExceptions here? Or, if it's important to do the cast, we should really do an instanceof check first so that we can generate a better error message than just ClassCastException
There was a problem hiding this comment.
same comment as #13209 (comment) about why is safe
| output[i] = Evals.asLong(bool); | ||
| outputNulls[i] = bool; | ||
| return; | ||
| } else if (rightNull) { | ||
| final boolean bool = Evals.asBoolean(leftInput[i]); | ||
| final boolean bool = Evals.asBoolean((String) leftInput[i]); |
There was a problem hiding this comment.
Here too, why is it safe to cast? do we need to check the type first? Or maybe check the type of the array that we are gotten, so we can amortize the cost of the check?
There was a problem hiding this comment.
this safe because (for better or for worse) the base classes explicitly wrap their input processors with CastToTypeVectorProcessor.cast (which is a no-op if the processor is already the correct type) to ensure that it is being used with the correct type. I don't remember exactly why I did this, presumably because its important for vector processing that everything is the right type so we don't get ugly class cast exceptions, but I can look into if this is actually necessary or not.
…ent with vector object selectors (apache#13209) * use object[] instead of string[] for vector expressions to be consistent with vector object selectors * simplify
* use object[] instead of string[] for vector expressions to be consistent with vector object selectors (#13209) * fix issue with nested column null value index incorrectly matching non-null values (#13211) * fix json_value sql planning with decimal type, fix vectorized expression math null value handling in default mode (#13214)
Description
Changes vectorized string expressions (such as concat) to deal in
Object[]instead ofString[], to be consistent with the behavior ofVectorObjectSelector, as well as with a similar change I did recently to array expressions in #12914.This fixes a class of bugs such as in the added test example, which would fail with an error of the form:
before the changes in this patch.
This PR has: