[GLUTEN-8327][CORE] Introduce the ConfigEntry to make the config definition more flexible #8328

yikf · 2024-12-24T12:04:52Z

What changes were proposed in this pull request?

#8327 . subtask of #8326

How was this patch tested?

unit tests and GA

github-actions · 2024-12-24T12:05:11Z

#8327

github-actions · 2024-12-24T12:05:23Z

Run Gluten Clickhouse CI on x86

yikf · 2024-12-24T12:05:29Z

@zhztheplayer @jackylee-ch Could you please take a look, thanks!

jackylee-ch · 2024-12-25T01:28:57Z

shims/common/src/main/scala/org/apache/gluten/config/ConfigBuilder.scala

+  private val TIME_STRING_PATTERN = Pattern.compile("(-?[0-9]+)([a-z]+)?")
+
+  def timeFromString(str: String, unit: TimeUnit): Long = {
+    val lower = str.toLowerCase(Locale.ROOT).trim


Can we just use JavaUtils.timeStringAs?

jackylee-ch · 2024-12-25T01:30:32Z

shims/common/src/main/scala/org/apache/gluten/config/ConfigBuilder.scala

+    new TypedConfigBuilder(this, byteFromString(_, unit), byteToString(_, unit))
+  }
+
+  def fallbackConf[T](fallback: ConfigEntry[T]): ConfigEntry[T] = {


any different between withAlternatives and fallbackConf? maybe only use one of them?

The fallbackConf is a complete ConfigEntry, including doc, version information, etc. withAlternatives is just a key.

withAlternative should be enough if we have some config name changed.

The meanings expressed are still different. "Alternative" indicates a choice, while for "rename", what we aim to do is to eventually deprecated it, and it should contain more warning information.

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?

Oh, vanilla Spark already has some cases for this kind of definition.

val ARROW_PYSPARK_FALLBACK_ENABLED = buildConf("spark.sql.execution.arrow.pyspark.fallback.enabled") .doc(s"When true, optimizations enabled by '${ARROW_PYSPARK_EXECUTION_ENABLED.key}' will " + "fallback automatically to non-optimized implementations if an error occurs.") .version("3.0.0") .fallbackConf(ARROW_FALLBACK_ENABLED) val ARROW_FALLBACK_ENABLED = buildConf("spark.sql.execution.arrow.fallback.enabled") .doc("(Deprecated since Spark 3.0, please set " + "'spark.sql.execution.arrow.pyspark.fallback.enabled'.)") .version("2.4.0") .booleanConf .createWithDefault(true)

@jackylee-ch

Thank you for sharing ideas!

I see Spark's SQLConf.scala uses both fallbackConf (as mentioned) and withAlternative for this kind of key routiing functionality. So would you suggest adding another deprecated API to emphasis on the deprecation of the configuration options?

So would you suggest adding another deprecated API to emphasis on the deprecation of the configuration options?

Yeap, and the withAlternative can be changed with deprecated.

SGTM, will follow this approach at the subsequent PR to change the configuration name.

See you guys' points. Temporarily, I think it's reasonable to keep the first PR aligned with vanilla Spark's manner. Let's see if we could use our own way to handle the deprecated options in subsequent efforts. Thanks!

shims/common/src/main/scala/org/apache/gluten/config/GlutenConfig.scala

github-actions · 2024-12-25T03:38:25Z

Run Gluten Clickhouse CI on x86

github-actions · 2024-12-25T06:03:38Z

Run Gluten Clickhouse CI on x86

zhztheplayer

Thanks for taking on the configuration enhancements. This series of work could be a big millstone for Gluten's overall code quality.

Also, would you like to add some comments on the newly added config-related classes (if needed), mentioning that the code is similar to Spark's relevant code but extended for Gluten's use? So users will know the difference here more quickly.

zhztheplayer · 2024-12-25T07:32:55Z

shims/common/src/main/scala/org/apache/gluten/config/ConfigEntry.scala

+
+  override def version: String = _version
+
+  override def backend: BackendType = _backend


It seems that this API is not yet called. This is designed for future use, right?

zhztheplayer · 2024-12-25T07:43:24Z

shims/common/src/main/scala/org/apache/gluten/config/ConfigEntry.scala

+  override def defaultValueString: String = stringConverter(_defaultVal)
+}
+
+private class ConfigEntryWithDefaultString[T](


This one is private (actually equivalent to private[config]) while others are private[gluten]. Is there any particular reason?

no, will keep consistent.

github-actions · 2024-12-25T09:06:34Z

Run Gluten Clickhouse CI on x86

github-actions · 2024-12-25T09:10:22Z

Run Gluten Clickhouse CI on x86

github-actions · 2024-12-25T09:13:12Z

Run Gluten Clickhouse CI on x86

github-actions · 2024-12-25T09:44:04Z

Run Gluten Clickhouse CI on x86

github-actions · 2024-12-25T09:51:22Z

Run Gluten Clickhouse CI on x86

jackylee-ch · 2024-12-25T10:30:12Z

shims/common/src/main/scala/org/apache/gluten/config/ConfigBuilder.scala

+    new TypedConfigBuilder(this, byteFromString(_, unit), byteToString(_, unit))
+  }
+
+  def fallbackConf[T](fallback: ConfigEntry[T]): ConfigEntry[T] = {


withAlternative should be enough if we have some config name changed.

jackylee-ch · 2024-12-25T10:32:50Z

shims/common/src/main/scala/org/apache/gluten/config/ConfigBuilder.scala

+  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


Maybe we can add deprecated here, so we can deprecated some configs and warning user while user setting it.

It should not be the case, maybe. onCreate occurs when the entry is created and it always happens. However, deprecation should take place when the user actively sets a deprecated key. Checking for deprecated keys from sparkConf during plugin initialization is more appropriate and will only issue a warning once.

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.

zhztheplayer · 2024-12-26T08:47:13Z

@yikf Would you rebase? Thanks!
@jackylee-ch If you have further review comments? Thanks!

github-actions · 2024-12-26T09:14:43Z

Run Gluten Clickhouse CI on x86

jackylee-ch

LGTM

github-actions · 2024-12-27T08:17:30Z

Run Gluten Clickhouse CI on x86

github-actions · 2024-12-27T09:32:10Z

Run Gluten Clickhouse CI on x86

github-actions · 2024-12-30T02:37:28Z

Run Gluten Clickhouse CI on x86

github-actions · 2024-12-30T03:32:31Z

Run Gluten Clickhouse CI on x86

yikf · 2024-12-30T06:04:07Z

@jackylee-ch @zhztheplayer Kindly ping, Could you take a look another? and help re-run the failed pipeline : )

