HyukjinKwon commented on code in PR #59320:
URL: https://github.com/apache/spark/pull/59320#discussion_r4258683487
##########
sql/catalyst/benchmarks/RetainedResolutionConfigsBenchmark-results.txt:
##########
@@ -0,0 +1,382 @@
+================================================================================================
+view: 32 settings, 25% retained, warm
+================================================================================================
+
+OpenJDK 64-Bit Server VM 17.0.15+6-Ubuntu-0ubuntu120.04 on Linux
5.4.0-1163-aws-fips
Review Comment:
**Nit (P3):** These numbers come from a local machine (Ubuntu OpenJDK
17.0.15 on Linux 5.4.0-aws-fips, Xeon 8375C). The other result sets in
`sql/catalyst/benchmarks/` are generated by the "Run benchmarks" GitHub Actions
workflow and include `-jdk21-results.txt` and `-jdk25-results.txt`, so this
file can't be compared with them or regenerated the same way. Could you
regenerate it with that workflow for JDK 17, 21, and 25? The `create-commit`
option can push the results to this branch.
**Verification:**
- **Inspection:** All three JDK result files exist for the benchmark, and
their environment headers match the workflow runner environment used by the
other committed catalyst benchmark results.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala:
##########
@@ -326,15 +326,30 @@ object Analyzer {
newConf: SQLConf,
existingConf: SQLConf,
createSparkVersion: String = ""): Unit = {
- existingConf.getAllConfs.foreach { case (key, value) =>
- if (key.startsWith("spark.sql.catalog.") ||
SQLConf.isSessionBindingPolicy(key)) {
- newConf.settings.put(key, value)
+ // The retained configs are memoized on `existingConf` unless
+ // `ANALYZER_MEMOIZE_RETAINED_RESOLUTION_CONFIGS` is off, which recomputes
them on every build.
+ if
(existingConf.getConf(SQLConf.ANALYZER_MEMOIZE_RETAINED_RESOLUTION_CONFIGS)) {
+ newConf.settings.putAll(existingConf.retainedResolutionConfigs)
+ } else {
+ existingConf.getAllConfs.foreach { case (key, value) =>
+ if (bodyFollowsCaller(key)) {
+ newConf.settings.put(key, value)
+ }
}
}
trySetAnsiValue(newConf, createSparkVersion)
}
+ /**
+ * True if a view or SQL function body takes the caller's value for `key`
rather than the
Review Comment:
**Nit (P3):** This reads as if the body always takes the caller's value for
these keys, but the overlay copies only keys the caller has explicitly set, and
only where session overrides are applied. For example, if a view is created
while a SESSION-bound key such as
`spark.sql.analyzer.lowerEmptyGroupingSetToGlobalAggregate.enabled` is
explicitly `false` and the caller later `RESET`s it, the body keeps the saved
`false`. Function confs built by `SessionCatalog` (`applySessionOverrides =
false`) also keep saved values. Could the wording be narrowed, e.g. "True if
the caller's explicitly set value for `key` is overlaid onto view and SQL
function body confs (when session overrides apply), replacing the value saved
with the body: catalog configs and SQL configs bound SESSION or
NOT_APPLICABLE."?
**Verification:**
- **Inspection:** The revised doc matches the overlay behavior: only
explicitly set caller keys selected by the predicate override saved values, and
only on paths that call retainResolutionConfigsForAnalysis.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala:
##########
@@ -2893,6 +2935,19 @@ object SQLConf {
.booleanConf
.createWithDefault(true)
+ val ANALYZER_MEMOIZE_RETAINED_RESOLUTION_CONFIGS =
+ buildConf("spark.sql.analyzer.memoizeRetainedResolutionConfigs.enabled")
+ .internal()
+ .version("4.4.0")
+ .withBindingPolicy(ConfigBindingPolicy.SESSION)
Review Comment:
**Nit (P3):** The `ConfigBindingPolicy` guidance for new configs says: "Can
the config change the result of resolving the body of a view/UDF/procedure,
i.e. the resolved plan? If not, use NOT_APPLICABLE." Since both values of this
flag produce the same body conf, `NOT_APPLICABLE` seems like the right label.
`isSessionBindingPolicy` retains `NOT_APPLICABLE` keys too, so nested bodies
would still inherit the caller's explicitly set value.
**Verification:**
- **Inspection:** The config's declared binding policy is NOT_APPLICABLE, as
the ConfigBindingPolicy selection rule requires for a config that cannot change
the resolved body plan.
- **Compatibility:** Memo-on and memo-off view and SQL function body confs
remain identical, and an explicitly set flag value still propagates into nested
body confs.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/internal/ModificationCountingMap.scala:
##########
@@ -0,0 +1,146 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.spark.sql.internal
+
+import java.util
+import java.util.function.{BiConsumer, BiFunction, Function => JFunction}
+
+/**
+ * A thread-safe map of [[SQLConf]] settings that counts the mutating calls on
it, so values
+ * derived from its entries can be reused while [[modificationCount]] is
unchanged.
+ *
+ * Like the `java.util.Collections.synchronizedMap` it replaces, every method
of the map
+ * synchronizes on the map itself, so a caller iterating the map must hold its
monitor. Unlike it,
+ * the key, value, and entry views are read-only, so every mutation goes
through a method that
+ * advances the count, and unsynchronized, so a caller must hold the monitor
while using them.
+ */
+private[sql] final class ModificationCountingMap extends util.Map[String,
String] {
+
+ private val underlying = new util.HashMap[String, String]
+
+ private val readOnlyUnderlying = util.Collections.unmodifiableMap[String,
String](underlying)
+
+ @volatile private var mutations = 0L
+
+ /**
+ * Number of mutating calls so far; a call may advance it without changing
the entries. Values
+ * derived from the entries stay valid while it is unchanged; use
+ * [[snapshotWithModificationCount]] to pair a count with the entries it
describes.
+ */
+ def modificationCount: Long = mutations
+
+ /** A copy of the entries and the [[modificationCount]] they correspond to.
*/
+ def snapshotWithModificationCount: (Long, util.Map[String, String]) =
synchronized {
+ (mutations, new util.HashMap[String, String](underlying))
+ }
+
+ private def mutate[T](f: => T): T = synchronized {
Review Comment:
**Nit (P3):** `getAllConfs` (`settings.synchronized { ... }`),
`setConf(Properties)`, and the `RuntimeConfig` PythonWorkerEnvironment
check-then-write all depend on these methods locking the map itself.
`Collections.synchronizedMap` guaranteed that before; now it's implemented
here, and none of the new tests would fail if `mutate` (or the reads) switched
to a private lock. A small test would pin it down. For example, assert
`Thread.holdsLock(map)` inside a `compute`/`merge` callback, or check that a
writer thread blocks while the test holds `map.synchronized`.
**Verification:**
- **Regression:** The new test passes against the current implementation and
fails when mutate is changed to use a different lock or no lock.
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
