[SPARK-21092][SQL] Wire SQLConf in logical plan and expressions #18299

rxin · 2017-06-14T07:24:36Z

What changes were proposed in this pull request?

It is really painful to not have configs in logical plan and expressions. We had to add all sorts of hacks (e.g. pass SQLConf explicitly in functions). This patch exposes SQLConf in logical plan, using a thread local variable and a getter closure that's set once there is an active SparkSession.

The implementation is a bit of a hack, since we didn't anticipate this need in the beginning (config was only exposed in physical plan). The implementation is described in SQLConf.get.

In terms of future work, we should follow up to clean up CBO (remove the need for passing in config).

How was this patch tested?

Updated relevant tests for constraint propagation.

rxin · 2017-06-14T07:25:13Z

Note that this patch is based on #18298. Once we merge that one the diff will become smaller.

Update: #18298 was merged. This is ready.

rxin · 2017-06-14T07:27:02Z

cc @wzhfy for review / CBO

cc @sameeragarwal this is what we discussed.

SparkQA · 2017-06-14T09:09:09Z

Test build #78036 has finished for PR 18299 at commit ced6e9b.

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

SparkQA · 2017-06-14T09:21:12Z

Test build #78038 has finished for PR 18299 at commit f059522.

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

wzhfy · 2017-06-14T18:17:29Z

Since we have a thread local variable in SQLConf, can we just provide getter/setter methods, get it in QueryPlan and set it in SparkSession?

rxin · 2017-06-14T21:36:19Z

The issue is that SparkSession might change the way they are wired and it's not always the case that when we create a new thread, we set the thread local conf.

sameeragarwal · 2017-06-14T21:43:32Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/QueryPlanConstraints.scala

+          .union(constructIsNotNullConstraints(validConstraints))
+          .filter { c =>
+            c.references.nonEmpty && c.references.subsetOf(outputSet) && c.deterministic
+          }


should we also bound their size as you suggested?

That belongs in a separate patch.

wzhfy · 2017-06-14T23:11:52Z

Got it, thanks.

wzhfy · 2017-06-14T23:13:33Z

...catalyst/src/test/scala/org/apache/spark/sql/catalyst/plans/ConstraintPropagationSuite.scala

+    assert(aliasedRelation.analyze.constraints.nonEmpty)
+
+    SQLConf.get.setConf(SQLConf.CONSTRAINT_PROPAGATION_ENABLED, false)
+    assert(aliasedRelation.analyze.constraints.isEmpty)


unset the config in the end

wzhfy · 2017-06-14T23:17:37Z

LGTM except one minor comment.

SparkQA · 2017-06-14T23:17:52Z

Test build #78068 has finished for PR 18299 at commit ea09164.

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

SparkQA · 2017-06-15T01:13:12Z

Test build #78070 has finished for PR 18299 at commit 14f2b41.

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

SparkQA · 2017-06-15T01:13:49Z

Test build #78071 has finished for PR 18299 at commit a032106.

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

SparkQA · 2017-06-15T04:09:29Z

Test build #78077 has finished for PR 18299 at commit cec78b5.

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

rxin · 2017-06-15T05:11:46Z

Merging in master.

cloud-fan · 2017-06-16T00:16:37Z

sql/core/src/main/scala/org/apache/spark/sql/SparkSession.scala

