Skip to content

[SPARK-28532][SPARK-28530][SQL][FOLLOWUP] Inline doc for FixedPoint(1) batches "Subquery" and "Join Reorder"#25320

Closed
yeshengm wants to merge 2 commits intoapache:masterfrom
yeshengm:SPARK-28530-followup
Closed

[SPARK-28532][SPARK-28530][SQL][FOLLOWUP] Inline doc for FixedPoint(1) batches "Subquery" and "Join Reorder"#25320
yeshengm wants to merge 2 commits intoapache:masterfrom
yeshengm:SPARK-28530-followup

Conversation

@yeshengm
Copy link
Contributor

@yeshengm yeshengm commented Jul 31, 2019

What changes were proposed in this pull request?

Explained why "Subquery" and "Join Reorder" optimization batches should be FixedPoint(1), which was introduced in SPARK-28532 and SPARK-28530.

How was this patch tested?

Existing UTs.

@yeshengm yeshengm changed the title [SPARK-28532][SPARK-28530][SQL][FOLLOWUP] Inline doc for FixedPoint(1) rules [SPARK-28532][SPARK-28530][SQL][FOLLOWUP] Inline doc for FixedPoint(1) batches "Subquery" and "Join Reorder" Jul 31, 2019
PropagateEmptyRelation) ::
Batch("Pullup Correlated Expressions", Once,
PullupCorrelatedPredicates) ::
// Subquery batch is FixedPoint(1) because optimization might not converge in sub-queries
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Based on the SPARK-28532 description, how about a statement below?

Subquery batch applies the optimizer rules recursively. Therefore, it makes no sense to enforce idempotence on it and we change this batch from Once to FixedPoint(1).

RemoveLiteralFromGroupExpressions,
RemoveRepetitionFromGroupExpressions) :: Nil ++
operatorOptimizationBatch) :+
// Join reorder should be FixedPoint(1) since cost can change anyway for AQP.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Based on the SPARK-28530 description, how about a statement below;

Since join casts in AQP can change between multiple runs, there is no reason that we have an idempotence enforcement on this batch. We thus make it FixedPoint(1) instead of Once.

Copy link
Contributor

@dilipbiswal dilipbiswal Jul 31, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@maropu join cost ? :-)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

!!!! my bad... thx, your comment ;) @dilipbiswal

@SparkQA
Copy link

SparkQA commented Aug 1, 2019

Test build #108494 has finished for PR 25320 at commit b02e7b6.

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

@yeshengm yeshengm force-pushed the SPARK-28530-followup branch from b02e7b6 to 836e0b8 Compare August 1, 2019 17:47
@SparkQA
Copy link

SparkQA commented Aug 1, 2019

Test build #108527 has finished for PR 25320 at commit 836e0b8.

  • This patch fails Scala style tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@yeshengm yeshengm force-pushed the SPARK-28530-followup branch from 836e0b8 to 7e47b70 Compare August 1, 2019 18:04
RemoveLiteralFromGroupExpressions,
RemoveRepetitionFromGroupExpressions) :: Nil ++
operatorOptimizationBatch) :+
// Since join casts in AQP can change between multiple runs, there is no reason that we have an
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: typeo .. casts -> cost

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh...

@SparkQA
Copy link

SparkQA commented Aug 1, 2019

Test build #108529 has finished for PR 25320 at commit 7e47b70.

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

@dilipbiswal
Copy link
Contributor

Just a minor comment , otherwise LGTM

@gatorsmile
Copy link
Member

Addressed the comment. Let me merge it now. Thanks!

@gatorsmile gatorsmile closed this in 10d4ffd Aug 2, 2019
@SparkQA
Copy link

SparkQA commented Aug 2, 2019

Test build #108584 has finished for PR 25320 at commit c3d8e3a.

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

@dilipbiswal
Copy link
Contributor

retest this please

j-baker pushed a commit to palantir/spark that referenced this pull request Jan 25, 2020
…) batches "Subquery" and "Join Reorder"

## What changes were proposed in this pull request?
Explained why "Subquery" and "Join Reorder" optimization batches should be `FixedPoint(1)`, which was introduced in SPARK-28532 and SPARK-28530.

## How was this patch tested?

Existing UTs.

Closes apache#25320 from yeshengm/SPARK-28530-followup.

Lead-authored-by: Xiao Li <gatorsmile@gmail.com>
Co-authored-by: Yesheng Ma <kimi.ysma@gmail.com>
Signed-off-by: gatorsmile <gatorsmile@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants