[SPARK-29968][SQL] Remove the Predicate code from SparkPlan #26604

maropu · 2019-11-20T03:03:26Z

What changes were proposed in this pull request?

This is to refactor Predicate code; it mainly removed newPredicate from SparkPlan.
Modifications are listed below;

Move Predicate from o.a.s.sqlcatalyst.expressions.codegen.GeneratePredicate.scala to o.a.s.sqlcatalyst.expressions.predicates.scala
To resolve the name conflict, rename o.a.s.sqlcatalyst.expressions.codegen.Predicate to o.a.s.sqlcatalyst.expressions.BasePredicate
Extend CodeGeneratorWithInterpretedFallback for BasePredicate

This comes from the @cloud-fan suggestion: #26420 (comment)

Why are the changes needed?

For better code/test coverage.

Does this PR introduce any user-facing change?

No.

How was this patch tested?

Existing tests.

maropu · 2019-11-20T03:04:09Z

@cloud-fan @viirya How about this refactoring?

SparkQA · 2019-11-20T03:06:03Z

Test build #114125 has finished for PR 26604 at commit fcc9d22.

This patch fails Scala style tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
class SpecificPredicate extends $
abstract class BasePredicate

maropu · 2019-11-20T03:13:01Z

IIUC newMutableProjection, newOrdering and newNaturalAscendingOrdering can be removed from SparkPlan, too.

dongjoon-hyun · 2019-11-20T04:18:23Z

Hi, @maropu . So, are you going to make a few PR for them one by one?
cc @cloud-fan .

