[GLUTEN-9849][VL] Avoid VeloxBloomFilterMightContain being applied to FileSourceScan partition filters #9850
Conversation
|
Run Gluten ClickHouse CI on ARM |
… FileSourceScan partition filters
|
Run Gluten ClickHouse CI on ARM |
|
Run Gluten ClickHouse CI on ARM |
|
Run Gluten ClickHouse CI on ARM |
| scanExec.output, | ||
| scanExec.requiredSchema, | ||
| scanExec.partitionFilters, | ||
| partitionFilters, |
There was a problem hiding this comment.
Do you need to consider this issue also for the BatchScanTransformer? Thanks.
There was a problem hiding this comment.
Do you need to consider this issue also for the BatchScanTransformer? Thanks.
I will try to add a unit test to cover this case
There was a problem hiding this comment.
Do you need to consider this issue also for the BatchScanTransformer? Thanks.
There are no partition filters in BatchScanExecTransformer, and runtime filters will also be converted to source v2 predicates, so there is no similar issue for ``BatchScanExecTransformer`.
|
Run Gluten ClickHouse CI on ARM |
|
Run Gluten ClickHouse CI on ARM |
|
Duplicate of #6650 , cc @WangGuangxin @zhztheplayer Could you please take a look? |
gluten-ut/spark35/src/test/scala/org/apache/spark/sql/GlutenInjectRuntimeFilterSuite.scala
Outdated
Show resolved
Hide resolved
|
Run Gluten ClickHouse CI on ARM |
|
Cc @WangGuangxin as he made a fix on similar issue before #6652 |
zhztheplayer
left a comment
zhztheplayer
left a comment
There was a problem hiding this comment.
Give me a couple of minutes to check this. Thanks!
There was a problem hiding this comment.
I haven't had a simple solution finalized so feel free to proceed.
@wForget If we exclude the might_contain from partition filters, we may lose the advantage brought by Spark bloom filters. No?
Yes, this will indeed cause performance regression, but the bloomFilterData that has been constructed by VeloxBloomFilter cannot be used in the driver unless we use jni to call velox in the driver. |
|
I would suggest to implement spark native bloom filter in velox, then we can remove this rule. |
It's a good idea while the challenge is we need to maintain the consistency among Spark versions and Velox. I.e., once Spark updates its bloom filter algorithm, we have to add a copy of that algorithm to Velox. Maybe we can first leave an abstraction layer in the C++ implementation for different Spark versions. |
|
Run Gluten ClickHouse CI on ARM |
| * Spark so produces different intermediate aggregate data. Thus we use different filter function / | ||
| * agg function types for Velox's version to distinguish from vanilla Spark's implementation. | ||
| * | ||
| * FIXME: Remove GlutenTaskOnlyExpression after the VeloxBloomFilter expr is made compatible with |
There was a problem hiding this comment.
@zhztheplayer @jinchengchenghh I added a comment to record this improvement, how about we merge this PR first?
There was a problem hiding this comment.
No problem from my end. cc @jinchengchenghh
And can we log an issue highlighting the performance gap because of the unavailability of scan + might_contain?
Also I guess vanilla Spark's scan + Velox might_contain will cause the issue as well?
There was a problem hiding this comment.
Also I guess vanilla Spark's scan + Velox might_contain will cause the issue as well?
In what scenarios does a vanilla spark scan with native expressions occur? Do we have an existing test case?
There was a problem hiding this comment.
I meant when velox_might_contain is included in a vanilla Spark scan node, which for whatever reason was fallen back by Gluten. I was just guessing whether the same issue will happen in the case.
There was a problem hiding this comment.
I meant when velox_might_contain is included in a vanilla Spark scan node, which for whatever reason was fallen back by Gluten. I was just guessing whether the same issue will happen in the case.
Oh, I see.
There was a problem hiding this comment.
And can we log an issue highlighting the performance gap because of the unavailability of scan + might_contain?
Filed an issue #10071
|
By the way, the current scenario may be uncommon. |
|
@zhztheplayer @jinchengchenghh @WangGuangxin @zhouyuan Thank you for your review, I will merge this pr later if there is no further discussion. |
|
Thanks, merged to main |
@jinchengchenghh The Spark community just accepted a V2 bloom filter implementation apache/spark#50933. |
6 participants
What changes were proposed in this pull request?
Exclude VeloxBloomFilterMightContain from FileSourceScan parition filters
Fixes: #9849
How was this patch tested?
added unit test