[VL] Make bloom_filter_agg fall back when might_contain is not transformable #3917

zhli1142015 · 2023-12-04T15:27:50Z

What changes were proposed in this pull request?

Fallback bloom_filter_agg when might_contain fall back, either because of might_contain self or other expressions in same operator. This fixes below error:

java.io.IOException: Unexpected Bloom filter version number (16777217)
  at org.apache.spark.util.sketch.BloomFilterImpl.readFrom0(BloomFilterImpl.java:256)
  at org.apache.spark.util.sketch.BloomFilterImpl.readFrom(BloomFilterImpl.java:265)
  at org.apache.spark.util.sketch.BloomFilter.readFrom(BloomFilter.java:178)
  at org.apache.spark.sql.catalyst.expressions.BloomFilterMightContain.deserialize(BloomFilterMightContain.scala:111)

How was this patch tested?

UT.

github-actions · 2023-12-04T15:28:11Z

Thanks for opening a pull request!

Could you open an issue for this pull request on Github Issues?

https://github.com/oap-project/gluten/issues

Then could you also rename commit message and pull request title in the following format?

[GLUTEN-${ISSUES_ID}][COMPONENT]feat/fix: ${detailed message}

See also:

Other pull requests

github-actions · 2023-12-04T15:28:25Z

Run Gluten Clickhouse CI

zhli1142015 · 2023-12-04T23:29:00Z

@JkSelf , @PHILO-HE , @jinchengchenghh , coudl you help to review this?
Thanks.

PHILO-HE

Thanks for your work! Just some suggestions.

PHILO-HE · 2023-12-05T06:31:39Z

gluten-core/src/main/scala/io/glutenproject/extension/columnar/TransformHintRule.scala

@@ -714,6 +714,13 @@ case class AddTransformHintRule() extends Rule[SparkPlan] {
          s"${e.getMessage}, original sparkplan is " +
            s"${plan.getClass}(${plan.children.toList.map(_.getClass)})")
    }