dongjoon-hyun · 2019-11-20T04:18:59Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/Projection.scala


  /**
-   * Returns an MutableProjection for given sequence of Expressions, which will be bound to
+   * Returns a MutableProjection for given sequence of Expressions, which will be bound to


Let's not touch this file in this PR~

You meant a separate PR for this typo?

dongjoon-hyun · 2019-11-20T04:20:48Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/predicates.scala

+/**
+ * Interface for generated/interpreted predicate
+ */
+abstract class BasePredicate {


It seems reasonable because we renamed Predicate => BasePredicate before.

dongjoon-hyun · 2019-11-20T04:22:17Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/predicates.scala



+/**
+ * Interface for generated/interpreted predicate


It's not wrong and this comes from the old comment. Shall we use A base class instead of Interface?

dongjoon-hyun · 2019-11-20T04:29:36Z

sql/core/src/main/scala/org/apache/spark/sql/execution/SparkPlan.scala

-      str
-    }
-    logWarning(s"Codegen disabled for this expression:\n $logMessage")
-    InterpretedPredicate.create(expression, inputSchema)


I didn't follow the context in the previous PR. Is this genInterpretedPredicate function unused?

Yea, CodeGeneratorWithInterpretedFallback#createInterpretedObject does the same thing with genInterpretedPredicate :

spark/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/CodeGeneratorWithInterpretedFallback.scala

Line 63 in 5a70af7

protected def createInterpretedObject(in: IN): OUT

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/predicates.scala

maropu · 2019-11-20T05:01:05Z

Thanks for the comment, @dongjoon-hyun. I just wanted to check if this refactoring is right first, and all the tests passed for this fix because this change might increase the test coverage for interpreted predicates (e.g., in SQLQueryTestSuite). So, I'm ok that all the refactoring is done in this pr. Should I do so?

maropu · 2019-11-20T05:42:43Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/catalog/ExternalCatalogUtils.scala


      val boundPredicate =
-        InterpretedPredicate.create(predicates.reduce(And).transform {
+        Predicate.create(predicates.reduce(And).transform {


Any reason to use the interpreted version here?

Sometimes it's better to use the interpreted version if the input data is known to be small. Codegen can be slower as compiling takes time.

Ah, ok. I'll revert this.

This reverts commit 73588fc.

cloud-fan · 2019-11-20T06:08:06Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/Optimizer.scala

    case Filter(condition, LocalRelation(output, data, isStreaming))
        if !hasUnevaluableExpr(condition) =>
-      val predicate = InterpretedPredicate.create(condition, output)
+      val predicate = Predicate.create(condition, output)


This is to optimize local relation so perf doesn't matter too much. The change should be fine.

cloud-fan · 2019-11-20T06:08:40Z

...e/src/main/scala/org/apache/spark/sql/execution/datasources/PartitioningAwareFileIndex.scala

      val predicate = partitionPruningPredicates.reduce(expressions.And)

-      val boundPredicate = InterpretedPredicate.create(predicate.transform {
+      val boundPredicate = Predicate.create(predicate.transform {


to be safe, I think we should keep using the interpreted version, in case there are only a few partitions.

cloud-fan · 2019-11-20T06:10:27Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/predicates.scala

+  def create(expr: Expression): BasePredicate = {
+    create(expr)
+  }
+


We can add a method def createInterpreted... for places that want to use interpreted predicates.

cloud-fan · 2019-11-20T06:40:06Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/catalog/ExternalCatalogUtils.scala


      val boundPredicate =
-        InterpretedPredicate.create(predicates.reduce(And).transform {
+        Predicate.createInterpretedPredicate(predicates.reduce(And).transform {


nit: double predicate looks weird, how about just Predicate.createInterpreted?

cloud-fan · 2019-11-20T06:42:46Z

sql/hive/src/test/scala/org/apache/spark/sql/sources/SimpleTextRelation.scala

            expressions.GreaterThan(attribute, literal)
        }.reduceOption(expressions.And).getOrElse(Literal(true))
-        InterpretedPredicate.create(filterCondition, inputAttributes)
+        Predicate.createInterpretedPredicate(filterCondition, inputAttributes)


This is a testing source, we don't care it's codegen or not. Should be fine to call Predicate.create

SparkQA · 2019-11-20T06:54:37Z

Test build #114127 has finished for PR 26604 at commit 4b1e311.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
class SpecificPredicate extends $
abstract class BasePredicate

cloud-fan · 2019-11-20T07:49:15Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/predicates.scala

+    InterpretedPredicate(in)
+  }
+
+  def createInterpreted(e: Expression, inputSchema: Seq[Attribute]): InterpretedPredicate =


this method seems not used.

SparkQA · 2019-11-20T08:05:02Z

Test build #114140 has finished for PR 26604 at commit 74183e6.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2019-11-20T08:05:02Z

Test build #114135 has finished for PR 26604 at commit 0896afe.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2019-11-20T08:05:02Z

Test build #114141 has finished for PR 26604 at commit 164507c.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2019-11-20T08:05:02Z

Test build #114129 has finished for PR 26604 at commit 73588fc.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2019-11-20T08:05:02Z

Test build #114132 has finished for PR 26604 at commit bed96f4.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds no public classes.

maropu · 2019-11-20T08:09:47Z

retest this please

SparkQA · 2019-11-20T12:36:18Z

Test build #114142 has finished for PR 26604 at commit 164507c.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

cloud-fan · 2019-11-20T13:14:07Z

thanks, merging to master!

cloud-fan · 2019-11-20T13:15:49Z

@maropu can you create JIRA tickets to do the same thing for newMutableProjection, newOrdering and newNaturalAscendingOrdering?

maropu · 2019-11-20T13:20:33Z

Yea, sure. Better to fix them in a single next PR?

cloud-fan · 2019-11-20T13:25:31Z

sounds good

maropu · 2019-11-20T13:27:34Z

ok

beliefer · 2019-11-21T03:24:42Z

@maropu Thanks for your change . This PR makes my work easier.

newPredicate has been removed in Spark 3.0 apache/spark#26604

Fix

4b1e311

maropu force-pushed the RefactorPredicate branch from fcc9d22 to 4b1e311 Compare November 20, 2019 03:09

dongjoon-hyun reviewed Nov 20, 2019

View reviewed changes

viirya reviewed Nov 20, 2019

View reviewed changes

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/predicates.scala Show resolved Hide resolved

Address reviews

73588fc

maropu commented Nov 20, 2019

View reviewed changes

Revert "Address reviews"

dd3088f

This reverts commit 73588fc.

cloud-fan reviewed Nov 20, 2019

View reviewed changes

Address comments

bed96f4

cloud-fan reviewed Nov 20, 2019

View reviewed changes

maropu force-pushed the RefactorPredicate branch from 37f470e to d10b004 Compare November 20, 2019 06:26

Address more comments

0896afe

maropu force-pushed the RefactorPredicate branch from d10b004 to 0896afe Compare November 20, 2019 06:27

cloud-fan reviewed Nov 20, 2019

View reviewed changes

Fix

74183e6

cloud-fan reviewed Nov 20, 2019

View reviewed changes

Fix

164507c

cloud-fan closed this in 0032d85 Nov 20, 2019

maropu mentioned this pull request Nov 20, 2019

[SPARK-27986][SQL] Support ANSI SQL filter predicate for aggregate expression. #26420

Closed

dongjoon-hyun added the SQL label Feb 5, 2020

changpingc added a commit to changpingc/GeoSpark that referenced this pull request Jul 9, 2020

sql: replace newPredicate with Predicate.create

fe962fe

newPredicate has been removed in Spark 3.0 apache/spark#26604

changpingc added a commit to changpingc/GeoSpark that referenced this pull request Aug 12, 2020

sql: replace newPredicate with Predicate.create

2147804

newPredicate has been removed in Spark 3.0 apache/spark#26604

[SPARK-29968][SQL] Remove the Predicate code from SparkPlan #26604

[SPARK-29968][SQL] Remove the Predicate code from SparkPlan #26604

Uh oh!

Conversation

maropu commented Nov 20, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

Uh oh!

maropu commented Nov 20, 2019

Uh oh!

SparkQA commented Nov 20, 2019

Uh oh!

maropu commented Nov 20, 2019

Uh oh!

dongjoon-hyun commented Nov 20, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maropu Nov 20, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dongjoon-hyun Nov 20, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

maropu commented Nov 20, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Nov 20, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Nov 20, 2019

Uh oh!

SparkQA commented Nov 20, 2019

Uh oh!

SparkQA commented Nov 20, 2019

Uh oh!

SparkQA commented Nov 20, 2019

Uh oh!

SparkQA commented Nov 20, 2019

Uh oh!

maropu commented Nov 20, 2019

Uh oh!

maropu commented Nov 20, 2019 •

edited

Loading

maropu Nov 20, 2019 •

edited

Loading

dongjoon-hyun Nov 20, 2019 •

edited

Loading