zhztheplayer · 2024-12-30T06:22:11Z

Thanks for the contribution and reviews!

yikf · 2024-12-30T08:06:53Z

@jackylee-ch @zhztheplayer Thanks all guys!

baibaichen · 2024-12-31T03:43:39Z

@yikf would we directly use org.apache.spark.ConfigEntry without defining a new ConfigEntry trait

yikf · 2024-12-31T05:58:47Z

@yikf would we directly use org.apache.spark.ConfigEntry without defining a new ConfigEntry trait

Hi @baibaichen , Thank you for your well-considered insights. We hope to use a custom ConfigEntry to achieve more flexible configuration. You can refer to the related discussion at #6970 (comment).

…fig definition more flexible (apache#8328)" This reverts commit 3c53f9f.

…fig definition more flexible (#8328)" (#8382) This reverts commit 3c53f9f.

github-actions bot added CORE works for Gluten Core VELOX RSS CLICKHOUSE DATA_LAKE labels Dec 24, 2024

yikf mentioned this pull request Dec 24, 2024

[CORE][umbrella] Configuration enhancements #8326

Open

4 tasks

jackylee-ch reviewed Dec 25, 2024

View reviewed changes

zhztheplayer changed the title ~~[GLUTEN-8327][CORE]Introduce the ConfigEntry to make the config definition more flexible.~~ [GLUTEN-8327][CORE] Introduce the ConfigEntry to make the config definition more flexible. Dec 25, 2024

yikf force-pushed the config-entry branch from 01e21ec to 7eeacda Compare December 25, 2024 03:37

yikf force-pushed the config-entry branch from 7eeacda to 017e4e3 Compare December 25, 2024 06:03

zhztheplayer reviewed Dec 25, 2024

View reviewed changes

yikf force-pushed the config-entry branch from 017e4e3 to 72d3346 Compare December 25, 2024 09:06

yikf force-pushed the config-entry branch from 72d3346 to 88e1b07 Compare December 25, 2024 09:09

yikf force-pushed the config-entry branch from a9444a1 to 5e6ceb1 Compare December 25, 2024 09:43

jackylee-ch reviewed Dec 25, 2024

View reviewed changes

zhztheplayer changed the title ~~[GLUTEN-8327][CORE] Introduce the ConfigEntry to make the config definition more flexible.~~ [GLUTEN-8327][CORE] Introduce the ConfigEntry to make the config definition more flexible Dec 26, 2024

yikf force-pushed the config-entry branch from 803b893 to e439972 Compare December 26, 2024 09:14

jackylee-ch approved these changes Dec 27, 2024

View reviewed changes

yikf force-pushed the config-entry branch from 31c318e to 68aec23 Compare December 27, 2024 09:31

yikf force-pushed the config-entry branch from 68aec23 to 9e6dfd1 Compare December 30, 2024 02:36

Introduce ConfigEntry for Gluten config

f9f4e7b

yikf force-pushed the config-entry branch from 9e6dfd1 to f9f4e7b Compare December 30, 2024 03:32

jackylee-ch merged commit 3c53f9f into apache:main Dec 30, 2024
46 checks passed

yikf deleted the config-entry branch December 30, 2024 08:06

baibaichen added a commit to baibaichen/gluten that referenced this pull request Dec 31, 2024

Revert "[GLUTEN-8327][CORE] Introduce the ConfigEntry to make the con…

6800f39

…fig definition more flexible (apache#8328)" This reverts commit 3c53f9f.

baibaichen mentioned this pull request Dec 31, 2024

Revert "[GLUTEN-8327][CORE] Introduce the ConfigEntry to make the config definition more flexible (#8328)" #8382

Merged

zhztheplayer pushed a commit that referenced this pull request Jan 1, 2025

Revert "[GLUTEN-8327][CORE] Introduce the ConfigEntry to make the con…

7047532

…fig definition more flexible (#8328)" (#8382) This reverts commit 3c53f9f.


		override def version: String = _version

		override def backend: BackendType = _backend

[GLUTEN-8327][CORE] Introduce the ConfigEntry to make the config definition more flexible #8328

[GLUTEN-8327][CORE] Introduce the ConfigEntry to make the config definition more flexible #8328

Conversation

yikf commented Dec 24, 2024

What changes were proposed in this pull request?

How was this patch tested?

github-actions bot commented Dec 24, 2024

github-actions bot commented Dec 24, 2024

yikf commented Dec 24, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jackylee-ch Dec 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhztheplayer Dec 26, 2024 • edited Loading

Choose a reason for hiding this comment

github-actions bot commented Dec 25, 2024

github-actions bot commented Dec 25, 2024

zhztheplayer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Dec 25, 2024

github-actions bot commented Dec 25, 2024

github-actions bot commented Dec 25, 2024

github-actions bot commented Dec 25, 2024

github-actions bot commented Dec 25, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhztheplayer commented Dec 26, 2024

github-actions bot commented Dec 26, 2024

jackylee-ch left a comment

Choose a reason for hiding this comment

github-actions bot commented Dec 27, 2024

github-actions bot commented Dec 27, 2024

github-actions bot commented Dec 30, 2024

github-actions bot commented Dec 30, 2024

yikf commented Dec 30, 2024 • edited Loading

zhztheplayer commented Dec 30, 2024

yikf commented Dec 30, 2024

baibaichen commented Dec 31, 2024 • edited Loading

yikf commented Dec 31, 2024

jackylee-ch Dec 26, 2024 •

edited

Loading

zhztheplayer Dec 26, 2024 •

edited

Loading

yikf commented Dec 30, 2024 •

edited

Loading

baibaichen commented Dec 31, 2024 •

edited

Loading