Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[SPARK-27088][SQL] Add a configuration to set log level for each batch at RuleExecutor #24136

Closed
wants to merge 2 commits into from
Closed

[SPARK-27088][SQL] Add a configuration to set log level for each batch at RuleExecutor #24136

wants to merge 2 commits into from

Conversation

chakravarthiT
Copy link
Contributor

What changes were proposed in this pull request?

Similar to #22406 , which has made log level for plan changes by each rule configurable ,this PR is to make log level for plan changes by each batch configurable,and I have reused the same configuration: "spark.sql.optimizer.planChangeLog.level".

Config proposed in this PR ,
spark.sql.optimizer.planChangeLog.batches - enable plan change logging only for a set of specified batches, separated by commas.

How was this patch tested?

Added UT , also tested manually and attached screenshots below.

1)Setting spark.sql.optimizer.planChangeLog.leve to warn.

settingLogLevelToWarn

2)setting spark.sql.optimizer.planChangeLog.batches to Resolution and Subquery.
settingBatchestoLog

  1. plan change logging enabled only for a set of specified batches(Resolution and Subquery)
    batchloggingOp

@chakravarthiT
Copy link
Contributor Author

@gengliangwang @HyukjinKwon Please review.

@HyukjinKwon
Copy link
Member

cc @maryannxue

maryannxue
maryannxue approved these changes Mar 20, 2019
View reviewed changes
Copy link
Contributor

@maryannxue maryannxue left a comment

Choose a reason for hiding this comment

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

LGTM, except some minor suggestions.

sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/optimizer/OptimizerLoggingSuite.scala Outdated Show resolved Hide resolved
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/optimizer/OptimizerLoggingSuite.scala Show resolved Hide resolved
@gatorsmile
Copy link
Member

ok to test

@SparkQA
Copy link

SparkQA commented Mar 21, 2019

Test build #103756 has finished for PR 24136 at commit de7aae0.

  • This patch fails due to an unknown error code, -9.
  • This patch merges cleanly.
  • This patch adds no public classes.

@maropu
Copy link
Member

maropu commented Mar 21, 2019

retest this please

@SparkQA
Copy link

SparkQA commented Mar 21, 2019

Test build #103760 has finished for PR 24136 at commit de7aae0.

  • This patch fails to build.
  • This patch merges cleanly.
  • This patch adds no public classes.

@chakravarthiT
Copy link
Contributor Author

looks like non-relevant failure.

@maropu
Copy link
Member

maropu commented Mar 22, 2019

retest this please

@SparkQA
Copy link

SparkQA commented Mar 22, 2019

Test build #103799 has finished for PR 24136 at commit de7aae0.

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

@SparkQA
Copy link

SparkQA commented Mar 22, 2019

Test build #103801 has finished for PR 24136 at commit f68d21b.

  • This patch fails Spark unit tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
  • class ArrowStreamPandasSerializer(ArrowStreamSerializer):
  • class ArrowStreamPandasUDFSerializer(ArrowStreamPandasSerializer):
  • class IdentifierImpl implements Identifier
  • class CatalogNotFoundException(message: String, cause: Throwable)
  • trait LookupCatalog
  • class FractionTimestampFormatter(zoneId: ZoneId)

@sandeep-katta sandeep-katta mentioned this pull request Mar 22, 2019
Closed
@HyukjinKwon HyukjinKwon changed the title [SPARK-27088][SQL] Apply conf "spark.sql.optimizer.planChangeLog.leve… [SPARK-27088][SQL] Add a configuration to set log level for each batch at RuleExecutor Mar 25, 2019
@SparkQA
Copy link

SparkQA commented Mar 26, 2019

Test build #103991 has finished for PR 24136 at commit 59b45d1.

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

@chakravarthiT
Copy link
Contributor Author

chakravarthiT commented Mar 27, 2019 •
edited
Loading

@maropu @HyukjinKwon handled review comments. and now that UT is passed,as message will be evaluated only when specific log level is set.

gatorsmile
gatorsmile reviewed Mar 27, 2019
View reviewed changes
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/rules/RuleExecutor.scala
} else {
logTrace(s"Batch ${batch.name} has no effect.")
}
planChangeLogger.logBatch(batch.name, batchStartPlan, curPlan)
Copy link
Member

Choose a reason for hiding this comment

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

Do the same thing for line 148?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The config "spark.sql.optimizer.planChangeLog.level", is used for logging plan changes after applying rule or batch . As per Line 148,it is logging when there is no change in plan,which conflicts the parameter(spark.sql.optimizer.planChangeLog.level). Its not required to log this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@gatorsmile please review

Copy link
Member

Choose a reason for hiding this comment

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

Please address them in a followup if there are further comments.

@gatorsmile
Copy link
Member

LGTM except the above comment.

@HyukjinKwon
Copy link
Member

retest this please

@SparkQA
Copy link

SparkQA commented Apr 10, 2019

Test build #104485 has finished for PR 24136 at commit 59b45d1.

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

@HyukjinKwon
Copy link
Member

Merged to master.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gengliangwang gengliangwang gengliangwang left review comments

@HyukjinKwon HyukjinKwon HyukjinKwon approved these changes

@gatorsmile gatorsmile gatorsmile left review comments

@maropu maropu maropu approved these changes

@maryannxue maryannxue maryannxue approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@chakravarthiT @HyukjinKwon @gatorsmile @SparkQA @maropu @gengliangwang @maryannxue