[SPARK-36706][SQL][3.1] OverwriteByExpression conversion in DataSourceV2Strategy use wrong param in translateFilter #33997

huaxingao · 2021-09-14T21:15:28Z

What changes were proposed in this pull request?

The wrong parameter is used in translateFilter in the following code

      val filters = splitConjunctivePredicates(deleteExpr).map {
        filter => DataSourceStrategy.translateFilter(deleteExpr,
          supportNestedPredicatePushdown = true).getOrElse(
            throw new AnalysisException(s"Cannot translate expression to source filter: $filter"))
      }.toArray

Using this as an example

spark.table("source2_t").writeTo("testcat.table_name").overwrite($"id1" === 3 && $"id2" === 3)

The above code will generate these filters:

And(EqualTo(id1, 3),EqualTo(id2, 3))  
And(EqualTo(id1, 3),EqualTo(id2, 3))

we want to fix the code so it will generate the filters like these:

EqualTo(id1, 3)  
EqualTo(id2, 3)

This problem only exists in 3.1. In 3.2 and 3.3, we have

      val filters = splitConjunctivePredicates(deleteExpr).flatMap { pred =>
        val filter = DataSourceStrategy.translateFilter(pred, supportNestedPredicatePushdown = true)
        if (filter.isEmpty) {
          throw QueryCompilationErrors.cannotTranslateExpressionToSourceFilterError(pred)
        }
        filter
      }.toArray

Why are the changes needed?

fix a bug in the code

Does this PR introduce any user-facing change?

no

How was this patch tested?

existing tests

…rategy use wrong deleteExpr translation

viirya · 2021-09-14T21:53:31Z

sql/core/src/test/scala/org/apache/spark/sql/DataFrameWriterV2Suite.scala

@@ -220,6 +220,33 @@ class DataFrameWriterV2Suite extends QueryTest with SharedSparkSession with Befo
      Seq(Row(1L, "a"), Row(2L, "b"), Row(4L, "d"), Row(5L, "e"), Row(6L, "f")))
  }

+  test("Overwrite: overwrite by expression: more than one filters") {


Add JIRA number?

Added Jira number and also updated PR description

viirya

The fix looks correct.

SparkQA · 2021-09-14T22:27:27Z

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

SparkQA · 2021-09-14T22:35:01Z

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

SparkQA · 2021-09-14T23:22:25Z

Test build #143280 has finished for PR 33997 at commit 954beb3.

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

SparkQA · 2021-09-14T23:47:32Z

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

SparkQA · 2021-09-14T23:54:58Z

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

SparkQA · 2021-09-15T00:32:31Z

Test build #143284 has finished for PR 33997 at commit ffb4f3a.

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

viirya · 2021-09-15T00:58:02Z

cc @sunchao @dongjoon-hyun

viirya · 2021-09-15T00:58:34Z

@huaxingao Could you also mention the reason why we only need fix it for 3.1 too in the description? Thanks.

SparkQA · 2021-09-15T01:36:12Z

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

SparkQA · 2021-09-15T01:43:27Z

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

SparkQA · 2021-09-15T04:58:22Z

Test build #143287 has finished for PR 33997 at commit 45b20c0.

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

sunchao · 2021-09-15T05:51:31Z

sql/core/src/test/scala/org/apache/spark/sql/DataFrameWriterV2Suite.scala

@@ -220,6 +220,33 @@ class DataFrameWriterV2Suite extends QueryTest with SharedSparkSession with Befo
      Seq(Row(1L, "a"), Row(2L, "b"), Row(4L, "d"), Row(5L, "e"), Row(6L, "f")))
  }

