[hotfix][multistage] support not-in in leaf stage#9610
Conversation
There was a problem hiding this comment.
should we also add tests for v1 like InTransformFunctionTest? also it's unclear to me how this new test relates?
There was a problem hiding this comment.
yeah I can add a test in v1 as well. the trick is that same column with multiple = is converted into range IN and multiple <> is converted into range NOT IN
There was a problem hiding this comment.
Can we document this trick in the comments? :) +1 to have tests on the transform functions only.
There was a problem hiding this comment.
Might want to add an explicit test in addition to the ones above (that test the functionality provided calcite rewrites) just to ensure this always works even if Calcite ever changes it's mind
walterddr
changed the title
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #9610 +/- ##
============================================
- Coverage 63.83% 63.79% -0.05%
+ Complexity 5267 4922 -345
============================================
Files 1888 1886 -2
Lines 101218 101222 +4
Branches 15427 15433 +6
============================================
- Hits 64617 64574 -43
- Misses 31869 31919 +50
+ Partials 4732 4729 -3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
No need to add the second one because they will be canonicalized to the same value
| NOT_IN("not_in", "notin"), | |
| NOT_IN("not_in"), |
There was a problem hiding this comment.
Same for other changes
| Preconditions.checkArgument(numArguments >= 2, "At least 2 arguments are required for [" | |
| + getName() + "] transform function: expression, values"); | |
| Preconditions.checkArgument(numArguments >= 2, "At least 2 arguments are required for [%s] transform function: expression, values", getName()); |
There was a problem hiding this comment.
| intValuesSV[i] = intValuesSV[i] == 0 ? 1 : 0; | |
| intValuesSV[i] = 1 - intValuesSV[i]; |
walterddr
force-pushed
the
pr_hotfix_range_search
branch
from
3e33485 to
60217a3
Compare
| } | ||
| } | ||
| @Test | ||
| public void testIntNotInTransformFunction() { |
There was a problem hiding this comment.
Can we add a e2e query execution test ? May be in one of the subclasses of BaseQueriesTest or TransformQueriesTest ?
There was a problem hiding this comment.
it's a bit hard b/c it will be rewrite as NOT(IN(...)) as stated in the description.
There was a problem hiding this comment.
I see. I am ok with merging though
6 participants
NOTIN is not supported in leaf stage as it is compiled as
NOT(IN(e, v))however this is optimized by calcute into a range searchNOTINin v2. adding this support