sebastianhillig-db commented on code in PR #59320: URL: https://github.com/apache/spark/pull/59320#discussion_r4270837826
########## 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: Regenerated with the "Run benchmarks" workflow for JDK 17, 21, and 25 ([17](https://github.com/sebastianhillig-db/spark/actions/runs/38560201246), [21](https://github.com/sebastianhillig-db/spark/actions/runs/38560212037), [25](https://github.com/sebastianhillig-db/spark/actions/runs/38560222559)); the results are in 8a305bd698d (JDK 21), 6bc31ddc15e (JDK 17), and b6c4ad9d489 (JDK 25). ########## 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: Narrowed in 7f92399c393 and 062accf4ee6. The doc now says `retainResolutionConfigsForAnalysis` overlays the caller's explicitly set value, that keys the caller hasn't set keep their saved values, and that confs built without that method (such as the function confs from `SessionCatalog`) don't consult the predicate. ########## 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: Changed to `NOT_APPLICABLE` in 3876ddb700e. `isSessionBindingPolicy` retains `NOT_APPLICABLE` keys, so nested bodies still inherit an explicitly set value; the on/off equivalence tests are unchanged and pass. ########## 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: Added in 088f0fa6c67. The test holds the map's monitor and checks that each mutator, `snapshotWithModificationCount`, and each read blocks until it's released. It fails when `mutate` uses a private lock or no lock, and when `get` is unsynchronized. -- 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]