+  test("SPARK-36706 Overwrite: overwrite by expression: more than one filters") {


should we test that the filters are changed? it seems we only check query answers but I think they should pass no matter with or without the fix?

Yes, the test would pass without the fix. but couldn't figure out an easy way to get the OverwriteByExpressionExec node and check the filters on it.

dongjoon-hyun · 2021-09-20T18:14:06Z

Hi, @huaxingao .
Shall we remove the test case and merge this PR first?
I believe we can add the test case later when you are ready.

dongjoon-hyun

+1, LGTM. Thank you, @huaxingao , @viirya , @sunchao .
Merged to branch-3.1.

…eV2Strategy use wrong param in translateFilter ### What changes were proposed in this pull request? The wrong parameter is used in `translateFilter` in the following code ``` val filters = splitConjunctivePredicates(deleteExpr).map { filter => DataSourceStrategy.translateFilter(deleteExpr, supportNestedPredicatePushdown = true).getOrElse( throw new AnalysisException(s"Cannot translate expression to source filter: $filter")) }.toArray ``` Using this as an example ``` spark.table("source2_t").writeTo("testcat.table_name").overwrite($"id1" === 3 && $"id2" === 3) ``` The above code will generate these filters: ``` And(EqualTo(id1, 3),EqualTo(id2, 3)) And(EqualTo(id1, 3),EqualTo(id2, 3)) ``` we want to fix the code so it will generate the filters like these: ``` EqualTo(id1, 3) EqualTo(id2, 3) ``` This problem only exists in 3.1. In 3.2 and 3.3, we have ``` val filters = splitConjunctivePredicates(deleteExpr).flatMap { pred => val filter = DataSourceStrategy.translateFilter(pred, supportNestedPredicatePushdown = true) if (filter.isEmpty) { throw QueryCompilationErrors.cannotTranslateExpressionToSourceFilterError(pred) } filter }.toArray ``` ### Why are the changes needed? fix a bug in the code ### Does this PR introduce _any_ user-facing change? no ### How was this patch tested? existing tests Closes #33997 from huaxingao/spark-36706. Authored-by: Huaxin Gao <huaxin_gao@apple.com> Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>

huaxingao · 2021-09-20T20:28:05Z

Thanks a lot! @dongjoon-hyun @sunchao @viirya

SparkQA · 2021-09-20T21:09:04Z

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

SparkQA · 2021-09-20T21:16:23Z

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

…eV2Strategy use wrong param in translateFilter ### What changes were proposed in this pull request? The wrong parameter is used in `translateFilter` in the following code ``` val filters = splitConjunctivePredicates(deleteExpr).map { filter => DataSourceStrategy.translateFilter(deleteExpr, supportNestedPredicatePushdown = true).getOrElse( throw new AnalysisException(s"Cannot translate expression to source filter: $filter")) }.toArray ``` Using this as an example ``` spark.table("source2_t").writeTo("testcat.table_name").overwrite($"id1" === 3 && $"id2" === 3) ``` The above code will generate these filters: ``` And(EqualTo(id1, 3),EqualTo(id2, 3)) And(EqualTo(id1, 3),EqualTo(id2, 3)) ``` we want to fix the code so it will generate the filters like these: ``` EqualTo(id1, 3) EqualTo(id2, 3) ``` This problem only exists in 3.1. In 3.2 and 3.3, we have ``` val filters = splitConjunctivePredicates(deleteExpr).flatMap { pred => val filter = DataSourceStrategy.translateFilter(pred, supportNestedPredicatePushdown = true) if (filter.isEmpty) { throw QueryCompilationErrors.cannotTranslateExpressionToSourceFilterError(pred) } filter }.toArray ``` ### Why are the changes needed? fix a bug in the code ### Does this PR introduce _any_ user-facing change? no ### How was this patch tested? existing tests Closes apache#33997 from huaxingao/spark-36706. Authored-by: Huaxin Gao <huaxin_gao@apple.com> Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>

huaxingao added 2 commits September 13, 2021 22:47

[SPARK-36706][SQL] OverwriteByExpression conversion in DataSourceV2St…

bec661e

…rategy use wrong deleteExpr translation

add UT

954beb3

github-actions bot added the SQL label Sep 14, 2021

viirya reviewed Sep 14, 2021

View reviewed changes

add jira num in UT

ffb4f3a

fix UT

45b20c0

viirya approved these changes Sep 15, 2021

View reviewed changes

sunchao reviewed Sep 15, 2021

View reviewed changes

remove added test

1d82af7

dongjoon-hyun approved these changes Sep 20, 2021

View reviewed changes

dongjoon-hyun closed this Sep 20, 2021

huaxingao deleted the spark-36706 branch September 20, 2021 20:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-36706][SQL][3.1] OverwriteByExpression conversion in DataSourceV2Strategy use wrong param in translateFilter #33997

[SPARK-36706][SQL][3.1] OverwriteByExpression conversion in DataSourceV2Strategy use wrong param in translateFilter #33997

huaxingao commented Sep 14, 2021 •

edited

viirya Sep 14, 2021

huaxingao Sep 14, 2021

viirya left a comment

SparkQA commented Sep 14, 2021

SparkQA commented Sep 14, 2021

SparkQA commented Sep 14, 2021

SparkQA commented Sep 14, 2021

SparkQA commented Sep 14, 2021

SparkQA commented Sep 15, 2021

viirya commented Sep 15, 2021

viirya commented Sep 15, 2021 •

edited

SparkQA commented Sep 15, 2021

SparkQA commented Sep 15, 2021

SparkQA commented Sep 15, 2021

sunchao Sep 15, 2021

huaxingao Sep 15, 2021

dongjoon-hyun commented Sep 20, 2021

dongjoon-hyun left a comment

huaxingao commented Sep 20, 2021

SparkQA commented Sep 20, 2021

SparkQA commented Sep 20, 2021

[SPARK-36706][SQL][3.1] OverwriteByExpression conversion in DataSourceV2Strategy use wrong param in translateFilter #33997

[SPARK-36706][SQL][3.1] OverwriteByExpression conversion in DataSourceV2Strategy use wrong param in translateFilter #33997

Conversation

huaxingao commented Sep 14, 2021 • edited

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?

viirya Sep 14, 2021

Choose a reason for hiding this comment

huaxingao Sep 14, 2021

Choose a reason for hiding this comment

viirya left a comment

Choose a reason for hiding this comment

SparkQA commented Sep 14, 2021

SparkQA commented Sep 14, 2021

SparkQA commented Sep 14, 2021

SparkQA commented Sep 14, 2021

SparkQA commented Sep 14, 2021

SparkQA commented Sep 15, 2021

viirya commented Sep 15, 2021

viirya commented Sep 15, 2021 • edited

SparkQA commented Sep 15, 2021

SparkQA commented Sep 15, 2021

SparkQA commented Sep 15, 2021

sunchao Sep 15, 2021

Choose a reason for hiding this comment

huaxingao Sep 15, 2021

Choose a reason for hiding this comment

dongjoon-hyun commented Sep 20, 2021

dongjoon-hyun left a comment

Choose a reason for hiding this comment

huaxingao commented Sep 20, 2021

SparkQA commented Sep 20, 2021

SparkQA commented Sep 20, 2021

huaxingao commented Sep 14, 2021 •

edited

viirya commented Sep 15, 2021 •

edited