[SPARK-34538][SQL] Hive Metastore support filter by not-in #31646

ulysses-you · 2021-02-25T12:29:28Z

What changes were proposed in this pull request?

Add Not(In) and Not(InSet) pattern when convert filter to metastore.

Why are the changes needed?

NOT IN is a useful condition to prune partition, it would be better to support it.

Technically, we can convert c not in(x,y) to c != x and c != y, then push it to metastore.

Avoid metastore overflow and respect the config spark.sql.hive.metastorePartitionPruningInSetThreshold, Not(InSet) won't push to metastore if it's value exceeds the threshold.

Does this PR introduce any user-facing change?

No.

How was this patch tested?

Add test.

wangyum · 2021-02-25T13:14:00Z

sql/hive/src/main/scala/org/apache/spark/sql/hive/client/HiveShim.scala

@@ -748,6 +748,10 @@ private[client] class Shim_v0_13 extends Shim_v0_12 {
      values.map(value => s"$name = $value").mkString("(", " or ", ")")
    }

+    def convertNotInToAnd(name: String, values: Seq[String]): String = {
+      values.map(value => s"$name != $value").mkString("(", " and ", ")")


More than 10,000 values will cause the Hive Metastore stack overflow.

How about stoping push it If it's values size exceeds the threshold ? In this case it can not be covert like >= and <=.

SparkQA · 2021-02-25T13:31:41Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/40045/

SparkQA · 2021-02-25T13:59:57Z

Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/40045/

SparkQA · 2021-02-25T14:03:08Z

Test build #135465 has finished for PR 31646 at commit 3f95df6.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

c21 · 2021-02-25T19:58:13Z

sql/hive/src/test/scala/org/apache/spark/sql/hive/client/FiltersSuite.scala

@@ -108,6 +108,28 @@ class FiltersSuite extends SparkFunSuite with Logging with PlanTest {
    (a("datecol", DateType) =!= Literal(Date.valueOf("2019-01-01"))) :: Nil,
    "datecol != 2019-01-01")

+  filterTest("not-in int filter",


Shall we add a test case for NULL as while, e.g. NOT IN (1, 2, ..., NULL)? Given we have bug before.

Yes it should be, add the null value for test.

Seems exists some issue about null value. Created #31659.

SparkQA · 2021-02-26T05:15:07Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/40068/

SparkQA · 2021-02-26T05:44:23Z

Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/40068/

SparkQA · 2021-02-26T06:10:13Z

Test build #135487 has finished for PR 31646 at commit cf3ba56.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

…4538

SparkQA · 2021-02-26T17:38:32Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/40095/

SparkQA · 2021-02-26T18:07:36Z

Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/40095/

SparkQA · 2021-02-26T21:22:16Z

Test build #135514 has finished for PR 31646 at commit 0415448.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
sealed trait PartitionSpec extends LeafExpression with Unevaluable
trait V2PartitionCommand extends Command
case class TruncateTable(table: LogicalPlan) extends Command
case class TruncatePartition(
case class TruncatePartitionExec(

ulysses-you · 2021-03-01T01:36:55Z

@maropu @cloud-fan Do you have time to take a look this, thanks!

ulysses-you · 2021-03-05T05:05:47Z

retest this please

SparkQA · 2021-03-05T06:23:56Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/40376/

SparkQA · 2021-03-05T06:52:44Z

Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/40376/

SparkQA · 2021-03-05T09:49:34Z

Test build #135794 has finished for PR 31646 at commit 0415448.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
sealed trait PartitionSpec extends LeafExpression with Unevaluable
trait V2PartitionCommand extends Command
case class TruncateTable(table: LogicalPlan) extends Command
case class TruncatePartition(
case class TruncatePartitionExec(

cloud-fan · 2021-03-09T14:50:33Z

sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala

@@ -863,7 +863,9 @@ object SQLConf {
      .doc("The threshold of set size for InSet predicate when pruning partitions through Hive " +
        "Metastore. When the set size exceeds the threshold, we rewrite the InSet predicate " +
        "to be greater than or equal to the minimum value in set and less than or equal to the " +
-        "maximum value in set. Larger values may cause Hive Metastore stack overflow.")
+        "maximum value in set. Larger values may cause Hive Metastore stack overflow. But for " +
+        "the predicate of Not InSet which values exceeds the threshold, we won't to push it " +


won't to push -> won't push

cloud-fan · 2021-03-09T14:57:32Z

sql/hive/src/main/scala/org/apache/spark/sql/hive/client/HiveShim.scala

@@ -767,6 +771,10 @@ private[client] class Shim_v0_13 extends Shim_v0_12 {
          if useAdvanced =>
        Some(convertInToOr(name, values))

+      case Not(In(ExtractAttribute(SupportedAttribute(name)), ExtractableLiterals(values)))


Not(a IN (null, 2)): if a = 1, the final result is null.
a != 2: if a = 1, the final result is true

I think this rewrite is incorrect. We need to make sure the values of IN are all not null.

My bad, missed this. For Not(InSet) case, null value can not be skip safely, it should convert to and(xx != null).

cloud-fan · 2021-03-10T04:39:48Z

sql/hive/src/main/scala/org/apache/spark/sql/hive/client/HiveShim.scala

@@ -775,14 +791,25 @@ private[client] class Shim_v0_13 extends Shim_v0_12 {
        convert(And(GreaterThanOrEqual(child, Literal(sortedValues.head, dataType)),
          LessThanOrEqual(child, Literal(sortedValues.last, dataType))))

+      case Not(InSet(_, values)) if values.size > inSetThreshold =>


can we move this to the beginning so that unsupported cases are grouped together?

sql/hive/src/test/scala/org/apache/spark/sql/hive/client/FiltersSuite.scala

SparkQA · 2021-03-10T06:01:17Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/40505/

SparkQA · 2021-03-10T06:05:26Z

Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/40505/

SparkQA · 2021-03-10T08:37:19Z

Test build #135915 has finished for PR 31646 at commit 93eedd3.

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

SparkQA · 2021-03-10T09:53:45Z

Test build #135922 has finished for PR 31646 at commit e8c7b6c.

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

cloud-fan · 2021-03-10T12:22:50Z

sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala

@@ -863,7 +863,9 @@ object SQLConf {
      .doc("The threshold of set size for InSet predicate when pruning partitions through Hive " +
        "Metastore. When the set size exceeds the threshold, we rewrite the InSet predicate " +
        "to be greater than or equal to the minimum value in set and less than or equal to the " +
-        "maximum value in set. Larger values may cause Hive Metastore stack overflow.")
+        "maximum value in set. Larger values may cause Hive Metastore stack overflow. But for " +
+        "the predicate of Not InSet which values exceeds the threshold, we won't push it to " +


But for InSet inside Not with values exceeding ...

sql/hive/src/test/scala/org/apache/spark/sql/hive/client/FiltersSuite.scala

cloud-fan · 2021-03-10T12:28:46Z

sql/hive/src/test/scala/org/apache/spark/sql/hive/client/HivePartitionFilteringSuite.scala

@@ -418,6 +418,52 @@ class HivePartitionFilteringSuite(version: String)
      dateStrValue)
  }

+  test("getPartitionsByFilter: not in chunk") {


This is the effective test that can catch correctness bugs. Does it test null?

The coverage of previous test is not good. I rewrite the test and include you mentioned.

cloud-fan · 2021-03-11T08:49:27Z

sql/hive/src/test/scala/org/apache/spark/sql/hive/client/HivePartitionFilteringSuite.scala

+    )
+    check(
+      Not(In(attr("chunk"), Seq(Literal("aa"), Literal("ab"), Literal(null)))),
+      chunkValue


yea, this test can detect the correctness bug about null handling.

cloud-fan · 2021-03-11T08:51:33Z

sql/hive/src/test/scala/org/apache/spark/sql/hive/client/HivePartitionFilteringSuite.scala

+    )
+  }
+
+  test("getPartitionsByFilter: not in/inset int type") {


I don't think int and string have much difference about this new feature. It should be sufficient to test string type and date type. Same to the FiltersSuite

Removed these.

SparkQA · 2021-03-11T14:13:09Z

Test build #135968 has finished for PR 31646 at commit 17aea47.

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

cloud-fan · 2021-03-11T15:19:44Z

thanks, merging to master!

ulysses-you · 2021-03-12T01:16:08Z

thanks for review and merging !

Add `Not(In)` and `Not(InSet)` pattern when convert filter to metastore. `NOT IN` is a useful condition to prune partition, it would be better to support it. Technically, we can convert `c not in(x,y)` to `c != x and c != y`, then push it to metastore. Avoid metastore overflow and respect the config `spark.sql.hive.metastorePartitionPruningInSetThreshold`, `Not(InSet)` won't push to metastore if it's value exceeds the threshold. No. Add test. Closes apache#31646 from ulysses-you/SPARK-34538. Authored-by: ulysses-you <ulyssesyou18@gmail.com> Signed-off-by: Wenchen Fan <wenchen@databricks.com> (cherry picked from commit 744a73d)

init

3f95df6

ulysses-you changed the title ~~[SPARK-33537][SQL] Hive Metastore support filter by not-in~~ [SPARK-34538][SQL] Hive Metastore support filter by not-in Feb 25, 2021

wangyum reviewed Feb 25, 2021

View reviewed changes

github-actions bot added the SQL label Feb 25, 2021

c21 reviewed Feb 25, 2021

View reviewed changes

ulysses-you added 4 commits February 26, 2021 11:19

fix test

3cefafe

add null test

0dc1c4a

avoid metastore stack over flow

b9c7f44

nit

cf3ba56

Merge branch 'master' of https://github.com/apache/spark into SPARK-3…

0415448

…4538

cloud-fan reviewed Mar 9, 2021

View reviewed changes

ulysses-you added 4 commits March 10, 2021 09:09

doc

113bd62

add null precheck

01b7466

test

e37f5f9

pattern

93eedd3

cloud-fan reviewed Mar 10, 2021

View reviewed changes

sql/hive/src/test/scala/org/apache/spark/sql/hive/client/FiltersSuite.scala Show resolved Hide resolved

move forward

e8c7b6c

cloud-fan reviewed Mar 10, 2021

View reviewed changes

sql/hive/src/test/scala/org/apache/spark/sql/hive/client/FiltersSuite.scala Show resolved Hide resolved

cloud-fan reviewed Mar 10, 2021

View reviewed changes

ulysses-you added 3 commits March 10, 2021 20:36

doc

062b1d4

not in datetype suite

4704ec3

HivePartitionFilteringSuite

5acbf6a

cloud-fan reviewed Mar 11, 2021

View reviewed changes

cloud-fan approved these changes Mar 11, 2021

View reviewed changes

remove int test

17aea47

cloud-fan closed this in 744a73d Mar 11, 2021

ulysses-you deleted the SPARK-34538 branch March 12, 2021 01:16

[SPARK-34538][SQL] Hive Metastore support filter by not-in #31646

[SPARK-34538][SQL] Hive Metastore support filter by not-in #31646

Conversation

ulysses-you commented Feb 25, 2021 • edited Loading

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?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SparkQA commented Feb 25, 2021

SparkQA commented Feb 25, 2021

SparkQA commented Feb 25, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SparkQA commented Feb 26, 2021

SparkQA commented Feb 26, 2021

SparkQA commented Feb 26, 2021

SparkQA commented Feb 26, 2021

SparkQA commented Feb 26, 2021

SparkQA commented Feb 26, 2021

ulysses-you commented Mar 1, 2021

ulysses-you commented Mar 5, 2021

SparkQA commented Mar 5, 2021

SparkQA commented Mar 5, 2021

SparkQA commented Mar 5, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ulysses-you Mar 10, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SparkQA commented Mar 10, 2021

SparkQA commented Mar 10, 2021

SparkQA commented Mar 10, 2021

SparkQA commented Mar 10, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cloud-fan Mar 11, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SparkQA commented Mar 11, 2021

cloud-fan commented Mar 11, 2021

ulysses-you commented Mar 12, 2021

ulysses-you commented Feb 25, 2021 •

edited

Loading

ulysses-you Mar 10, 2021 •

edited

Loading

cloud-fan Mar 11, 2021 •

edited

Loading