jackylee-ch commented on code in PR #8328: URL: https://github.com/apache/incubator-gluten/pull/8328#discussion_r1897634116
########## shims/common/src/main/scala/org/apache/gluten/config/ConfigBuilder.scala: ########## @@ -0,0 +1,262 @@ +/* + * 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.gluten.config + +import org.apache.spark.network.util.ByteUnit +import org.apache.spark.network.util.JavaUtils + +import java.util.Locale +import java.util.concurrent.TimeUnit +import java.util.regex.Pattern; + +object BackendType extends Enumeration { + type BackendType = Value + val COMMON, VELOX, CLICKHOUSE = Value +} + +private[gluten] case class ConfigBuilder(key: String) { + import ConfigHelpers._ + + private[config] var _doc = "" + private[config] var _version = "" + private[config] var _backend = BackendType.COMMON + private[config] var _public = true + private[config] var _alternatives = List.empty[String] + private[config] var _onCreate: Option[ConfigEntry[_] => Unit] = None + + def doc(s: String): ConfigBuilder = { + _doc = s + this + } + + def version(s: String): ConfigBuilder = { + _version = s + this + } + + def backend(backend: BackendType.BackendType): ConfigBuilder = { + _backend = backend + this + } + + def internal(): ConfigBuilder = { + _public = false + this + } + + def onCreate(callback: ConfigEntry[_] => Unit): ConfigBuilder = { + _onCreate = Option(callback) + this + } + + def withAlternative(key: String): ConfigBuilder = { + _alternatives = _alternatives :+ key + this + } + + def intConf: TypedConfigBuilder[Int] = { + new TypedConfigBuilder(this, toNumber(_, _.toInt, key, "int")) + } + + def longConf: TypedConfigBuilder[Long] = { + new TypedConfigBuilder(this, toNumber(_, _.toLong, key, "long")) + } + + def doubleConf: TypedConfigBuilder[Double] = { + new TypedConfigBuilder(this, toNumber(_, _.toDouble, key, "double")) + } + + def booleanConf: TypedConfigBuilder[Boolean] = { + new TypedConfigBuilder(this, toBoolean(_, key)) + } + + def stringConf: TypedConfigBuilder[String] = { + new TypedConfigBuilder(this, identity) + } + + def timeConf(unit: TimeUnit): TypedConfigBuilder[Long] = { + new TypedConfigBuilder(this, timeFromString(_, unit), timeToString(_, unit)) + } + + def bytesConf(unit: ByteUnit): TypedConfigBuilder[Long] = { + new TypedConfigBuilder(this, byteFromString(_, unit), byteToString(_, unit)) + } + + def fallbackConf[T](fallback: ConfigEntry[T]): ConfigEntry[T] = { Review Comment: So, the `fallbackConf` means we would use the conf value from other configs espetially when current config not set, and the `withAlternative` means the config name changed. I'm ok with current meaning of `fallbackConf`, but for `withAlternative`, `deprecated` sounds better to me. The old config name is not recommended. Using `deperacated` can better remind users and make it easier for us to remove it in the future. A little case for `withAlternative`: ``` val EXTENDED_COLUMNAR_TRANSFORM_RULES = buildConf("spark.gluten.sql.columnar.extended.columnar.transform.rules") .withAlternative("spark.gluten.sql.columnar.extended.columnar.pre.rules") .doc("A comma-separated list of classes for the extended columnar transform rules.") .stringConf .createWithDefaultString("") ``` After: ``` val EXTENDED_COLUMNAR_PRE_RULES = buildConf("spark.gluten.sql.columnar.extended.columnar.pre.rules") .doc("A comma-separated list of classes for the extended columnar transform rules.") .deprecated('1.2.0') .stringConf .createWithDefaultString("") val EXTENDED_COLUMNAR_TRANSFORM_RULES = buildConf("spark.gluten.sql.columnar.extended.columnar.transform.rules") .doc("A comma-separated list of classes for the extended columnar transform rules.") .version('1.2.0') .stringConf .fallbackConf(EXTENDED_COLUMNAR_PRE_RULES) ``` @zhztheplayer any thoughs? ########## shims/common/src/main/scala/org/apache/gluten/config/ConfigBuilder.scala: ########## @@ -0,0 +1,229 @@ +/* + * 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.gluten.config + +import org.apache.spark.network.util.ByteUnit +import org.apache.spark.network.util.JavaUtils + +import java.util.concurrent.TimeUnit +import java.util.regex.Pattern; + +object BackendType extends Enumeration { + type BackendType = Value + val COMMON, VELOX, CLICKHOUSE = Value +} + +private[gluten] case class ConfigBuilder(key: String) { + import ConfigHelpers._ + + private[config] var _doc = "" + private[config] var _version = "" + private[config] var _backend = BackendType.COMMON + private[config] var _public = true + private[config] var _alternatives = List.empty[String] + private[config] var _onCreate: Option[ConfigEntry[_] => Unit] = None Review Comment: Got your means. We can add the `deprecated` here and check all the deprecated keys during plugin initialization. This can be done in separated pr. -- 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]