+
+    if (TransformHints.isAlreadyTagged(plan) && TransformHints.isNotTransformable(plan)) {


Maybe, we can move this part into a dedicated rule after AddTransformHintRule (see link).

Thanks, moved to a separate rule.

PHILO-HE · 2023-12-05T06:43:19Z

shims/spark34/src/main/scala/io/glutenproject/sql/shims/spark34/Spark34Shims.scala

@@ -130,6 +130,33 @@ class Spark34Shims extends SparkShims {
      @transient locations: Array[String] = Array.empty): PartitionedFile =
    PartitionedFile(partitionValues, SparkPath.fromPathString(filePath), start, length, locations)

+  override def handleBloomFilterFallback(plan: SparkPlan)(fun: SparkPlan => Unit): Unit = {


This method looks repeated. If so, maybe, we can just move this method into a common place. But just move the check for BloomFilterAggregate into shims, as I note it doesn't exist in spark3.2.

Spark 3.3/3.4:

def hasBloomFilterAggregate(agg): Boolean = { agg.aggregateExpressions.exists( expr.aggregateFunction.isInstanceOf[BloomFilterAggregate]) }

Spark 3.2:

def hasBloomFilterAggregate(agg): Boolean = { false }

Updated, thanks,

PHILO-HE · 2023-12-05T07:08:54Z

Also note this doc: https://github.com/oap-project/gluten/blob/main/docs/velox-backend-limitations.md#runtime-bloomfilter. With this fix, we need to document the behavior into fallback section. Thanks!

github-actions · 2023-12-05T07:52:02Z

Run Gluten Clickhouse CI

zhli1142015 · 2023-12-05T07:54:24Z

Also note this doc: https://github.com/oap-project/gluten/blob/main/docs/velox-backend-limitations.md#runtime-bloomfilter. With this fix, we need to document the behavior into fallback section. Thanks!

Thanks, updated the doc.

github-actions · 2023-12-05T07:57:57Z

Run Gluten Clickhouse CI

PHILO-HE · 2023-12-05T08:25:23Z

gluten-core/src/main/scala/io/glutenproject/extension/columnar/TransformHintRule.scala

+ * need fall back related bloom filter agg.
+ */
+case class FallbackBloomFilterAggIfNeeded() extends Rule[SparkPlan] {
+  override def apply(plan: SparkPlan): SparkPlan = plan.transformDown {


Looks good! Maybe, we can skip the handling and just return the original plan if spark.gluten.sql.native.bloomFilter=false. Right?

Make sense, thanks.

github-actions · 2023-12-05T08:44:50Z

Run Gluten Clickhouse CI

github-actions · 2023-12-05T08:53:29Z

Run Gluten Clickhouse CI

PHILO-HE

Looks good! Thanks!

JkSelf

LGTM. Thanks @zhli1142015 @PHILO-HE .

ulysses-you · 2023-12-06T03:23:50Z

Hi all, there is a conflict between this pr and #3843. I sent a #3940 to fix it.

…ormable (apache#3917)

…t transformable (apache#3917)" This reverts commit d7d8e28.

…t transformable (#3917)" (#3977) * Revert "[VL] Make bloom_filter_agg fall back when might_contain is not transformable (#3917)" This reverts commit d7d8e28. * Revert "[GLUTEN-3917][FOLLOWUP] Add back SparkShimLoader import (#3940)" This reverts commit 81bb6c9.

[VL] fallback bloom_filter_agg when might_contain is fallbacked

119062f

zhli1142015 marked this pull request as ready for review December 4, 2023 23:28

zhli1142015 changed the title ~~[WIP][VL] fallback bloom_filter_agg when might_contain is fallbacked~~ [VL] fallback bloom_filter_agg when might_contain is fallbacked Dec 4, 2023

PHILO-HE reviewed Dec 5, 2023

View reviewed changes

PHILO-HE changed the title ~~[VL] fallback bloom_filter_agg when might_contain is fallbacked~~ [VL] Make bloom_filter_agg fall back when might_contain is not transformable Dec 5, 2023

address comments

f04d6d1

update doc

a0fba76

zhli1142015 requested a review from PHILO-HE December 5, 2023 08:11

PHILO-HE reviewed Dec 5, 2023

View reviewed changes

address comment

03f4570

minor fix

4e62718

zhli1142015 requested a review from PHILO-HE December 5, 2023 11:31

PHILO-HE approved these changes Dec 6, 2023

View reviewed changes

JkSelf approved these changes Dec 6, 2023

View reviewed changes

JkSelf merged commit d7d8e28 into apache:main Dec 6, 2023
17 checks passed

github-actions bot mentioned this pull request Dec 6, 2023

[GLUTEN-3917][FOLLOWUP] Add back SparkShimLoader import #3940

Merged

loneylee pushed a commit to loneylee/gluten that referenced this pull request Dec 8, 2023

[VL] Make bloom_filter_agg fall back when might_contain is not transf…

124acb9

…ormable (apache#3917)

loneylee added a commit to loneylee/gluten that referenced this pull request Dec 8, 2023

Revert "[VL] Make bloom_filter_agg fall back when might_contain is no…

f340493

…t transformable (apache#3917)" This reverts commit d7d8e28.

loneylee added a commit to loneylee/gluten that referenced this pull request Dec 8, 2023

Revert "[VL] Make bloom_filter_agg fall back when might_contain is no…

c0c7e99

…t transformable (apache#3917)" This reverts commit d7d8e28.

loneylee mentioned this pull request Dec 8, 2023

Revert "[VL] Make bloom_filter_agg fall back when might_contain is not transformable (#3917)" #3977

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[VL] Make bloom_filter_agg fall back when might_contain is not transformable #3917

[VL] Make bloom_filter_agg fall back when might_contain is not transformable #3917

zhli1142015 commented Dec 4, 2023 •

edited

github-actions bot commented Dec 4, 2023

github-actions bot commented Dec 4, 2023

zhli1142015 commented Dec 4, 2023

PHILO-HE left a comment

PHILO-HE Dec 5, 2023

zhli1142015 Dec 5, 2023

PHILO-HE Dec 5, 2023

zhli1142015 Dec 5, 2023

PHILO-HE commented Dec 5, 2023

github-actions bot commented Dec 5, 2023

zhli1142015 commented Dec 5, 2023

github-actions bot commented Dec 5, 2023

PHILO-HE Dec 5, 2023

zhli1142015 Dec 5, 2023

github-actions bot commented Dec 5, 2023

github-actions bot commented Dec 5, 2023

PHILO-HE left a comment

JkSelf left a comment

ulysses-you commented Dec 6, 2023

[VL] Make bloom_filter_agg fall back when might_contain is not transformable #3917

[VL] Make bloom_filter_agg fall back when might_contain is not transformable #3917

Conversation

zhli1142015 commented Dec 4, 2023 • edited

What changes were proposed in this pull request?

How was this patch tested?

github-actions bot commented Dec 4, 2023

github-actions bot commented Dec 4, 2023

zhli1142015 commented Dec 4, 2023

PHILO-HE left a comment

Choose a reason for hiding this comment

PHILO-HE Dec 5, 2023

Choose a reason for hiding this comment

zhli1142015 Dec 5, 2023

Choose a reason for hiding this comment

PHILO-HE Dec 5, 2023

Choose a reason for hiding this comment

zhli1142015 Dec 5, 2023

Choose a reason for hiding this comment

PHILO-HE commented Dec 5, 2023

github-actions bot commented Dec 5, 2023

zhli1142015 commented Dec 5, 2023

github-actions bot commented Dec 5, 2023

PHILO-HE Dec 5, 2023

Choose a reason for hiding this comment

zhli1142015 Dec 5, 2023

Choose a reason for hiding this comment

github-actions bot commented Dec 5, 2023

github-actions bot commented Dec 5, 2023

PHILO-HE left a comment

Choose a reason for hiding this comment

JkSelf left a comment

Choose a reason for hiding this comment

ulysses-you commented Dec 6, 2023

zhli1142015 commented Dec 4, 2023 •

edited