comphead commented on code in PR #5972:
URL: https://github.com/apache/datafusion-comet/pull/5972#discussion_r4277251176
##########
spark/src/main/scala/org/apache/comet/serde/QueryPlanSerde.scala:
##########
@@ -1108,6 +1201,77 @@ object QueryPlanSerde extends Logging with CometExprShim
with CometTypeShim {
}
}
+ /** Spark 4.2 changed this setting from String to an Enumeration.Value. */
+ private[comet] def usesInterpretedProjection(conf: SQLConf): Boolean =
+ conf.getConf(SQLConf.CODEGEN_FACTORY_MODE).toString == "NO_CODEGEN"
Review Comment:
`NO_CODEGEN` only disables whole-stage codegen from SPARK-44236, which is in
3.5.9 and 4.x. I could not find `NO_CODEGEN` anywhere in `sql/core` main of
v3.4.3, while v3.5.9 has it in `WholeStageCodegenExec`. So on 3.4.x this
returns true while Spark still fuses and defers the Project, and the protection
is skipped. `operators.scala` already gates the same check with
`isSpark35Plus`, and `CometCacheColumnarRule.scala` line 67 has the same string
compare. Could this helper be version-aware and `CometCacheColumnarRule` go
through it? The comment above it only covers the String versus
`Enumeration.Value` difference.
##########
spark/src/main/scala/org/apache/comet/serde/QueryPlanSerde.scala:
##########
@@ -818,6 +837,34 @@ object QueryPlanSerde extends Logging with CometExprShim
with CometTypeShim {
}
val fn = aggExpr.aggregateFunction
+ if (aggExpr.mode == Partial || aggExpr.mode == Complete) {
+ // Raw-input aggregates can skip arguments (FILTER, null-skipping, or
state-dependent
+ // updates). Final/PartialMerge only read buffers, so their original
children are irrelevant.
+ var pending = fn.children.toList
+ while (pending.nonEmpty) {
+ val child = pending.head
+ pending = pending.tail
+ child match {
+ case nextDay: NextDay
+ if CometNextDay.evaluationMaskName(nextDay).isDefined &&
+ (aggExpr.filter.isDefined || fn.isInstanceOf[First] ||
fn.isInstanceOf[Last]) =>
Review Comment:
This guard covers `FILTER`, `First` and `Last`, but other aggregates that
Comet serializes natively also skip an argument in Spark.
`MaxByAndMinBy.updateExpressions` evaluates `valueExpr` only in some `CaseWhen`
branches (identical in 3.4.3, 3.5.9 and 4.x). `Count` and `Covariance` skip the
later argument after a null earlier one. `CometMaxMinBy` passes every argument
to `exprToProto` and the native accumulators consume whole-batch arrays. A
repro I have not run: ANSI on, one Parquet file with `(DATE '2024-01-01',
'Monday', 2)` and `(DATE '2024-01-01', 'NOT_A_DAY', 1)`, then `SELECT
max_by(next_day(d, dow), k) FROM t`. In Spark the second row keeps the old
value because 2 > 1, so `next_day` is never evaluated for it. Could the guard
cover `MaxBy` and `MinBy` and the multi-argument `Count` and `Cov*` family, or
be an allowlist of aggregates that evaluate every argument for every row? A
fixture next to `max(next_day(d, dow))` in `next_day_safe_evaluation.sql` would
pin it.
##########
spark/src/main/scala/org/apache/comet/CometConf.scala:
##########
@@ -260,6 +260,17 @@ object CometConf extends ShimCometConf {
.booleanConf
.createWithDefault(true)
+ val COMET_EXEC_PRESERVE_EVALUATION_MASKS_ENABLED: ConfigEntry[Boolean] =
+ conf(s"$COMET_EXEC_CONFIG_PREFIX.preserveEvaluationMasks.enabled")
+ .category(CATEGORY_EXEC)
+ .doc(
+ "Preserve Spark's row evaluation when operators such as LIMIT or
semi/anti joins " +
+ "can skip inputs to throwing expressions. Currently this applies to
unbase64. " +
+ "Set to false to retain native execution when the input is known to
be valid. " +
+ "When disabled, Comet may fail on malformed Base64 in rows Spark
would skip.")
Review Comment:
This text still says "Currently this applies to unbase64" and that Comet may
fail on malformed Base64. The flag now also governs `next_day` and
`levenshtein`. The `next_day` and `levenshtein` rows in `expressions.md` still
say non-collated input runs natively, which no longer holds for ANSI dynamic
weekdays under a limit or deferred projection, or for a nullable three-argument
threshold. `CometLevenshtein.getUnsupportedReasons` also lists only collation,
so the nullable-threshold case is missing from the generated reasons docs.
##########
spark/src/test/resources/sql-tests/expressions/string/levenshtein_nullable_threshold.sql:
##########
@@ -0,0 +1,34 @@
+-- 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.
+
+-- MinSparkVersion: 3.5
+-- ConfigMatrix: spark.sql.codegen.factoryMode=FALLBACK,CODEGEN_ONLY,NO_CODEGEN
Review Comment:
The nullable-threshold guard is a plan-time decision, and as far as I can
tell it never reads `spark.sql.codegen.factoryMode`
(`usesInterpretedProjection` is only used by the `next_day` deferral analysis).
For the `expect_fallback` query both sides run Spark, so the three-value
`ConfigMatrix` repeats one check three times. The same fallback is also
asserted by `levenshtein_threshold.sql` (lines 49 and 59) and
`levenshtein_collation_interpreted.sql`. Could the matrix go, and the
collation-specific queries move into `levenshtein_interpreted_arguments.sql`?
--
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]
