fix bugs with multi-value string array expression handling#12244
Closed
clintropolis wants to merge 3 commits intoapache:masterfrom
Closed
fix bugs with multi-value string array expression handling#12244clintropolis wants to merge 3 commits intoapache:masterfrom
clintropolis wants to merge 3 commits intoapache:masterfrom
Conversation
| if (row.size() == 0) { | ||
| return new Object[]{null}; | ||
| if (row.size() == 0 || (row.size() == 1 && selector.getObject() == null)) { | ||
| if (homogenize) { |
Contributor
There was a problem hiding this comment.
maybe I am overthinking but does it make sense to take out this branch and return a different supplier itself? Might be unnecessary if this method is not very hot.
Member
Author
There was a problem hiding this comment.
eh, it is a hot path, though probably not the worst thing happening in non-vectorized expressions, so an extra branch probably doesn't do that much heh
that said, I think these input binding suppliers are probably worth thinking about either optimizing with additional specialized implementations, or at least using as an example of what not to do when we add multi-value string and array support for vectorized expressions.
Member
Author
|
closing in favor of #12241 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Split out bug fixes from #12241 in case that PR is too noisy. This PR fixes some issues uncovered while working on that other PR, related to null coercion with multi-value string array expressions, similar to the bugs fixed #12078 (and controlled by the flag #12210)
This fix exists entirely within that other PR, though there is a subtle difference between the output of
CalciteMultiValueStringQueryTest.testMultiValueListFilterComposeddue to differences in behavior between the falllbackfilterexpression and the nativeListFilteredVirtualColumn(filterproduces 0 length arrays for rows which do not match, whileListFilteredVirtualColumnconsiders these values asnullwhich is consistent with the behavior of other multi-value string dimension selectors).Should the other PR be merged before this one, this can be closed (and likewise will fix up the other PR should this one be merged first).
This PR has: