[SPARK-46600][SQL] Move shared code between SqlConf and SqlApiConf to SqlApiConfHelper #44602

amaliujia · 2024-01-05T03:11:13Z

What changes were proposed in this pull request?

This code proposes to introduce a new object named SqlApiConfHelper to contain shared code between SqlApiConf and SqlConf.

Why are the changes needed?

As of now, SqlConf will access some of the variables of SqlApiConf while SqlApiConf also try to initialize SqlConf upon initialization. This PR is to avoid potential circular dependency between SqlConf and SqlApiConf. The shared variables or access to the shared variables are moved to the new SqlApiConfHelper. So either SqlApiConf and SqlConf wants to initialize the other side, they will only initialize the same third object.

Does this PR introduce any user-facing change?

NO

How was this patch tested?

Existing UT

Was this patch authored or co-authored using generative AI tooling?

NO

… another object.

amaliujia · 2024-01-05T16:44:47Z

@cloud-fan @hvanhovell

sql/api/src/main/scala/org/apache/spark/sql/internal/SqlApiConfHelper.scala

cloud-fan · 2024-01-05T17:06:05Z

sql/api/src/main/scala/org/apache/spark/sql/internal/SqlApiConf.scala

-  private[sql] def setConfGetter(getter: () => SqlApiConf): Unit = {
-    confGetter.set(getter)
-  }
+  private val confGetter = SqlApiConfHelper.getConfGetter


do we need this val? def get: SqlApiConf = SqlApiConfHelper.getConfGetter.get()()

Right. Consolidated.

zsxwing · 2024-01-05T18:50:13Z

sql/api/src/main/scala/org/apache/spark/sql/internal/SqlApiConfHelper.scala

+    new AtomicReference[() => SqlApiConf](() => DefaultSqlApiConf)
+  }
+
+  def getConfGetter: AtomicReference[() => SqlApiConf] = confGetter


If this line is called in a JVM before any code touches object SQLConf, are we going to return DefaultSqlApiConf which is wrong?

@hvanhovell Do you know if the scenario mentioned here could happen?

@zsxwing good point! How about this: We make SqlApiConf the only place to deal with the active conf problem. It should use reflection to get the active SQLConf and set it as the getter. SQLConf should only access this new SqlApiConfHelper for config names.

@zsxwing I don't think it is an issue. An end user should never touch this object unless they know what they are doing. That is a limitation of this approach, we should document this, and we can add a liniting rule if we must.

Just to make sure that we are on the same page. In a normal situation, the initialisation is as follows:

User touches the SQLConf object. This loads and initialises the SQLConf object. The loading of the SQLConf object triggers the initialisation of the SqlApiConfHelper because it needs to set the setter, and it needs to load the shared keys.

User touces the SqlApiConf object. This tries to load the SQLConf object as part of its initialisation (see previous bullet), if it is on the classpath the getter will be updated to return the SQLConf.

hvanhovell · 2024-01-08T21:34:16Z

sql/api/src/main/scala/org/apache/spark/sql/internal/SqlApiConfHelper.scala

+
+import java.util.concurrent.atomic.AtomicReference
+
+private[sql] object SqlApiConfHelper {


Can you add some documentation explaining why this is needed? And how this code should never be used by an end-user.

+1 to document the rules to use SqlApiConfHelper.

hvanhovell

LGTM

hvanhovell · 2024-01-09T02:21:21Z

Merging to master/3.5

… SqlApiConfHelper ### What changes were proposed in this pull request? This code proposes to introduce a new object named `SqlApiConfHelper` to contain shared code between `SqlApiConf` and `SqlConf`. ### Why are the changes needed? As of now, SqlConf will access some of the variables of SqlApiConf while SqlApiConf also try to initialize SqlConf upon initialization. This PR is to avoid potential circular dependency between SqlConf and SqlApiConf. The shared variables or access to the shared variables are moved to the new `SqlApiConfHelper`. So either SqlApiConf and SqlConf wants to initialize the other side, they will only initialize the same third object. ### Does this PR introduce _any_ user-facing change? NO ### How was this patch tested? Existing UT ### Was this patch authored or co-authored using generative AI tooling? NO Closes #44602 from amaliujia/refactor_sql_api. Authored-by: Rui Wang <[email protected]> Signed-off-by: Herman van Hovell <[email protected]> (cherry picked from commit 03fc5e2) Signed-off-by: Herman van Hovell <[email protected]>

…qlApiConf to SqlApiConfHelper (apache#655) [SPARK-46600][SQL] Move shared code between SqlConf and SqlApiConf to SqlApiConfHelper This code proposes to introduce a new object named `SqlApiConfHelper` to contain shared code between `SqlApiConf` and `SqlConf`. As of now, SqlConf will access some of the variables of SqlApiConf while SqlApiConf also try to initialize SqlConf upon initialization. This PR is to avoid potential circular dependency between SqlConf and SqlApiConf. The shared variables or access to the shared variables are moved to the new `SqlApiConfHelper`. So either SqlApiConf and SqlConf wants to initialize the other side, they will only initialize the same third object. NO Existing UT NO Closes apache#44602 from amaliujia/refactor_sql_api. Authored-by: Rui Wang <[email protected]> (cherry picked from commit 03fc5e2) Signed-off-by: Herman van Hovell <[email protected]> Co-authored-by: Rui Wang <[email protected]>

[SPARK-46600][SQL] Move shared code between SqlConf and SqlApiConf to…

9a5f0b5

… another object.

github-actions bot added the SQL label Jan 5, 2024

update

73b12de

cloud-fan reviewed Jan 5, 2024

View reviewed changes

sql/api/src/main/scala/org/apache/spark/sql/internal/SqlApiConfHelper.scala Outdated Show resolved Hide resolved

cloud-fan reviewed Jan 5, 2024

View reviewed changes

cloud-fan approved these changes Jan 5, 2024

View reviewed changes

update

e4997bf

zsxwing reviewed Jan 5, 2024

View reviewed changes

amaliujia force-pushed the refactor_sql_api branch from 0d989b0 to e4997bf Compare January 6, 2024 08:24

hvanhovell reviewed Jan 8, 2024

View reviewed changes

update

7ae3ac2

hvanhovell approved these changes Jan 8, 2024

View reviewed changes

cloud-fan approved these changes Jan 9, 2024

View reviewed changes

hvanhovell closed this in 03fc5e2 Jan 9, 2024

turboFei mentioned this pull request Dec 19, 2024

[SPARK-44284][CONNECT] Create simple conf system for sql/api #41838

Closed


		import java.util.concurrent.atomic.AtomicReference

		private[sql] object SqlApiConfHelper {

[SPARK-46600][SQL] Move shared code between SqlConf and SqlApiConf to SqlApiConfHelper #44602

[SPARK-46600][SQL] Move shared code between SqlConf and SqlApiConf to SqlApiConfHelper #44602

Uh oh!

Conversation

amaliujia commented Jan 5, 2024

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?

Was this patch authored or co-authored using generative AI tooling?

Uh oh!

amaliujia commented Jan 5, 2024

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zsxwing Jan 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hvanhovell left a comment

Choose a reason for hiding this comment

Uh oh!

hvanhovell commented Jan 9, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

zsxwing Jan 5, 2024 •

edited

Loading