[GLUTEN-7362][VL] Add test for 'IN' and 'OR' filter in Scan #7363
Conversation
|
Is this a Gluten issue? |
Yes, the first singularOrList in |
|
cc @rui-mo Thank you. |
| 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.
| if (!canPushdownSingularOrList(singularOrList, true)) { | ||
| return false; | ||
| } | ||
| 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.
| @@ -286,6 +286,16 @@ class SubstraitToVeloxPlanConverter { | |||
| return true; | |||
| } | |||
|
|
|||
| /// 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.
| @@ -239,6 +239,13 @@ class MiscOperatorSuite extends VeloxWholeStageTransformerSuite with AdaptiveSpa | |||
| "select l_orderkey from lineitem " + | |||
| "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.
zml1206
changed the title
|
Local test passed, I modified pr to only add ut. @rui-mo |
zml1206
changed the title
zml1206
changed the title
rui-mo
changed the title
What changes were proposed in this pull request?
Fixes: #7362
Resolved by #6754, add test for it.
How was this patch tested?
UT