You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by GitBox <gi...@apache.org> on 2020/12/16 16:55:19 UTC
[GitHub] [spark] aokolnychyi opened a new pull request #30808: [SPARK-33784][SQL] Rename dataSourceRewriteRules batch
aokolnychyi opened a new pull request #30808:
URL: https://github.com/apache/spark/pull/30808
<!--
Thanks for sending a pull request! Here are some tips for you:
1. If this is your first time, please read our contributor guidelines: https://spark.apache.org/contributing.html
2. Ensure you have added or run the appropriate tests for your PR: https://spark.apache.org/developer-tools.html
3. If the PR is unfinished, add '[WIP]' in your PR title, e.g., '[WIP][SPARK-XXXX] Your PR title ...'.
4. Be sure to keep the PR description updated to reflect all changes.
5. Please write your PR title to summarize what this PR proposes.
6. If possible, provide a concise example to reproduce the issue for a faster review.
7. If you want to add a new configuration, please read the guideline first for naming configurations in
'core/src/main/scala/org/apache/spark/internal/config/ConfigEntry.scala'.
-->
### What changes were proposed in this pull request?
<!--
Please clarify what changes you are proposing. The purpose of this section is to outline the changes and how this PR fixes the issue.
If possible, please consider writing useful notes for better and faster reviews in your PR. See the examples below.
1. If you refactor some codes with changing classes, showing the class hierarchy will help reviewers.
2. If you fix some SQL features, you can provide some references of other DBMSes.
3. If there is design documentation, please add the link.
4. If there is a discussion in the mailing list, please add the link.
-->
This PR tries to rename `dataSourceRewriteRules` into something more generic.
### Why are the changes needed?
<!--
Please clarify why the changes are needed. For instance,
1. If you propose a new API, clarify the use case for a new API.
2. If you fix a bug, you can clarify why it is a bug.
-->
These changes are needed to address the post-review discussion [here](https://github.com/apache/spark/pull/30558#discussion_r533885837).
### Does this PR introduce _any_ user-facing change?
<!--
Note that it means *any* user-facing change including all aspects such as the documentation fix.
If yes, please clarify the previous behavior and the change this PR proposes - provide the console output, description and/or an example to show the behavior difference if possible.
If possible, please also clarify if this is a user-facing change compared to the released Spark versions or within the unreleased branches such as master.
If no, write 'No'.
-->
Yes but the changes haven't been released yet.
### How was this patch tested?
<!--
If tests were added, say they were added here. Please make sure to add some test cases that check the changes thoroughly including negative and positive cases if possible.
If it was tested in a way different from regular unit tests, please clarify how you tested step by step, ideally copy and paste-able, so that other reviewers can test and check, and descendants can verify in the future.
If tests were not added, please describe why they were not added and/or why it was difficult to add.
-->
Existing tests.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on pull request #30808: [SPARK-33784][SQL] Rename dataSourceRewriteRules batch
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #30808:
URL: https://github.com/apache/spark/pull/30808#issuecomment-746849130
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/37499/
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on pull request #30808: [SPARK-33784][SQL] Rename dataSourceRewriteRules batch
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #30808:
URL: https://github.com/apache/spark/pull/30808#issuecomment-748973100
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/37748/
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] dongjoon-hyun commented on pull request #30808: [SPARK-33784][SQL] Rename dataSourceRewriteRules batch
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on pull request #30808:
URL: https://github.com/apache/spark/pull/30808#issuecomment-746641825
Thank you for making a follow-up, @aokolnychyi . I believe we can reach an agreement here.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] aokolnychyi commented on a change in pull request #30808: [SPARK-33784][SQL] Rename dataSourceRewriteRules batch
Posted by GitBox <gi...@apache.org>.
aokolnychyi commented on a change in pull request #30808:
URL: https://github.com/apache/spark/pull/30808#discussion_r544465441
##########
File path: sql/core/src/main/scala/org/apache/spark/sql/SparkSessionExtensions.scala
##########
@@ -200,19 +200,20 @@ class SparkSessionExtensions {
optimizerRules += builder
}
- private[this] val dataSourceRewriteRules = mutable.Buffer.empty[RuleBuilder]
+ private[this] val postOperatorOptimizationRules = mutable.Buffer.empty[RuleBuilder]
- private[sql] def buildDataSourceRewriteRules(session: SparkSession): Seq[Rule[LogicalPlan]] = {
- dataSourceRewriteRules.map(_.apply(session)).toSeq
+ private[sql] def buildPostOperatorOptimizationRules(
+ session: SparkSession): Seq[Rule[LogicalPlan]] = {
+ postOperatorOptimizationRules.map(_.apply(session)).toSeq
}
/**
- * Inject an optimizer `Rule` builder that rewrites data source plans into the [[SparkSession]].
+ * Inject an optimizer `Rule` builder that rewrites logical plans into the [[SparkSession]].
* The injected rules will be executed after the operator optimization batch and before rules
* that depend on stats.
*/
- def injectDataSourceRewriteRule(builder: RuleBuilder): Unit = {
- dataSourceRewriteRules += builder
+ def injectPostOperatorOptimizationRule(builder: RuleBuilder): Unit = {
Review comment:
This isn't final.
I'll explain my current thinking here. We are going to use this batch to rewrite plans directly after operator optimization. I did not go for `planRewriteRules` due to the [comment](https://github.com/apache/spark/pull/30558#discussion_r533151935) made by @dongjoon-hyun and @viirya (please let me know if you don't feel that way anymore). I also did not go for `preCBORules` as there quite a few things that will happen before CBO like discussed in the original thread.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] aokolnychyi commented on a change in pull request #30808: [WIP] [SPARK-33784][SQL] Rename dataSourceRewriteRules batch
Posted by GitBox <gi...@apache.org>.
aokolnychyi commented on a change in pull request #30808:
URL: https://github.com/apache/spark/pull/30808#discussion_r544908719
##########
File path: sql/core/src/main/scala/org/apache/spark/sql/SparkSessionExtensions.scala
##########
@@ -200,19 +200,20 @@ class SparkSessionExtensions {
optimizerRules += builder
}
- private[this] val dataSourceRewriteRules = mutable.Buffer.empty[RuleBuilder]
+ private[this] val postOperatorOptimizationRules = mutable.Buffer.empty[RuleBuilder]
- private[sql] def buildDataSourceRewriteRules(session: SparkSession): Seq[Rule[LogicalPlan]] = {
- dataSourceRewriteRules.map(_.apply(session)).toSeq
+ private[sql] def buildPostOperatorOptimizationRules(
+ session: SparkSession): Seq[Rule[LogicalPlan]] = {
+ postOperatorOptimizationRules.map(_.apply(session)).toSeq
}
/**
- * Inject an optimizer `Rule` builder that rewrites data source plans into the [[SparkSession]].
+ * Inject an optimizer `Rule` builder that rewrites logical plans into the [[SparkSession]].
* The injected rules will be executed after the operator optimization batch and before rules
* that depend on stats.
*/
- def injectDataSourceRewriteRule(builder: RuleBuilder): Unit = {
- dataSourceRewriteRules += builder
+ def injectPostOperatorOptimizationRule(builder: RuleBuilder): Unit = {
Review comment:
I do share some of the points brought by @cloud-fan. There is no clear separation between CBO and RBO and we run quite some rule-based optimizations after the existing cost-based optimization rule. Also, we need to run this batch before early scan pushdown rules to capture possible rewrites, which, in turn, run before CBO. That’s why `preCBORules` does seem to ideally convey what this rule does.
Our existing hook in the extensions API says `The injected rules will be executed during the operator optimization batch`. That was one of my motivations for the current name as we have resolution and post-hoc resolution rules.
That said, I don’t mean `postOperatorOptimizationRules` is a perfect name either. It just seems to better reflect the place in the Spark optimizer. I’ll be happy to discuss and iterate further.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] dongjoon-hyun commented on pull request #30808: [SPARK-33784][SQL] Rename dataSourceRewriteRules batch
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on pull request #30808:
URL: https://github.com/apache/spark/pull/30808#issuecomment-749687637
Thank you all for the decision!
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] aokolnychyi commented on a change in pull request #30808: [WIP] [SPARK-33784][SQL] Rename dataSourceRewriteRules batch
Posted by GitBox <gi...@apache.org>.
aokolnychyi commented on a change in pull request #30808:
URL: https://github.com/apache/spark/pull/30808#discussion_r544908719
##########
File path: sql/core/src/main/scala/org/apache/spark/sql/SparkSessionExtensions.scala
##########
@@ -200,19 +200,20 @@ class SparkSessionExtensions {
optimizerRules += builder
}
- private[this] val dataSourceRewriteRules = mutable.Buffer.empty[RuleBuilder]
+ private[this] val postOperatorOptimizationRules = mutable.Buffer.empty[RuleBuilder]
- private[sql] def buildDataSourceRewriteRules(session: SparkSession): Seq[Rule[LogicalPlan]] = {
- dataSourceRewriteRules.map(_.apply(session)).toSeq
+ private[sql] def buildPostOperatorOptimizationRules(
+ session: SparkSession): Seq[Rule[LogicalPlan]] = {
+ postOperatorOptimizationRules.map(_.apply(session)).toSeq
}
/**
- * Inject an optimizer `Rule` builder that rewrites data source plans into the [[SparkSession]].
+ * Inject an optimizer `Rule` builder that rewrites logical plans into the [[SparkSession]].
* The injected rules will be executed after the operator optimization batch and before rules
* that depend on stats.
*/
- def injectDataSourceRewriteRule(builder: RuleBuilder): Unit = {
- dataSourceRewriteRules += builder
+ def injectPostOperatorOptimizationRule(builder: RuleBuilder): Unit = {
Review comment:
I do share some of the points brought by @cloud-fan. There is no clear separation between CBO and RBO and we run quite some rule-based optimizations after the existing cost-based optimization rule. Also, we need to run this batch before early scan pushdown rules to capture possible rewrites, which, in turn, run before CBO. That’s why `preCBORules` does seem to ideally capture what this rule does.
Our existing hook in the extensions API says `The injected rules will be executed during the operator optimization batch`. That was one of my motivations for the current name as we have resolution and post-hoc resolution rules.
That said, I don’t mean `postOperatorOptimizationRules` is a perfect name either. It just seems to better reflect the place in the Spark optimizer. I’ll be happy to discuss and iterate further.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA removed a comment on pull request #30808: [SPARK-33784][SQL] Rename dataSourceRewriteRules batch
Posted by GitBox <gi...@apache.org>.
SparkQA removed a comment on pull request #30808:
URL: https://github.com/apache/spark/pull/30808#issuecomment-746653157
**[Test build #132897 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/132897/testReport)** for PR 30808 at commit [`8de3958`](https://github.com/apache/spark/commit/8de39583a23bde3a29fa1af5cc98d9aa4de0b68d).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #30808: [SPARK-33784][SQL] Rename dataSourceRewriteRules batch
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #30808:
URL: https://github.com/apache/spark/pull/30808#issuecomment-748927040
**[Test build #133149 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/133149/testReport)** for PR 30808 at commit [`dcd0b7c`](https://github.com/apache/spark/commit/dcd0b7c5fa972572d53c8322071b85169d6682ff).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] rdblue commented on pull request #30808: [WIP] [SPARK-33784][SQL] Rename dataSourceRewriteRules batch
Posted by GitBox <gi...@apache.org>.
rdblue commented on pull request #30808:
URL: https://github.com/apache/spark/pull/30808#issuecomment-748532969
I think that `preCBORules` is a bad name, but I am fine with using it to move forward.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] aokolnychyi commented on a change in pull request #30808: [WIP] [SPARK-33784][SQL] Rename dataSourceRewriteRules batch
Posted by GitBox <gi...@apache.org>.
aokolnychyi commented on a change in pull request #30808:
URL: https://github.com/apache/spark/pull/30808#discussion_r544908719
##########
File path: sql/core/src/main/scala/org/apache/spark/sql/SparkSessionExtensions.scala
##########
@@ -200,19 +200,20 @@ class SparkSessionExtensions {
optimizerRules += builder
}
- private[this] val dataSourceRewriteRules = mutable.Buffer.empty[RuleBuilder]
+ private[this] val postOperatorOptimizationRules = mutable.Buffer.empty[RuleBuilder]
- private[sql] def buildDataSourceRewriteRules(session: SparkSession): Seq[Rule[LogicalPlan]] = {
- dataSourceRewriteRules.map(_.apply(session)).toSeq
+ private[sql] def buildPostOperatorOptimizationRules(
+ session: SparkSession): Seq[Rule[LogicalPlan]] = {
+ postOperatorOptimizationRules.map(_.apply(session)).toSeq
}
/**
- * Inject an optimizer `Rule` builder that rewrites data source plans into the [[SparkSession]].
+ * Inject an optimizer `Rule` builder that rewrites logical plans into the [[SparkSession]].
* The injected rules will be executed after the operator optimization batch and before rules
* that depend on stats.
*/
- def injectDataSourceRewriteRule(builder: RuleBuilder): Unit = {
- dataSourceRewriteRules += builder
+ def injectPostOperatorOptimizationRule(builder: RuleBuilder): Unit = {
Review comment:
I do share some of the points brought by @cloud-fan. There is no clear separation between CBO and RBO and we run quite some rule-based optimizations after the existing cost-based optimization rule. Also, we need to run this batch before early scan pushdown rules to capture possible rewrites, which, in turn, run before CBO. That’s why `preCBORules` does not seem to ideally convey what this rule does.
Our existing hook in the extensions API says `The injected rules will be executed during the operator optimization batch`. That was one of my motivations for the current name as we have resolution and post-hoc resolution rules.
That said, I don’t mean `postOperatorOptimizationRules` is a perfect name either. It just seems to better reflect the place in the Spark optimizer. I’ll be happy to discuss and iterate further.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] gatorsmile commented on pull request #30808: [SPARK-33784][SQL] Rename dataSourceRewriteRules batch
Posted by GitBox <gi...@apache.org>.
gatorsmile commented on pull request #30808:
URL: https://github.com/apache/spark/pull/30808#issuecomment-747189931
Let us mark as WIP before we get an agreement on the API.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan closed pull request #30808: [SPARK-33784][SQL] Rename dataSourceRewriteRules batch
Posted by GitBox <gi...@apache.org>.
cloud-fan closed pull request #30808:
URL: https://github.com/apache/spark/pull/30808
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA removed a comment on pull request #30808: [SPARK-33784][SQL] Rename dataSourceRewriteRules batch
Posted by GitBox <gi...@apache.org>.
SparkQA removed a comment on pull request #30808:
URL: https://github.com/apache/spark/pull/30808#issuecomment-748927040
**[Test build #133149 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/133149/testReport)** for PR 30808 at commit [`dcd0b7c`](https://github.com/apache/spark/commit/dcd0b7c5fa972572d53c8322071b85169d6682ff).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #30808: [SPARK-33784][SQL] Rename dataSourceRewriteRules batch
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #30808:
URL: https://github.com/apache/spark/pull/30808#issuecomment-748958036
Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/37748/
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] dongjoon-hyun commented on pull request #30808: [SPARK-33784][SQL] Rename dataSourceRewriteRules batch
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on pull request #30808:
URL: https://github.com/apache/spark/pull/30808#issuecomment-751010169
Sorry for late response. The new backporting PR (SPARK-33621 and SPARK-33784) looks good to me and it landed at branch-3.1 for Apache Spark 3.1.0 a few minutes ago.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on pull request #30808: [SPARK-33784][SQL] Rename dataSourceRewriteRules batch
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #30808:
URL: https://github.com/apache/spark/pull/30808#issuecomment-749061872
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/133149/
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] aokolnychyi edited a comment on pull request #30808: [SPARK-33784][SQL] Rename dataSourceRewriteRules batch
Posted by GitBox <gi...@apache.org>.
aokolnychyi edited a comment on pull request #30808:
URL: https://github.com/apache/spark/pull/30808#issuecomment-750860326
@dongjoon-hyun @cloud-fan, shall I cherry-pick this change in full or shall I cherry-pick only part that is present in 3.1?
Like I mentioned earlier, the second part of this change (the hook to inject custom rules) was created before 3.1 was cut but was merged a couple of days after. I think it only makes sense to have the new batch in 3.1 if we also add the hook.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] aokolnychyi commented on pull request #30808: [SPARK-33784][SQL] Rename dataSourceRewriteRules batch
Posted by GitBox <gi...@apache.org>.
aokolnychyi commented on pull request #30808:
URL: https://github.com/apache/spark/pull/30808#issuecomment-748925627
I've updated this PR.
Thanks everyone who participated in the discussion and @dongjoon-hyun for creating a summary to move this forward.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #30808: [SPARK-33784][SQL] Rename dataSourceRewriteRules batch
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #30808:
URL: https://github.com/apache/spark/pull/30808#issuecomment-746800347
Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/37499/
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] gatorsmile commented on a change in pull request #30808: [SPARK-33784][SQL] Rename dataSourceRewriteRules batch
Posted by GitBox <gi...@apache.org>.
gatorsmile commented on a change in pull request #30808:
URL: https://github.com/apache/spark/pull/30808#discussion_r544774803
##########
File path: sql/core/src/main/scala/org/apache/spark/sql/SparkSessionExtensions.scala
##########
@@ -200,19 +200,20 @@ class SparkSessionExtensions {
optimizerRules += builder
}
- private[this] val dataSourceRewriteRules = mutable.Buffer.empty[RuleBuilder]
+ private[this] val postOperatorOptimizationRules = mutable.Buffer.empty[RuleBuilder]
- private[sql] def buildDataSourceRewriteRules(session: SparkSession): Seq[Rule[LogicalPlan]] = {
- dataSourceRewriteRules.map(_.apply(session)).toSeq
+ private[sql] def buildPostOperatorOptimizationRules(
+ session: SparkSession): Seq[Rule[LogicalPlan]] = {
+ postOperatorOptimizationRules.map(_.apply(session)).toSeq
}
/**
- * Inject an optimizer `Rule` builder that rewrites data source plans into the [[SparkSession]].
+ * Inject an optimizer `Rule` builder that rewrites logical plans into the [[SparkSession]].
* The injected rules will be executed after the operator optimization batch and before rules
* that depend on stats.
*/
- def injectDataSourceRewriteRule(builder: RuleBuilder): Unit = {
- dataSourceRewriteRules += builder
+ def injectPostOperatorOptimizationRule(builder: RuleBuilder): Unit = {
Review comment:
To be honest, I do not like this name. Our catalyst is designed as a query compiler. There does not exist a concept in a query compiler called "Post Operator Optimization". If you talk about this extension point in a Database class, no one understands what it means from the name.
My point is still the same. Here, we are adding the extension point for advanced users to add the rules between the rule-based optimizer (RBO) rules and the cost-based optimizer (CBO) rules.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] aokolnychyi commented on pull request #30808: [SPARK-33784][SQL] Rename dataSourceRewriteRules batch
Posted by GitBox <gi...@apache.org>.
aokolnychyi commented on pull request #30808:
URL: https://github.com/apache/spark/pull/30808#issuecomment-749438435
Yep, will do today, @cloud-fan.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #30808: [SPARK-33784][SQL] Rename dataSourceRewriteRules batch
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #30808:
URL: https://github.com/apache/spark/pull/30808#issuecomment-748946259
Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/37748/
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] aokolnychyi commented on pull request #30808: [WIP] [SPARK-33784][SQL] Rename dataSourceRewriteRules batch
Posted by GitBox <gi...@apache.org>.
aokolnychyi commented on pull request #30808:
URL: https://github.com/apache/spark/pull/30808#issuecomment-748918910
I am going to update this PR to use `preCBORules` if there is enough consesus. I did have similar concerns to what @cloud-fan mentioned in [this](https://github.com/apache/spark/pull/30808#discussion_r544787602) comment but I don't have an ideal alternative.
Let me change the name and unblock this PR.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] aokolnychyi commented on a change in pull request #30808: [WIP] [SPARK-33784][SQL] Rename dataSourceRewriteRules batch
Posted by GitBox <gi...@apache.org>.
aokolnychyi commented on a change in pull request #30808:
URL: https://github.com/apache/spark/pull/30808#discussion_r544917345
##########
File path: sql/core/src/main/scala/org/apache/spark/sql/SparkSessionExtensions.scala
##########
@@ -200,19 +200,20 @@ class SparkSessionExtensions {
optimizerRules += builder
}
- private[this] val dataSourceRewriteRules = mutable.Buffer.empty[RuleBuilder]
+ private[this] val postOperatorOptimizationRules = mutable.Buffer.empty[RuleBuilder]
- private[sql] def buildDataSourceRewriteRules(session: SparkSession): Seq[Rule[LogicalPlan]] = {
- dataSourceRewriteRules.map(_.apply(session)).toSeq
+ private[sql] def buildPostOperatorOptimizationRules(
+ session: SparkSession): Seq[Rule[LogicalPlan]] = {
+ postOperatorOptimizationRules.map(_.apply(session)).toSeq
}
/**
- * Inject an optimizer `Rule` builder that rewrites data source plans into the [[SparkSession]].
+ * Inject an optimizer `Rule` builder that rewrites logical plans into the [[SparkSession]].
* The injected rules will be executed after the operator optimization batch and before rules
* that depend on stats.
*/
- def injectDataSourceRewriteRule(builder: RuleBuilder): Unit = {
- dataSourceRewriteRules += builder
+ def injectPostOperatorOptimizationRule(builder: RuleBuilder): Unit = {
Review comment:
I'd be also interested to hear from @hvanhovell and other folks who commented.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] gatorsmile commented on a change in pull request #30808: [SPARK-33784][SQL] Rename dataSourceRewriteRules batch
Posted by GitBox <gi...@apache.org>.
gatorsmile commented on a change in pull request #30808:
URL: https://github.com/apache/spark/pull/30808#discussion_r544777666
##########
File path: sql/core/src/main/scala/org/apache/spark/sql/SparkSessionExtensions.scala
##########
@@ -200,19 +200,20 @@ class SparkSessionExtensions {
optimizerRules += builder
}
- private[this] val dataSourceRewriteRules = mutable.Buffer.empty[RuleBuilder]
+ private[this] val postOperatorOptimizationRules = mutable.Buffer.empty[RuleBuilder]
- private[sql] def buildDataSourceRewriteRules(session: SparkSession): Seq[Rule[LogicalPlan]] = {
- dataSourceRewriteRules.map(_.apply(session)).toSeq
+ private[sql] def buildPostOperatorOptimizationRules(
+ session: SparkSession): Seq[Rule[LogicalPlan]] = {
+ postOperatorOptimizationRules.map(_.apply(session)).toSeq
}
/**
- * Inject an optimizer `Rule` builder that rewrites data source plans into the [[SparkSession]].
+ * Inject an optimizer `Rule` builder that rewrites logical plans into the [[SparkSession]].
* The injected rules will be executed after the operator optimization batch and before rules
* that depend on stats.
*/
- def injectDataSourceRewriteRule(builder: RuleBuilder): Unit = {
- dataSourceRewriteRules += builder
+ def injectPostOperatorOptimizationRule(builder: RuleBuilder): Unit = {
Review comment:
BTW, eventually, we should move all the statistics based rules from the optimizer to the planner
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] dongjoon-hyun commented on pull request #30808: [SPARK-33784][SQL] Rename dataSourceRewriteRules batch
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on pull request #30808:
URL: https://github.com/apache/spark/pull/30808#issuecomment-750317630
Gentle ping, @aokolnychyi .
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on pull request #30808: [SPARK-33784][SQL] Rename dataSourceRewriteRules batch
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #30808:
URL: https://github.com/apache/spark/pull/30808#issuecomment-747049703
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/132897/
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #30808: [SPARK-33784][SQL] Rename dataSourceRewriteRules batch
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #30808:
URL: https://github.com/apache/spark/pull/30808#issuecomment-746746380
Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/37499/
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] aokolnychyi commented on a change in pull request #30808: [WIP] [SPARK-33784][SQL] Rename dataSourceRewriteRules batch
Posted by GitBox <gi...@apache.org>.
aokolnychyi commented on a change in pull request #30808:
URL: https://github.com/apache/spark/pull/30808#discussion_r544908719
##########
File path: sql/core/src/main/scala/org/apache/spark/sql/SparkSessionExtensions.scala
##########
@@ -200,19 +200,20 @@ class SparkSessionExtensions {
optimizerRules += builder
}
- private[this] val dataSourceRewriteRules = mutable.Buffer.empty[RuleBuilder]
+ private[this] val postOperatorOptimizationRules = mutable.Buffer.empty[RuleBuilder]
- private[sql] def buildDataSourceRewriteRules(session: SparkSession): Seq[Rule[LogicalPlan]] = {
- dataSourceRewriteRules.map(_.apply(session)).toSeq
+ private[sql] def buildPostOperatorOptimizationRules(
+ session: SparkSession): Seq[Rule[LogicalPlan]] = {
+ postOperatorOptimizationRules.map(_.apply(session)).toSeq
}
/**
- * Inject an optimizer `Rule` builder that rewrites data source plans into the [[SparkSession]].
+ * Inject an optimizer `Rule` builder that rewrites logical plans into the [[SparkSession]].
* The injected rules will be executed after the operator optimization batch and before rules
* that depend on stats.
*/
- def injectDataSourceRewriteRule(builder: RuleBuilder): Unit = {
- dataSourceRewriteRules += builder
+ def injectPostOperatorOptimizationRule(builder: RuleBuilder): Unit = {
Review comment:
I do share some of the points brought by @cloud-fan. There is no clear separation between CBO and RBO and we run quite some rule-based optimizations after the existing cost-based optimization rule. Also, we need to run this batch before early scan pushdown rules to capture possible rewrites, which, in turn, run before CBO. That’s why `preCBORules` does seem to capture what this rule does.
Our existing hook in the extensions API says `The injected rules will be executed during the operator optimization batch`. That was one of my motivations for the current name as we have resolution and post-hoc resolution rules.
That said, I don’t mean `postOperatorOptimizationRules` is a perfect name either. It just seems to better reflect the place in the Spark optimizer. I’ll be happy to discuss and iterate further.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] HyukjinKwon commented on pull request #30808: [WIP] [SPARK-33784][SQL] Rename dataSourceRewriteRules batch
Posted by GitBox <gi...@apache.org>.
HyukjinKwon commented on pull request #30808:
URL: https://github.com/apache/spark/pull/30808#issuecomment-748915436
I am okay with `preCBORules` too. Looks like we're okay with this name.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on pull request #30808: [SPARK-33784][SQL] Rename dataSourceRewriteRules batch
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #30808:
URL: https://github.com/apache/spark/pull/30808#issuecomment-748973100
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] aokolnychyi commented on pull request #30808: [SPARK-33784][SQL] Rename dataSourceRewriteRules batch
Posted by GitBox <gi...@apache.org>.
aokolnychyi commented on pull request #30808:
URL: https://github.com/apache/spark/pull/30808#issuecomment-746638555
cc @rdblue @HyukjinKwon @dongjoon-hyun @cloud-fan @viirya @dbtsai @sunchao @gatorsmile @maryannxue
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on pull request #30808: [SPARK-33784][SQL] Rename dataSourceRewriteRules batch
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #30808:
URL: https://github.com/apache/spark/pull/30808#issuecomment-747049703
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/132897/
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on pull request #30808: [SPARK-33784][SQL] Rename dataSourceRewriteRules batch
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on pull request #30808:
URL: https://github.com/apache/spark/pull/30808#issuecomment-749416944
It conflicts with 3.1, @aokolnychyi can you open a backport PR? thanks!
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #30808: [SPARK-33784][SQL] Rename dataSourceRewriteRules batch
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #30808:
URL: https://github.com/apache/spark/pull/30808#issuecomment-746653157
**[Test build #132897 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/132897/testReport)** for PR 30808 at commit [`8de3958`](https://github.com/apache/spark/commit/8de39583a23bde3a29fa1af5cc98d9aa4de0b68d).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a change in pull request #30808: [SPARK-33784][SQL] Rename dataSourceRewriteRules batch
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #30808:
URL: https://github.com/apache/spark/pull/30808#discussion_r544787602
##########
File path: sql/core/src/main/scala/org/apache/spark/sql/SparkSessionExtensions.scala
##########
@@ -200,19 +200,20 @@ class SparkSessionExtensions {
optimizerRules += builder
}
- private[this] val dataSourceRewriteRules = mutable.Buffer.empty[RuleBuilder]
+ private[this] val postOperatorOptimizationRules = mutable.Buffer.empty[RuleBuilder]
- private[sql] def buildDataSourceRewriteRules(session: SparkSession): Seq[Rule[LogicalPlan]] = {
- dataSourceRewriteRules.map(_.apply(session)).toSeq
+ private[sql] def buildPostOperatorOptimizationRules(
+ session: SparkSession): Seq[Rule[LogicalPlan]] = {
+ postOperatorOptimizationRules.map(_.apply(session)).toSeq
}
/**
- * Inject an optimizer `Rule` builder that rewrites data source plans into the [[SparkSession]].
+ * Inject an optimizer `Rule` builder that rewrites logical plans into the [[SparkSession]].
* The injected rules will be executed after the operator optimization batch and before rules
* that depend on stats.
*/
- def injectDataSourceRewriteRule(builder: RuleBuilder): Unit = {
- dataSourceRewriteRules += builder
+ def injectPostOperatorOptimizationRule(builder: RuleBuilder): Unit = {
Review comment:
It's unfortunate that we don't have a clear separation between RBO and CBO. There are RBO rules before and after the only CBO rule `CostBasedJoinReorder`.
I think the general idea of this batch is to allow people to inject special optimizer rules that can't be run together with the main operator optimizer batch. It's indeed a Spark specific thing, as the main operator optimizer batch will be run many times until reaching the fixed point, the new batch added here will be run only once.
It's really hard to do the naming here. To match the actual purpose and to be general, how about `Phase 2 Optimizer Rules` or `Run Once Optimizer Rules`?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] dongjoon-hyun commented on pull request #30808: [WIP] [SPARK-33784][SQL] Rename dataSourceRewriteRules batch
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on pull request #30808:
URL: https://github.com/apache/spark/pull/30808#issuecomment-748537721
Thank you for making us move forward, @rdblue .
@aokolnychyi , could you update this PR once more to get further comments?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] dongjoon-hyun commented on pull request #30808: [WIP] [SPARK-33784][SQL] Rename dataSourceRewriteRules batch
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on pull request #30808:
URL: https://github.com/apache/spark/pull/30808#issuecomment-748537960
Also, @gatorsmile . Please let us know what you think about the AS-IS direction.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] rdblue commented on pull request #30808: [SPARK-33784][SQL] Rename dataSourceRewriteRules batch
Posted by GitBox <gi...@apache.org>.
rdblue commented on pull request #30808:
URL: https://github.com/apache/spark/pull/30808#issuecomment-746703602
I'm fine with post operator optimization.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan edited a comment on pull request #30808: [SPARK-33784][SQL] Rename dataSourceRewriteRules batch
Posted by GitBox <gi...@apache.org>.
cloud-fan edited a comment on pull request #30808:
URL: https://github.com/apache/spark/pull/30808#issuecomment-749416452
thanks, merging to master!
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on pull request #30808: [SPARK-33784][SQL] Rename dataSourceRewriteRules batch
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #30808:
URL: https://github.com/apache/spark/pull/30808#issuecomment-746849130
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/37499/
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] aokolnychyi commented on pull request #30808: [SPARK-33784][SQL] Rename dataSourceRewriteRules batch
Posted by GitBox <gi...@apache.org>.
aokolnychyi commented on pull request #30808:
URL: https://github.com/apache/spark/pull/30808#issuecomment-750860326
@dongjoon-hyun @cloud-fan, shall I cherry-pick this change in full or shall I cherry-pick only part that is present in 3.1?
Like I mentioned earlier, the second part of this change (the hook to inject custom rules) was created before 3.1 was cut but was merged a couple of days after. I think it only makes sense to have the new batch in 3.1 only if we also add the hook.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] HyukjinKwon commented on pull request #30808: [SPARK-33784][SQL] Rename dataSourceRewriteRules batch
Posted by GitBox <gi...@apache.org>.
HyukjinKwon commented on pull request #30808:
URL: https://github.com/apache/spark/pull/30808#issuecomment-748925322
Seems we reached to the agreement. I am removing `WIP`.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] gatorsmile commented on a change in pull request #30808: [SPARK-33784][SQL] Rename dataSourceRewriteRules batch
Posted by GitBox <gi...@apache.org>.
gatorsmile commented on a change in pull request #30808:
URL: https://github.com/apache/spark/pull/30808#discussion_r544776825
##########
File path: sql/core/src/main/scala/org/apache/spark/sql/SparkSessionExtensions.scala
##########
@@ -200,19 +200,20 @@ class SparkSessionExtensions {
optimizerRules += builder
}
- private[this] val dataSourceRewriteRules = mutable.Buffer.empty[RuleBuilder]
+ private[this] val postOperatorOptimizationRules = mutable.Buffer.empty[RuleBuilder]
- private[sql] def buildDataSourceRewriteRules(session: SparkSession): Seq[Rule[LogicalPlan]] = {
- dataSourceRewriteRules.map(_.apply(session)).toSeq
+ private[sql] def buildPostOperatorOptimizationRules(
+ session: SparkSession): Seq[Rule[LogicalPlan]] = {
+ postOperatorOptimizationRules.map(_.apply(session)).toSeq
}
/**
- * Inject an optimizer `Rule` builder that rewrites data source plans into the [[SparkSession]].
+ * Inject an optimizer `Rule` builder that rewrites logical plans into the [[SparkSession]].
* The injected rules will be executed after the operator optimization batch and before rules
* that depend on stats.
*/
- def injectDataSourceRewriteRule(builder: RuleBuilder): Unit = {
- dataSourceRewriteRules += builder
+ def injectPostOperatorOptimizationRule(builder: RuleBuilder): Unit = {
Review comment:
The batch/function/rule names in Optimizer.scala is not critical to me. They are private. We can change them whenever we need. However, this API is an external developer API. We should be very careful to name it.
To me, either preCBORules or postRBORules are much better than postOperatorOptimizationRule.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] aokolnychyi commented on a change in pull request #30808: [WIP] [SPARK-33784][SQL] Rename dataSourceRewriteRules batch
Posted by GitBox <gi...@apache.org>.
aokolnychyi commented on a change in pull request #30808:
URL: https://github.com/apache/spark/pull/30808#discussion_r546654503
##########
File path: sql/core/src/main/scala/org/apache/spark/sql/SparkSessionExtensions.scala
##########
@@ -200,19 +200,19 @@ class SparkSessionExtensions {
optimizerRules += builder
}
- private[this] val dataSourceRewriteRules = mutable.Buffer.empty[RuleBuilder]
+ private[this] val preCBORules = mutable.Buffer.empty[RuleBuilder]
- private[sql] def buildDataSourceRewriteRules(session: SparkSession): Seq[Rule[LogicalPlan]] = {
- dataSourceRewriteRules.map(_.apply(session)).toSeq
+ private[sql] def buildPreCBORules(session: SparkSession): Seq[Rule[LogicalPlan]] = {
+ preCBORules.map(_.apply(session)).toSeq
}
/**
- * Inject an optimizer `Rule` builder that rewrites data source plans into the [[SparkSession]].
- * The injected rules will be executed after the operator optimization batch and before rules
- * that depend on stats.
+ * Inject an optimizer `Rule` builder that rewrites logical plans into the [[SparkSession]].
+ * The injected rules will be executed once after the operator optimization batch and
Review comment:
I added that the rule will be executed once to the docs.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #30808: [SPARK-33784][SQL] Rename dataSourceRewriteRules batch
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #30808:
URL: https://github.com/apache/spark/pull/30808#issuecomment-749045361
**[Test build #133149 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/133149/testReport)** for PR 30808 at commit [`dcd0b7c`](https://github.com/apache/spark/commit/dcd0b7c5fa972572d53c8322071b85169d6682ff).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public classes.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] gatorsmile commented on a change in pull request #30808: [SPARK-33784][SQL] Rename dataSourceRewriteRules batch
Posted by GitBox <gi...@apache.org>.
gatorsmile commented on a change in pull request #30808:
URL: https://github.com/apache/spark/pull/30808#discussion_r544794560
##########
File path: sql/core/src/main/scala/org/apache/spark/sql/SparkSessionExtensions.scala
##########
@@ -200,19 +200,20 @@ class SparkSessionExtensions {
optimizerRules += builder
}
- private[this] val dataSourceRewriteRules = mutable.Buffer.empty[RuleBuilder]
+ private[this] val postOperatorOptimizationRules = mutable.Buffer.empty[RuleBuilder]
- private[sql] def buildDataSourceRewriteRules(session: SparkSession): Seq[Rule[LogicalPlan]] = {
- dataSourceRewriteRules.map(_.apply(session)).toSeq
+ private[sql] def buildPostOperatorOptimizationRules(
+ session: SparkSession): Seq[Rule[LogicalPlan]] = {
+ postOperatorOptimizationRules.map(_.apply(session)).toSeq
}
/**
- * Inject an optimizer `Rule` builder that rewrites data source plans into the [[SparkSession]].
+ * Inject an optimizer `Rule` builder that rewrites logical plans into the [[SparkSession]].
* The injected rules will be executed after the operator optimization batch and before rules
* that depend on stats.
*/
- def injectDataSourceRewriteRule(builder: RuleBuilder): Unit = {
- dataSourceRewriteRules += builder
+ def injectPostOperatorOptimizationRule(builder: RuleBuilder): Unit = {
Review comment:
`Phase 2 Optimizer Rules` or `Run Once Optimizer Rules` does not explain the location of the batch.
How many batches do we want to add? I am afraid we will add phase 2, 3, 4, .... This is endless. It looks very random. We should not expect the users need to understand/read the source code of our optimizer every new release. It is also fragile.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on pull request #30808: [SPARK-33784][SQL] Rename dataSourceRewriteRules batch
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on pull request #30808:
URL: https://github.com/apache/spark/pull/30808#issuecomment-749416452
thanks, merging to master/3.1!
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #30808: [SPARK-33784][SQL] Rename dataSourceRewriteRules batch
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #30808:
URL: https://github.com/apache/spark/pull/30808#issuecomment-747046517
**[Test build #132897 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/132897/testReport)** for PR 30808 at commit [`8de3958`](https://github.com/apache/spark/commit/8de39583a23bde3a29fa1af5cc98d9aa4de0b68d).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public classes.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org