liuneng1994 commented on code in PR #13380:
URL: https://github.com/apache/gluten/pull/13380#discussion_r4345086113
##########
backends-bolt/src/main/scala/org/apache/gluten/config/BoltConfig.scala:
##########
@@ -216,7 +218,14 @@ object BoltConfig extends ConfigRegistry {
SPARK_HADOOP_PARQUET_ENCRYPTION_KMS_MAX_RETRY_TIMES,
SPARK_HADOOP_PARQUET_ENCRYPTION_KMS_INSTANCE_VERSION,
SPARK_HADOOP_PARQUET_ENCRYPTION_KMS_CLIENT_CLASS
- )
+ ) ++ {
+ // Spark 3.4 always chunks Base64 and ignores this unregistered SQL key.
+ if (SQLConf.get.getAllDefinedConfs.exists(_._1 ==
SPARK_CHUNK_BASE64_STRING_ENABLED_KEY)) {
Review Comment:
`EXTRA_NATIVE_SESSION_CONF_KEYS` is a `lazy val`, so this check runs once
per JVM and the result is frozen at first access. In a long-lived executor with
multiple sessions, a key set later (or only in another session) will never be
forwarded to native. `getAllDefinedConfs` may also include defaults, making the
condition effectively always true. Please evaluate this per access
(`SQLConf.get.getConfString(key, null) != null`) or add the key unconditionally.
##########
gluten-substrait/src/main/scala/org/apache/gluten/backendsapi/SparkPlanExecApi.scala:
##########
@@ -228,6 +228,8 @@ trait SparkPlanExecApi {
GenericExpressionTransformer(substraitExprName, child, expr)
}
+ def genSparkVersionStaticInvokeTransformer(): Option[ExpressionTransformer]
= None
Review Comment:
The literal this produces (`SPARK_VERSION_SHORT + " " + SPARK_REVISION`) is
backend-agnostic; `ExpressionConverter` already returns the same value for the
`SparkVersion` expression for every backend. Making `getSparkVersion` an opt-in
hook therefore creates a Bolt-vs-Velox/CH divergence for no semantic reason.
Could this be handled unconditionally in `ExpressionConverter`? If the opt-in
is deliberate, please document why.
##########
backends-bolt/src/main/scala/org/apache/gluten/expression/aggregate/BoltBloomFilterAggregate.scala:
##########
@@ -54,14 +54,12 @@ case class BoltBloomFilterAggregate(
override def prettyName: String = "bolt_bloom_filter_agg"
- // Mark as lazy so that `estimatedNumItems` is not evaluated during tree
transformation.
- private lazy val estimatedNumItems: Long =
- Math.min(
- estimatedNumItemsExpression.eval().asInstanceOf[Number].longValue,
- SQLConf.get
- .getConfString("spark.sql.optimizer.runtime.bloomFilter.maxNumItems",
"4000000")
- .toLong
- )
+ // Match the native aggregate's capacity without evaluating numBits during
transformation.
+ private lazy val capacityFromNumBits: Int = {
Review Comment:
`capacity = numBits / 16` assumes the native bloom filter consumes 16 bits
per item. Please point at the native implementation/constant that establishes
this ratio (and that `boltBloomFilterMaxNumBits` is in bits, not capacity),
since a wrong ratio changes false-positive rates instead of failing loudly.
##########
backends-bolt/src/main/scala/org/apache/gluten/expression/ExpressionTransformer.scala:
##########
@@ -120,6 +121,14 @@ case class ToUnixTimestampTransformer(
override def children: Seq[ExpressionTransformer] = {
timeExpTransformer.dataType match {
case _: TimestampType => Seq(timeExpTransformer)
+ case _: DateType =>
+ val timezone =
original.asInstanceOf[TimeZoneAwareExpression].timeZoneId.getOrElse {
+ throw new GlutenNotSupportException(
+ "DATE unix timestamp requires a resolved time zone")
+ }
+ // This native overload ignores the format for DATE inputs, including
a null format,
+ // while using the expression's time zone rather than the session
default.
+ Seq(timeExpTransformer, formatTransformer,
LiteralTransformer(timezone))
Review Comment:
This changes the `to_unix_timestamp` arity for DATE inputs to three
arguments. Please link the native function signature/registration that accepts
`(date, format, timezone)` and confirm the format argument is ignored for DATE
(as the comment says); otherwise this fails at native execution rather than
falling back.
--
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]