@@ -87,6 +87,11 @@ class SparkSession private(

  sparkContext.assertNotStopped()

+  // If there is no active SparkSession, uses the default SQL conf. Otherwise, use the session's.
+  SQLConf.setSQLConfGetter(() => {
+    SparkSession.getActiveSession.map(_.sessionState.conf).getOrElse(SQLConf.getFallbackConf)


shall we move this to SparkSession.Builder.getOrCreate?

## What changes were proposed in this pull request? It is really painful to not have configs in logical plan and expressions. We had to add all sorts of hacks (e.g. pass SQLConf explicitly in functions). This patch exposes SQLConf in logical plan, using a thread local variable and a getter closure that's set once there is an active SparkSession. The implementation is a bit of a hack, since we didn't anticipate this need in the beginning (config was only exposed in physical plan). The implementation is described in `SQLConf.get`. In terms of future work, we should follow up to clean up CBO (remove the need for passing in config). ## How was this patch tested? Updated relevant tests for constraint propagation. Author: Reynold Xin <rxin@databricks.com> Closes apache#18299 from rxin/SPARK-21092.

… conf in LogicalPlan ## What changes were proposed in this pull request? After wiring `SQLConf` in logical plan ([PR 18299](apache#18299)), we can remove the need of passing `conf` into `def stats` and `def computeStats`. ## How was this patch tested? Covered by existing tests, plus some modified existing tests. Author: wangzhenhua <wangzhenhua@huawei.com> Author: Zhenhua Wang <wzh_zju@163.com> Closes apache#18391 from wzhfy/removeConf.

It is really painful to not have configs in logical plan and expressions. We had to add all sorts of hacks (e.g. pass SQLConf explicitly in functions). This patch exposes SQLConf in logical plan, using a thread local variable and a getter closure that's set once there is an active SparkSession. The implementation is a bit of a hack, since we didn't anticipate this need in the beginning (config was only exposed in physical plan). The implementation is described in `SQLConf.get`. In terms of future work, we should follow up to clean up CBO (remove the need for passing in config). Updated relevant tests for constraint propagation. Author: Reynold Xin <rxin@databricks.com> Closes apache#18299 from rxin/SPARK-21092.

rxin changed the title ~~Spark 21092~~ [SPARK-21092][SQL] Wire SQLConf in logical plan and expressions Jun 14, 2017

rxin added 3 commits June 14, 2017 14:30

[SPARK-21092][SQL] Wire SQLConf in logical plan and expressions

90cff62

More docs

b03af7f

Merge with master

ea09164

rxin force-pushed the SPARK-21092 branch from f059522 to ea09164 Compare June 14, 2017 21:35

sameeragarwal reviewed Jun 14, 2017

View reviewed changes

wzhfy reviewed Jun 14, 2017

View reviewed changes

rxin added 2 commits June 14, 2017 16:25

Update PruneFilter rule.

14f2b41

Properly unset configs.

a032106

Update OuterJoinEliminationSuite

cec78b5

rxin mentioned this pull request Jun 15, 2017

[SPARK-21103][SQL] QueryPlanConstraints should be part of LogicalPlan #18310

Closed

asfgit closed this in fffeb6d Jun 15, 2017

cloud-fan reviewed Jun 16, 2017

View reviewed changes

wzhfy mentioned this pull request Jun 22, 2017

[SPARK-21180][SQL] Remove conf from stats functions since now we have conf in LogicalPlan #18391

Closed

gatorsmile mentioned this pull request Jul 7, 2017

[SPARK-21345][SQL][TEST][test-maven] SparkSessionBuilderSuite should clean up stopped sessions. #18567

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-21092][SQL] Wire SQLConf in logical plan and expressions #18299

[SPARK-21092][SQL] Wire SQLConf in logical plan and expressions #18299

rxin commented Jun 14, 2017 •

edited

Loading

rxin commented Jun 14, 2017 •

edited

Loading

rxin commented Jun 14, 2017 •

edited

Loading

SparkQA commented Jun 14, 2017

SparkQA commented Jun 14, 2017

wzhfy commented Jun 14, 2017

rxin commented Jun 14, 2017

sameeragarwal Jun 14, 2017

rxin Jun 14, 2017

wzhfy commented Jun 14, 2017

wzhfy Jun 14, 2017

wzhfy commented Jun 14, 2017

SparkQA commented Jun 14, 2017

SparkQA commented Jun 15, 2017

SparkQA commented Jun 15, 2017

SparkQA commented Jun 15, 2017

rxin commented Jun 15, 2017

cloud-fan Jun 16, 2017

[SPARK-21092][SQL] Wire SQLConf in logical plan and expressions #18299

[SPARK-21092][SQL] Wire SQLConf in logical plan and expressions #18299

Conversation

rxin commented Jun 14, 2017 • edited Loading

What changes were proposed in this pull request?

How was this patch tested?

rxin commented Jun 14, 2017 • edited Loading

rxin commented Jun 14, 2017 • edited Loading

SparkQA commented Jun 14, 2017

SparkQA commented Jun 14, 2017

wzhfy commented Jun 14, 2017

rxin commented Jun 14, 2017

sameeragarwal Jun 14, 2017

Choose a reason for hiding this comment

rxin Jun 14, 2017

Choose a reason for hiding this comment

wzhfy commented Jun 14, 2017

wzhfy Jun 14, 2017

Choose a reason for hiding this comment

wzhfy commented Jun 14, 2017

SparkQA commented Jun 14, 2017

SparkQA commented Jun 15, 2017

SparkQA commented Jun 15, 2017

SparkQA commented Jun 15, 2017

rxin commented Jun 15, 2017

cloud-fan Jun 16, 2017

Choose a reason for hiding this comment

rxin commented Jun 14, 2017 •

edited

Loading

rxin commented Jun 14, 2017 •

edited

Loading

rxin commented Jun 14, 2017 •

edited

Loading