felipepessoto commented on code in PR #12967:
URL: https://github.com/apache/gluten/pull/12967#discussion_r4029957127
##########
gluten-substrait/src/main/scala/org/apache/gluten/extension/columnar/validator/Validators.scala:
##########
@@ -273,17 +279,21 @@ object Validators {
case _ => false
}
val isSupportedNtz = plan match {
+ case _: HashAggregateExec | _: ObjectHashAggregateExec | _:
SortAggregateExec => true
+ case _: ShuffleExchangeExec => true
case p: ProjectExec =>
p.projectList.forall {
expr =>
(!containsNTZ(expr.dataType) &&
!expr.references.exists(a => containsNTZ(a.dataType))) ||
+ isDirectNtzProjection(expr) ||
expr.exists {
case Hour(child, _) => containsNTZ(child.dataType)
case Minute(child, _) => containsNTZ(child.dataType)
case Second(child, _) => containsNTZ(child.dataType)
case TimestampAdd(_, _, child, _) =>
containsNTZ(child.dataType)
case c: Cast if isNTZ(c.dataType) || isNTZ(c.child.dataType)
=> true
+ case IsNull(child) => containsNTZ(child.dataType)
Review Comment:
Added explicit `TIMESTAMP_UTC` coverage for Spark's `isnull` and `isnotnull`
in
[facebookincubator/velox#18963](https://github.com/facebookincubator/velox/pull/18963),
commit
[fdead606c](https://github.com/facebookincubator/velox/commit/fdead606cf2166c0e492fc5dc813d69a68f12685).
The shared case matrix covers flat, constant, and dictionary inputs,
including dictionary-level nulls and nulls in base vectors. Both predicates are
compared with explicit non-null Boolean expectations derived from the declared
null pattern, not from each other's output. No production implementation
changes were needed.
##########
docs/velox-configuration.md:
##########
@@ -29,7 +29,7 @@ nav_order: 16
| spark.gluten.sql.columnar.backend.velox.cudf.shuffleMaxPrefetchBytes
| 🔄 Dynamic | 1028MB | Maximum bytes to prefetch in CPU
memory during GPU shuffle read while waiting for GPU available.
|
| spark.gluten.sql.columnar.backend.velox.directorySizeGuess
| âš“ Static | 32KB | Deprecated, rename to
spark.gluten.sql.columnar.backend.velox.footerEstimatedSize
|
| spark.gluten.sql.columnar.backend.velox.driverSideBroadcastHashTableBuild
| 🔄 Dynamic | false | Enable driver-side broadcast hash
table build. When enabled, the hash table is built and serialized on the
driver, then broadcast to executors. When disabled, each executor builds its
own hash table from the broadcast data.
|
-| spark.gluten.sql.columnar.backend.velox.enableTimestampNtzValidation
| 🔄 Dynamic | false | Enable validation fallback for
TimestampNTZ type. When true, any plan containing TimestampNTZ will fall back
to Spark execution. When false, allows native execution for TimestampNTZ scan.
|
+| spark.gluten.sql.columnar.backend.velox.enableTimestampNtzValidation
| 🔄 Dynamic | false | Enable validation fallback for
TimestampNTZ type. When true, plans with TimestampNTZ in their input or output
schemas fall back to Spark execution. When false, supported TimestampNTZ scans,
aggregates, shuffles and projections are eligible for native execution.
Supported projections include direct columns and struct fields, casts,
hour/minute/second extraction, timestamp addition and null checks. Other
operator, expression and native validation rules still apply.
|
Review Comment:
Regenerated `docs/velox-configuration.md` with the same high-level
description in
[5630a57695](https://github.com/apache/gluten/commit/5630a5769566a60b47b90e2997441a2aa1d0eda5).
##########
backends-velox/src/main/scala/org/apache/gluten/config/VeloxConfig.scala:
##########
@@ -981,9 +981,12 @@ object VeloxConfig extends ConfigRegistry {
val ENABLE_TIMESTAMP_NTZ_VALIDATION =
buildConf("spark.gluten.sql.columnar.backend.velox.enableTimestampNtzValidation")
.doc(
- "Enable validation fallback for TimestampNTZ type. When true, any plan
" +
- "containing TimestampNTZ will fall back to Spark execution. When
false, " +
- "allows native execution for TimestampNTZ scan.")
+ "Enable validation fallback for TimestampNTZ type. When true, plans
with " +
+ "TimestampNTZ in their input or output schemas fall back to Spark
execution. " +
+ "When false, supported TimestampNTZ scans, aggregates, shuffles and
projections " +
+ "are eligible for native execution. Supported projections include
direct columns " +
+ "and struct fields, casts, hour/minute/second extraction, timestamp
addition " +
+ "and null checks. Other operator, expression and native validation
rules still apply.")
Review Comment:
Simplified in
[5630a57695](https://github.com/apache/gluten/commit/5630a5769566a60b47b90e2997441a2aa1d0eda5).
Removed the operator/expression enumeration and kept only the flag's
high-level fallback behavior and the caveat that other validation rules still
apply.
--
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]
