jackylee-ch commented on code in PR #12549:
URL: https://github.com/apache/gluten/pull/12549#discussion_r3949763018
##########
gluten-substrait/src/main/scala/org/apache/gluten/config/GlutenConfig.scala:
##########
@@ -489,110 +490,155 @@ object GlutenConfig extends ConfigRegistry {
def prefixOf(backendName: String): String =
s"spark.gluten.sql.columnar.backend.$backendName"
def prefixSessionOf(backendName: String): String =
s"spark.gluten.$backendName"
- private lazy val nativeKeys = Set(
- DEBUG_ENABLED.key,
- BENCHMARK_SAVE_DIR.key,
- GlutenCoreConfig.COLUMNAR_TASK_OFFHEAP_SIZE_IN_BYTES.key,
- COLUMNAR_MAX_BATCH_SIZE.key,
- SHUFFLE_WRITER_BUFFER_SIZE.key,
- COLUMNAR_CUDF_ENABLED.key,
- SQLConf.LEGACY_SIZE_OF_NULL.key,
- SQLConf.LEGACY_STATISTICAL_AGGREGATE.key,
- SQLConf.JSON_GENERATOR_IGNORE_NULL_FIELDS.key,
- SQLConf.RUNTIME_BLOOM_FILTER_EXPECTED_NUM_ITEMS.key,
- SQLConf.RUNTIME_BLOOM_FILTER_NUM_BITS.key,
- SQLConf.RUNTIME_BLOOM_FILTER_MAX_NUM_BITS.key,
- SQLConf.RUNTIME_BLOOM_FILTER_MAX_NUM_ITEMS.key,
- "spark.io.compression.codec",
- "spark.sql.decimalOperations.allowPrecisionLoss",
- "spark.sql.legacy.parquet.returnNullStructIfAllFieldsMissing",
- // s3 config
- SPARK_S3_ACCESS_KEY,
- SPARK_S3_SECRET_KEY,
- SPARK_S3_ENDPOINT,
- SPARK_S3_CONNECTION_SSL_ENABLED,
- SPARK_S3_PATH_STYLE_ACCESS,
- SPARK_S3_USE_INSTANCE_CREDENTIALS,
- SPARK_S3_IAM,
- SPARK_S3_IAM_SESSION_NAME,
- SPARK_S3_RETRY_MAX_ATTEMPTS,
- SPARK_S3_CONNECTION_MAXIMUM,
- SPARK_S3_ENDPOINT_REGION,
- SPARK_S3_AWS_IMDS_ENABLED,
- "spark.gluten.velox.fs.s3a.retry.mode",
- "spark.gluten.velox.awsSdkLogLevel",
- "spark.gluten.velox.s3UseProxyFromEnv",
- "spark.gluten.velox.s3PayloadSigningPolicy",
- "spark.gluten.velox.s3LogLocation",
- // gcs config
- SPARK_GCS_STORAGE_ROOT_URL,
- SPARK_GCS_AUTH_TYPE,
- SPARK_GCS_AUTH_SERVICE_ACCOUNT_JSON_KEYFILE,
- SPARK_REDACTION_REGEX,
- "spark.gluten.sql.columnar.backend.velox.queryTraceEnabled",
- "spark.gluten.sql.columnar.backend.velox.queryTraceDir",
- "spark.gluten.sql.columnar.backend.velox.queryTraceNodeIds",
- "spark.gluten.sql.columnar.backend.velox.queryTraceMaxBytes",
- "spark.gluten.sql.columnar.backend.velox.queryTraceTaskRegExp",
- "spark.gluten.sql.columnar.backend.velox.opTraceDirectoryCreateConfig",
- "spark.gluten.sql.columnar.backend.velox.enableUserExceptionStacktrace",
- "spark.gluten.sql.columnar.backend.velox.enableSystemExceptionStacktrace",
- "spark.gluten.sql.columnar.backend.velox.memoryUseHugePages",
- "spark.gluten.sql.columnar.backend.velox.cachePrefetchMinPct",
-
"spark.gluten.sql.columnar.backend.velox.memoryPoolCapacityTransferAcrossTasks",
- "spark.gluten.sql.columnar.backend.velox.preferredBatchBytes",
- "spark.gluten.sql.columnar.backend.velox.cudf.enableTableScan",
-
"spark.gluten.sql.columnar.backend.velox.columnarBatchSerializerCompression"
- )
+ // Declarations of non-Gluten configurations (Spark SQL / Spark core /
Hadoop keys that have no
+ // Gluten ConfigEntry) to be passed to native side. `registerConf` /
`registerStaticConf` declare
+ // only the native delivery: the key stays owned by Spark / Hadoop, so
nothing is registered as a
+ // Gluten config entry or to SQLConf. Gluten's own configurations declare
native passing via
+ // `ConfigBuilder.passToNative` at their definitions instead.
+ private def registerNativeConfs(): Unit = {
+ // Force GlutenCoreConfig's object initialization, so that its own
`passToNative`
+ // registrations are in place before native confs are selected.
+ GlutenCoreConfig.ensureRegistered()
+
+ // Spark SQL confs read by native. All of these rely on native's own
fallback matching Spark's
+ // default, so nothing is delivered when the key is unset - see
`ConfigBuilder.passToNative`.
+ // `spark.sql.legacy.sizeOfNull` is `passToNative()` for documentation
purposes only: it is
+ // never read from the conf map, since the value is baked as a substrait
literal at plan
+ // conversion (see `ExpressionConverter`).
+
registerConf(SQLConf.LEGACY_SIZE_OF_NULL.key).stringConf.passToNative().createOptional
+ // Read by `ConfigExtractor` as a bool with its own fallback of `true`,
matching Spark's
+ // default. A string literal because not every supported Spark version has
the entry.
+ registerConf("spark.sql.legacy.parquet.returnNullStructIfAllFieldsMissing")
+ .booleanConf
+ .passToNative()
+ .createOptional
+ registerConf(SQLConf.JSON_GENERATOR_IGNORE_NULL_FIELDS.key)
+ .stringConf
+ .passToNative()
+ .createOptional
+ registerConf(SQLConf.RUNTIME_BLOOM_FILTER_EXPECTED_NUM_ITEMS.key)
+ .stringConf
+ .passToNative()
+ .createOptional
+ registerConf(SQLConf.RUNTIME_BLOOM_FILTER_NUM_BITS.key)
+ .stringConf
+ .passToNative()
+ .createOptional
+ registerConf(SQLConf.RUNTIME_BLOOM_FILTER_MAX_NUM_BITS.key)
+ .stringConf
+ .passToNative()
+ .createOptional
+ registerConf(SQLConf.RUNTIME_BLOOM_FILTER_MAX_NUM_ITEMS.key)
+ .stringConf
+ .passToNative()
+ .createOptional
+
registerConf(SPARK_IO_COMPRESSION_CODEC).stringConf.passToNative().createOptional
+ // Velox compares the value against upper-cased literals; ClickHouse
lower-cases it itself.
+ // Declaring `transform(toUpperCase)` mirrors Spark's own entry which also
upper-cases.
+ registerConf(SQLConf.LEGACY_TIME_PARSER_POLICY.key)
+ .stringConf
+ .transform(_.toUpperCase(Locale.ROOT))
+ .passToNative()
+ .createOptional
+
registerConf(SQLConf.CASE_SENSITIVE.key).stringConf.passToNative().createOptional
+
registerConf(SQLConf.IGNORE_MISSING_FILES.key).stringConf.passToNative().createOptional
+ registerConf(SQLConf.LEGACY_STATISTICAL_AGGREGATE.key)
+ .stringConf
+ .passToNative()
+ .createOptional
+ registerConf(SQLConf.DECIMAL_OPERATIONS_ALLOW_PREC_LOSS.key)
+ .stringConf
+ .passToNative()
+ .createOptional
Review Comment:
Fixed in `621ed689a` — and one of them was a real bug, thanks.
Eleven keys moved off `stringConf`:
- 9 booleans → `booleanConf`: `caseSensitive`, `ignoreMissingFiles`,
`legacy.statisticalAggregate`, `decimalOperations.allowPrecisionLoss`,
`json.generator.ignoreNullFields`, `legacy.sizeOfNull`, `ansi.enabled`,
`fs.s3a.aws.imds.enabled`, `parquet.writeLegacyFormat`
- the 4 `spark.sql.optimizer.runtime.bloomFilter.*` sizing keys →
`longConf`, matching Spark's own entries
The real bug is `spark.sql.decimalOperations.allowPrecisionLoss`. ClickHouse
holds it in `BOOL_VALUE_SETTINGS` (`cpp-ch/local-engine/Common/CHUtil.h:46-47`)
and `BackendInitializerUtil::toField` does `value == "true" || value == "1"`
(`cpp-ch/local-engine/Common/CHUtil.cpp:632`) — a **case-sensitive** compare.
So a user writing `TRUE` turned precision loss off in ClickHouse while Spark
read it as on. `booleanConf` renders it `true`.
The others are latent rather than live: Velox uses `boost::iequals` or
`folly::to<bool>` at its read sites, so it tolerated the raw case. Declaring
the converter Spark declares makes that a property of the declaration instead
of something each read site has to get right.
Added a `NativeConfPassingSuite` case for the normalization, including that
a value Spark itself would reject (`"yes"`) is still delivered unchanged rather
than failing conf selection — that runs per task, and Spark raises on it at its
own read site with its own message.
`spark.sql.mapKeyDedupPolicy` stays `stringConf`: Spark 4.1 declares it as
an enum (`MapKeyDedupPolicy.Value`) where 3.x declares it as a string, so a
typed builder cannot be written once for all supported versions.
--
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]
