[GLUTEN-7362][VL] Add test for 'IN' and 'OR' filter in Scan#7363
[GLUTEN-7362][VL] Add test for 'IN' and 'OR' filter in Scan#7363rui-mo merged 1 commit intoapache:mainfrom
Conversation
|
Is this a Gluten issue? |
Yes, the first singularOrList in |
|
cc @rui-mo Thank you. |
rui-mo
left a comment
There was a problem hiding this comment.
Thanks. These are my thoughts.
| runQueryAndCompare( | ||
| "select count(1) from lineitem " + | ||
| "where (l_shipmode in ('TRUCK', 'MAIL') or l_shipmode in ('AIR', 'FOB')) " + | ||
| "and l_shipmode in ('RAIL','SHIP')") { |
There was a problem hiding this comment.
I think this issue is in the condition (A or B) and C, when C is already pushed down, the (A or B) cannot be pushed down.
There was a problem hiding this comment.
Yes, but only for in, which is singularOrList.
| @@ -1669,7 +1669,8 @@ bool SubstraitToVeloxPlanConverter::canPushdownOr( | |||
| } | |||
There was a problem hiding this comment.
And I suggest we add a setOrRange function in RangeRecorder which returns false when other conditions already exist for a field. We could return false if it returns false at L1665 in the canPushdownOr function.
There was a problem hiding this comment.
Thanks for your suggestion, updated.
| uint32_t fieldIdx = getColumnIndexFromSingularOrList(singularOrList); | ||
| // Disable IN pushdown for int-like types. | ||
| if (!rangeRecorders.at(fieldIdx).setInRange(true /*forOrRelation*/)) { | ||
| if (!rangeRecorders.at(fieldIdx).setInRange(sign /*forOrRelation*/)) { |
There was a problem hiding this comment.
We don't need this change as this logic is checking the nested expressions in an or condition, while here the issue is or cannot be pushed down totally.
|
|
||
| /// Set the existence of or-range and returns whether it can coexist with | ||
| /// existing conditions for this field. | ||
| bool setOrRange() { |
There was a problem hiding this comment.
Sorry but I just notice 'setMultiRange' is for the 'or' condition, so we can just use it.
There was a problem hiding this comment.
setMultiRange seems to conflict with setCertainRangeForFunction of 1681L.
| "where l_partkey in (1552, 674) or l_partkey in (1552) and l_orderkey > 1") { _ => } | ||
| checkLengthAndPlan(df, 73) | ||
|
|
||
| runQueryAndCompare( |
There was a problem hiding this comment.
We have merged d6326f0. Would you like to try if this issue has been fixed? Perhaps we can merge this test to ensure the functionality. Thanks for your patience.
There was a problem hiding this comment.
Thank you, I will try it locally later.
|
Local test passed, I modified pr to only add ut. @rui-mo |
What changes were proposed in this pull request?
Fixes: #7362
Resolved by #6754, add test for it.
How was this patch tested?
UT