[SPARK-41959][SQL] Improve v1 writes with empty2null #39475

ulysses-you · 2023-01-10T04:43:33Z

What changes were proposed in this pull request?

Cleanup some unnecessary Empty2Null related code

Why are the changes needed?

V1Writes checked idempotency using WriteFiles, so it's unnecessary to check if empty2null if exists again.

Does this PR introduce any user-facing change?

no

How was this patch tested?

pass CI

ulysses-you · 2023-01-10T04:44:34Z

sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/V1Writes.scala

@@ -91,32 +92,31 @@ object V1Writes extends Rule[LogicalPlan] with SQLConfHelper {
  }

  private def prepareQuery(write: V1WriteCommand, query: LogicalPlan): LogicalPlan = {
-    val hasEmpty2Null = query.exists(p => hasEmptyToNull(p.expressions))
-    val empty2NullPlan = if (hasEmpty2Null) {


this check has no meaning now since we have already checked WriteFiles for rule idempotency.

ulysses-you · 2023-01-10T04:45:48Z

sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/V1Writes.scala

+    // Here we use the original required ordering to compare if matches so that we can preserve the
+    // user-specified sort ordering with such case:
+    // `INSERT INTO TABLE t PARTITION(p) SELECT c, p FROM t SORT BY p, c`.
+    val requiredOrdering = write.requiredOrdering


this is the key change that use the original required ordering before pulling out Empty2Null

ulysses-you · 2023-01-10T04:45:53Z

cc @cloud-fan @EnricoMi

cloud-fan · 2023-01-10T05:12:22Z

can you update the test added in #39431 ?

ulysses-you · 2023-01-10T05:27:15Z

can you update the test added in #39431 ?

sure, updated

EnricoMi

I can confirm that for the example in #38356 master writes

!WriteFiles
+- *(1) Sort [p#19 ASC NULLS FIRST], false, 0
   +- *(1) Project [id#10, sort_col#11, empty2null(p#12) AS p#19]
      +- ShuffleQueryStage 0
         +- Exchange SinglePartition, REPARTITION_BY_NUM, [plan_id=18]
            +- LocalTableScan [id#10, sort_col#11, p#12]

while this fix writes

!WriteFiles
+- *(1) Project [id#10, sort_col#11, empty2null(p#12) AS p#19]
   +- *(1) Sort [p#12 ASC NULLS FIRST, sort_col#11 ASC NULLS FIRST], false, 0
      +- ShuffleQueryStage 0
         +- Exchange SinglePartition, REPARTITION_BY_NUM, [plan_id=18]
            +- LocalTableScan [id#10, sort_col#11, p#12]

LGTM!

EnricoMi · 2023-01-10T07:05:33Z

sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/V1Writes.scala

-    val requiredOrdering = write.requiredOrdering.map(_.transform {
-      case a: Attribute => attrMap.getOrElse(a, a)
-    }.asInstanceOf[SortOrder])


Looks like I was half way there: #38356 (comment)
Only adding this back into the else branch below to get to the same Sort was missing in my fix.

cloud-fan · 2023-01-10T07:58:54Z

sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/FileFormatWriter.scala

@@ -159,7 +159,6 @@ object FileFormatWriter extends Logging {
    val actualOrdering = writeFilesOpt.map(_.child)
      .getOrElse(materializeAdaptiveSparkPlan(plan))
      .outputOrdering.map(_.child)
-    val orderingMatched = V1WritesUtils.isOrderingMatched(requiredOrdering, actualOrdering)


why this line doesn't work anymore?

We do not add the sort in rule V1Writes since the ordering matches. So the actual ordering is the user-specified which does not rewrite the attribute using the new query that wrapped with Empty2Null. But the required ordering has rewritten in rule V1Writes.

if ordering matches with user-specified:

Project (Empty2Null(p) #2) Sort (p#1)

if ordering does not match and V1Write adds a sort:

Sort(p #2) Project (Empty2Null(p) #2)

cloud-fan · 2023-01-10T08:08:53Z

!WriteFiles
+- *(1) Project [id#10, sort_col#11, empty2null(p#12) AS p#19]
   +- *(1) Sort [p#12 ASC NULLS FIRST, sort_col#11 ASC NULLS FIRST], false, 0
      +- ShuffleQueryStage 0
         +- Exchange SinglePartition, REPARTITION_BY_NUM, [plan_id=18]
            +- LocalTableScan [id#10, sort_col#11, p#12]

!WriteFiles means something is wrong. @ulysses-you can you check if the expr id is correctly rewritten?

ulysses-you · 2023-01-10T08:17:32Z

!WriteFiles means something is wrong. @ulysses-you can you check if the expr id is correctly rewritten?

@cloud-fan fix in #39468

cloud-fan · 2023-01-10T14:10:58Z

sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/V1Writes.scala

    val outputOrdering = query.outputOrdering
-    // Check if the ordering is already matched to ensure the idempotency of the rule.
    val orderingMatched = isOrderingMatched(requiredOrdering, outputOrdering)
    if (orderingMatched) {
      empty2NullPlan


Even if we don't add the sort here, the caller will rewrite the attributes in v1 command which will update required orderng expression as well, right?

yes, that's the root reason in FileFormatWriter why the previous isOrderingMatched does not work when planned write enabled. The attributes in required ordering have been rewritten but the acutal ordering have not.

EnricoMi · 2023-01-13T07:03:20Z

@cloud-fan @ulysses-you I can confirm with #39468, the plan for the example now looks like

WriteFiles
+- *(1) Project [id#30, sort_col#31, empty2null(p#32) AS p#39]
   +- *(1) Sort [p#32 ASC NULLS FIRST, sort_col#31 ASC NULLS FIRST], false, 0
      +- ShuffleQueryStage 0
         +- Exchange SinglePartition, REPARTITION_BY_NUM, [plan_id=68]
            +- LocalTableScan [id#30, sort_col#31, p#32]

@ulysses-you can you please rebase with master or merge master into this branch
@cloud-fan can this go ahead then?

ulysses-you · 2023-01-13T14:41:52Z

@EnricoMi thank you for the confirm ! I send a new approach for this issue see #39556

…and AliasAwareQueryOutputOrdering to take all aliases into account ### What changes were proposed in this pull request? Currently `AliasAwareOutputPartitioning` and `AliasAwareQueryOutputOrdering` takes only the last alias by aliased expressions into account. We could avoid some extra shuffles and sorts with better alias handling. ### Why are the changes needed? Performance improvement and this also fix the issue in #39475. ### Does this PR introduce _any_ user-facing change? Yes, this PR fixes the issue in #39475. ### How was this patch tested? Added new UT. Closes #37525 from peter-toth/SPARK-40086-fix-aliasawareoutputexpression. Lead-authored-by: Peter Toth <[email protected]> Co-authored-by: ulysses-you <[email protected]> Signed-off-by: Wenchen Fan <[email protected]> (cherry picked from commit 6341b06) Signed-off-by: Wenchen Fan <[email protected]>

…and AliasAwareQueryOutputOrdering to take all aliases into account ### What changes were proposed in this pull request? Currently `AliasAwareOutputPartitioning` and `AliasAwareQueryOutputOrdering` takes only the last alias by aliased expressions into account. We could avoid some extra shuffles and sorts with better alias handling. ### Why are the changes needed? Performance improvement and this also fix the issue in #39475. ### Does this PR introduce _any_ user-facing change? Yes, this PR fixes the issue in #39475. ### How was this patch tested? Added new UT. Closes #37525 from peter-toth/SPARK-40086-fix-aliasawareoutputexpression. Lead-authored-by: Peter Toth <[email protected]> Co-authored-by: ulysses-you <[email protected]> Signed-off-by: Wenchen Fan <[email protected]>

ulysses-you · 2023-02-20T02:20:10Z

@cloud-fan any comments ? thank you

cloud-fan · 2023-02-20T08:41:05Z

thanks, merging to master/3.4 (code cleanup for planned write)

### What changes were proposed in this pull request? Cleanup some unnecessary `Empty2Null` related code ### Why are the changes needed? V1Writes checked idempotency using WriteFiles, so it's unnecessary to check if empty2null if exists again. ### Does this PR introduce _any_ user-facing change? no ### How was this patch tested? pass CI Closes #39475 from ulysses-you/SPARK-41959. Authored-by: ulysses-you <[email protected]> Signed-off-by: Wenchen Fan <[email protected]> (cherry picked from commit 547737b) Signed-off-by: Wenchen Fan <[email protected]>

…and AliasAwareQueryOutputOrdering to take all aliases into account ### What changes were proposed in this pull request? Currently `AliasAwareOutputPartitioning` and `AliasAwareQueryOutputOrdering` takes only the last alias by aliased expressions into account. We could avoid some extra shuffles and sorts with better alias handling. ### Why are the changes needed? Performance improvement and this also fix the issue in apache#39475. ### Does this PR introduce _any_ user-facing change? Yes, this PR fixes the issue in apache#39475. ### How was this patch tested? Added new UT. Closes apache#37525 from peter-toth/SPARK-40086-fix-aliasawareoutputexpression. Lead-authored-by: Peter Toth <[email protected]> Co-authored-by: ulysses-you <[email protected]> Signed-off-by: Wenchen Fan <[email protected]> (cherry picked from commit 6341b06) Signed-off-by: Wenchen Fan <[email protected]>

### What changes were proposed in this pull request? Cleanup some unnecessary `Empty2Null` related code ### Why are the changes needed? V1Writes checked idempotency using WriteFiles, so it's unnecessary to check if empty2null if exists again. ### Does this PR introduce _any_ user-facing change? no ### How was this patch tested? pass CI Closes apache#39475 from ulysses-you/SPARK-41959. Authored-by: ulysses-you <[email protected]> Signed-off-by: Wenchen Fan <[email protected]> (cherry picked from commit 547737b) Signed-off-by: Wenchen Fan <[email protected]>

github-actions bot added the SQL label Jan 10, 2023

ulysses-you commented Jan 10, 2023

View reviewed changes

ulysses-you force-pushed the SPARK-41959 branch from 82e20f1 to f14183c Compare January 10, 2023 05:26

EnricoMi mentioned this pull request Jan 10, 2023

[SPARK-40885] Sort may not take effect when it is the last 'Transform' operator #38356

Closed

EnricoMi approved these changes Jan 10, 2023

View reviewed changes

cloud-fan reviewed Jan 10, 2023

View reviewed changes

ulysses-you mentioned this pull request Jan 13, 2023

[SPARK-42049][SQL] Improve AliasAwareOutputExpression #39556

Closed

peter-toth mentioned this pull request Jan 17, 2023

[SPARK-40086][SPARK-42049][SQL] Improve AliasAwareOutputPartitioning and AliasAwareQueryOutputOrdering to take all aliases into account #37525

Closed

Improve v1 writes with empty2null

099ac9f

ulysses-you force-pushed the SPARK-41959 branch from f14183c to 099ac9f Compare February 1, 2023 02:25

cloud-fan approved these changes Feb 1, 2023

View reviewed changes

cloud-fan closed this in 547737b Feb 20, 2023

ulysses-you deleted the SPARK-41959 branch February 21, 2023 01:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-41959][SQL] Improve v1 writes with empty2null #39475

[SPARK-41959][SQL] Improve v1 writes with empty2null #39475

ulysses-you commented Jan 10, 2023 •

edited

Loading

ulysses-you Jan 10, 2023

ulysses-you Jan 10, 2023

ulysses-you commented Jan 10, 2023

cloud-fan commented Jan 10, 2023

ulysses-you commented Jan 10, 2023

EnricoMi left a comment

EnricoMi Jan 10, 2023

cloud-fan Jan 10, 2023

ulysses-you Jan 10, 2023

ulysses-you Jan 10, 2023

cloud-fan commented Jan 10, 2023

ulysses-you commented Jan 10, 2023

cloud-fan Jan 10, 2023

ulysses-you Jan 11, 2023

EnricoMi commented Jan 13, 2023

ulysses-you commented Jan 13, 2023

ulysses-you commented Feb 20, 2023

cloud-fan commented Feb 20, 2023

[SPARK-41959][SQL] Improve v1 writes with empty2null #39475

[SPARK-41959][SQL] Improve v1 writes with empty2null #39475

Conversation

ulysses-you commented Jan 10, 2023 • 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

ulysses-you commented Jan 10, 2023

cloud-fan commented Jan 10, 2023

ulysses-you commented Jan 10, 2023

EnricoMi left a comment

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

Choose a reason for hiding this comment

cloud-fan commented Jan 10, 2023

ulysses-you commented Jan 10, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EnricoMi commented Jan 13, 2023

ulysses-you commented Jan 13, 2023

ulysses-you commented Feb 20, 2023

cloud-fan commented Feb 20, 2023

ulysses-you commented Jan 10, 2023 •

edited

Loading