Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .github/workflows/pr_build_linux.yml
Original file line number Diff line number Diff line change
Expand Up @@ -618,6 +618,7 @@ jobs:
org.apache.comet.CometStringDecodeSuite
org.apache.comet.CometWidthBucketSuite
org.apache.comet.CometUuidExpressionSuite
org.apache.comet.serde.CometEvaluationMaskSuite
org.apache.comet.serde.CometScalarFunctionSuite
org.apache.comet.serde.CometLiteralSuite
org.apache.comet.serde.QueryPlanSerdeSuite
Expand Down
1 change: 1 addition & 0 deletions .github/workflows/pr_build_macos.yml
Original file line number Diff line number Diff line change
Expand Up @@ -322,6 +322,7 @@ jobs:
org.apache.comet.CometStringDecodeSuite
org.apache.comet.CometWidthBucketSuite
org.apache.comet.CometUuidExpressionSuite
org.apache.comet.serde.CometEvaluationMaskSuite
org.apache.comet.serde.CometScalarFunctionSuite
org.apache.comet.serde.CometLiteralSuite
org.apache.comet.serde.QueryPlanSerdeSuite
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -253,6 +253,10 @@
- Spark 4.0.1 (audited 2026-08-24): `NullIntolerant` becomes `override def nullIntolerant: Boolean = true`; `inputTypes` widens to `StringTypeWithCollation(supportsTrimCollation = true)`. Behaviour is byte-level and collation-independent, so no divergence for `UTF8_BINARY` and no shim is needed.
- Spark 4.1.1 (audited 2026-08-24): adds `contextIndependentFoldable`; no behavioural change on the decode path.
- Comet native implementation (`spark_unbase64`, `native/spark-expr/src/string_funcs/unbase64.rs`) ports the JDK MIME decoder rules: 256-entry decode LUT, a reused per-batch scratch `Vec<u8>` copied into a preallocated `BinaryBuilder`, all four error messages reproduced verbatim. `CometUnBase64` handles `failOnError = false` natively; `failOnError = true` (reachable from `to_binary('base64')` / `try_to_binary`) requires strict RFC 4648 validation and is not yet implemented natively, so those cases stay on the JVM codegen dispatcher via `CodegenDispatchFallback`.
- Native decoding is limited to column and literal children. Compound children use JVM codegen dispatch when enabled, otherwise they fall back to Spark. Both native decoding and JVM dispatch evaluate batches, so `CometExecRule` also preserves Spark's row pipeline where a limit, preordered top-K or unpartitioned `WindowGroupLimit`, or a first-match semi/anti join condition can skip malformed input. Partitioned window limits drain their groups and do not independently require this fallback, but an outer limit can still protect them. Ordinary decoding projections below a blocking sort or exchange remain eligible unless another limiting consumer intervenes.
- With AQE and an outer limit, a final aggregate that decodes its results and uses incompatible intermediate buffers stays in Spark together with its partial aggregate before stages materialize. This also applies when an intervening sort currently consumes all input, because AQE can remove that sort later. A final aggregate only merging already-decoded inputs does not need this result-evaluation protection.
- `spark.comet.exec.preserveEvaluationMasks.enabled` defaults to `true`. Users who know their Base64 input is valid can set it to `false` to retain native execution across these operator boundaries, including the AQE aggregate protection above. With this opt-out, eager batch evaluation can raise errors on malformed values in rows Spark would skip. Expression support restrictions, including strict decoding and compound-child dispatch, still apply.
- Remaining evaluation-mask gaps, including other throwing expressions and enclosing filter/conditional masks, are tracked in [#6006](https://github.com/apache/datafusion-comet/issues/6006).

## upper

Expand Down
14 changes: 14 additions & 0 deletions docs/source/user-guide/latest/compatibility/index.md
Original file line number Diff line number Diff line change
Expand Up @@ -121,6 +121,20 @@ divergence:
- Spark 4.2 introduced additional ANSI arithmetic overflow behavior differences that Comet does
not yet track ([#4967](https://github.com/apache/datafusion-comet/issues/4967)).

## Errors from rows Spark skips

Batch evaluation can raise a data-dependent error on a row that Spark's row pipeline would
never evaluate. Comet preserves operator-level evaluation masks for `unbase64` under limits,
first-match semi/anti join conditions, and ordered top-K or unpartitioned window limits.
This protection is enabled by default. Setting
`spark.comet.exec.preserveEvaluationMasks.enabled=false` retains native execution for known-valid
input, but can raise errors from malformed Base64 in rows Spark would skip.

This policy currently enrolls only `UnBase64`; it does not establish error-evaluation parity for
ANSI arithmetic, casts, decimal division, or `element_at`. Per-row conditional evaluation
(`AND`, `CASE`) and aggregate `FILTER` require separate checks. The broader audit and remaining
work are tracked in [#6006](https://github.com/apache/datafusion-comet/issues/6006).

## Known result-value divergences

The following native paths silently return values that differ from Spark for edge-case inputs.
Expand Down
2 changes: 1 addition & 1 deletion docs/source/user-guide/latest/expressions.md
Original file line number Diff line number Diff line change
Expand Up @@ -625,7 +625,7 @@ to Spark ([#2837](https://github.com/apache/datafusion-comet/issues/2837)). Enab
| `try_to_binary` | ✅ | — | Rewrites to `try_eval(to_binary(...))`, which routes through the JVM codegen dispatcher |
| `try_to_number` | ✅ | Codegen dispatch | Routed through the JVM codegen dispatcher |
| `ucase` | ✅ | Hybrid | |
| `unbase64` | ✅ | Codegen dispatch | Column or literal input runs natively; other child expressions, and the strict form used by `to_binary(str, 'base64')`, route through the JVM codegen dispatcher |
| `unbase64` | ✅ | Codegen dispatch | Column or literal input runs natively; other child expressions, and the strict form used by `to_binary(str, 'base64')`, route through the JVM codegen dispatcher. Where limits, preordered top-K or unpartitioned window limits, or first-match semi/anti join conditions can skip malformed input, the affected pipeline stays in Spark's row execution. For known-valid input, set `spark.comet.exec.preserveEvaluationMasks.enabled=false` to retain native execution. Disabling this protection can raise errors on malformed values in rows Spark would skip. |
| `upper` | ✅ | Hybrid | |

---
Expand Down
11 changes: 11 additions & 0 deletions spark/src/main/scala/org/apache/comet/CometConf.scala
Original file line number Diff line number Diff line change
Expand Up @@ -215,6 +215,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.")
.booleanConf
.createWithDefault(true)

val COMET_EXEC_PROJECT_ENABLED: ConfigEntry[Boolean] =
createExecEnabledConfig("project", defaultValue = true)
val COMET_EXEC_FILTER_ENABLED: ConfigEntry[Boolean] =
Expand Down
179 changes: 175 additions & 4 deletions spark/src/main/scala/org/apache/comet/rules/CometExecRule.scala
Original file line number Diff line number Diff line change
Expand Up @@ -23,9 +23,10 @@ import scala.collection.mutable.ListBuffer
import scala.jdk.CollectionConverters._

import org.apache.spark.sql.SparkSession
import org.apache.spark.sql.catalyst.expressions.{Divide, DoubleLiteral, EqualNullSafe, EqualTo, Expression, FloatLiteral, GreaterThan, GreaterThanOrEqual, KnownFloatingPointNormalized, LessThan, LessThanOrEqual, NamedExpression, Remainder}
import org.apache.spark.sql.catalyst.expressions.{Divide, DoubleLiteral, EqualNullSafe, EqualTo, Expression, FloatLiteral, GreaterThan, GreaterThanOrEqual, KnownFloatingPointNormalized, LessThan, LessThanOrEqual, NamedExpression, Remainder, SortOrder}
import org.apache.spark.sql.catalyst.expressions.aggregate.{AggregateMode, Final, Partial, PartialMerge}
import org.apache.spark.sql.catalyst.optimizer.NormalizeNaNAndZero
import org.apache.spark.sql.catalyst.plans.{JoinType, LeftAnti, LeftSemi}
import org.apache.spark.sql.catalyst.rules.Rule
import org.apache.spark.sql.catalyst.trees.TreeNodeTag
import org.apache.spark.sql.catalyst.util.sideBySide
Expand All @@ -48,7 +49,7 @@ import org.apache.spark.sql.execution.datasources.v2.csv.CSVScan
import org.apache.spark.sql.execution.datasources.v2.json.JsonScan
import org.apache.spark.sql.execution.datasources.v2.parquet.ParquetScan
import org.apache.spark.sql.execution.exchange.{BroadcastExchangeExec, BroadcastExchangeLike, ReusedExchangeExec, ShuffleExchangeExec, ShuffleExchangeLike}
import org.apache.spark.sql.execution.joins.{BroadcastHashJoinExec, BroadcastNestedLoopJoinExec, ShuffledHashJoinExec, SortMergeJoinExec}
import org.apache.spark.sql.execution.joins.{BroadcastHashJoinExec, BroadcastNestedLoopJoinExec, HashJoin, ShuffledHashJoinExec, SortMergeJoinExec}
import org.apache.spark.sql.execution.window.WindowExec
import org.apache.spark.sql.internal.SQLConf
import org.apache.spark.sql.types._
Expand Down Expand Up @@ -90,6 +91,9 @@ object CometExecRule {
val COMET_UNSAFE_PARTIAL: TreeNodeTag[String] =
TreeNodeTag[String]("comet.unsafePartialAgg")

private[rules] val UNSAFE_EXPRESSION_EVALUATION: TreeNodeTag[String] =
TreeNodeTag[String]("comet.unsafeExpressionEvaluation")

/**
* Info message for a native operator that keeps a `Scan` over an input that now reads its
* shuffle directly, because its native plan's leaves do not line up with its inputs.
Expand Down Expand Up @@ -370,6 +374,9 @@ case class CometExecRule(session: SparkSession)
// spotless:on
private def transform(plan: SparkPlan): SparkPlan = {
def convertNode(op: SparkPlan): SparkPlan = op match {
case op if op.getTagValue(CometExecRule.UNSAFE_EXPRESSION_EVALUATION).isDefined =>
withFallbackReason(op, op.getTagValue(CometExecRule.UNSAFE_EXPRESSION_EVALUATION).get)

// Scan marker produced by an optional, out-of-tree scan contrib (e.g. contrib/delta).
// Matched by trait (no compile-time dependency on the contrib) and present only when that
// contrib is on the classpath. The marker carries its own serde handler and typically wraps
Expand Down Expand Up @@ -931,9 +938,10 @@ case class CometExecRule(session: SparkSession)
// corresponding Final or PartialMerge cannot be converted and the intermediate buffer
// formats are incompatible. This runs before transform() so the tags are checked
// during the bottom-up conversion. Tags persist through AQE stage creation.
tagUnsafePartialAggregates(planWithJoinRewritten)
val planWithEvaluationMasks = preserveEvaluationMasks(planWithJoinRewritten)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment block above describes tagUnsafePartialAggregates, but it now sits above val planWithEvaluationMasks = preserveEvaluationMasks(...). Could it move below that line, with a short comment on the new pass saying why it must run first (it sets the early Final tag that tagUnsafePartialAggregates and canAggregateBeConverted consume)?

tagUnsafePartialAggregates(planWithEvaluationMasks)

var newPlan = revertUnsafePartialAggregates(transform(planWithJoinRewritten))
var newPlan = revertUnsafePartialAggregates(transform(planWithEvaluationMasks))

// if the plan cannot be run fully natively then explain why (when appropriate
// config is enabled)
Expand Down Expand Up @@ -1046,6 +1054,167 @@ case class CometExecRule(session: SparkSession)
}
}

/** Keep opted-in expressions in Spark's row pipeline where an operator can skip inputs. */
private def preserveEvaluationMasks(plan: SparkPlan): SparkPlan = {
if (!CometConf.COMET_EXEC_PRESERVE_EVALUATION_MASKS_ENABLED.get()) return plan

def findEvaluationMaskName(expr: Expression): Option[String] = {
var name: Option[String] = None
expr.exists { child =>
name = QueryPlanSerde.evaluationMaskName(child)
name.isDefined
}
Comment on lines +1061 to +1066

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

findEvaluationMaskName descends into every child, including under TryEval. By my reading try_to_binary(x, 'base64') is TryEval(ToBinary(...)) and holds UnBase64(failOnError = true). TryEval catches the failure and returns NULL, so evaluating a row that Spark would skip cannot raise there, yet SELECT try_to_binary(s, 'base64') FROM t LIMIT 10 should now be forced to Spark. Could the traversal stop at TryEval? I read TryEval and TryToBinary in Spark 4.1.3 and did not run the query. I could not find a test in the suite or the SQL fixture for try_to_binary.

name
}

def originalPlan(node: SparkPlan): SparkPlan = node match {
case scan: CometScanExec =>
scan.wrapped
.copy(partitionFilters = scan.partitionFilters, dataFilters = scan.dataFilters)
// The local selector saves its parent's original plan, but does not evaluate that
// parent's projection or offset. Only the enclosing top-K owns those operations.
case local: CometLocalTopKExec => local
case comet: CometExec => comet.originalPlan
Comment thread
sunchao marked this conversation as resolved.
case shuffle: CometShuffleExchangeExec => shuffle.originalPlan
case broadcast: CometBroadcastExchangeExec => broadcast.originalPlan
case _ => node
}

def evaluatedExpressions(node: SparkPlan): Seq[Expression] = node match {
case local: CometLocalTopKExec => local.sortOrder
// V1 dataFilters include predicates Spark cannot push into Parquet. Without native row
// filtering they only inform metadata pruning; the Spark Filter above evaluates rows.
case _: FileSourceScanExec if !CometConf.COMET_PARQUET_ROW_FILTER_PUSHDOWN_ENABLED.get() =>
Seq.empty
case _ => node.expressions
}

// Most plans contain no opted-in expression. Include native originals and sticky tags
// so repeated AQE passes still restore an already-protected subtree when necessary.
if (!plan.exists(node =>
node.getTagValue(CometExecRule.UNSAFE_EXPRESSION_EVALUATION).isDefined ||
evaluatedExpressions(originalPlan(node)).exists(findEvaluationMaskName(_).isDefined))) {
return plan
}

def firstMatch(joinType: JoinType): Boolean = joinType match {
case LeftSemi | LeftAnti => true
case _ => false
}

def protect(
node: SparkPlan,
belowLimit: Boolean,
hasLimitAncestor: Boolean): (SparkPlan, Option[String]) = {
val original = originalPlan(node)
val startsLimit = original match {
// Offset-only collection does not stop its input early.
case collect: CollectLimitExec => collect.limit >= 0
case _: LocalLimitExec | _: GlobalLimitExec => true

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The line above says offset-only collection does not stop its input early, but GlobalLimitExec is counted as an early stop here without the same check. In Spark 3.5.9 a non-root OFFSET n plans as GlobalLimitExec with the default limit = -1 and only does iter.drop(offset), and the generated code adds the count check only when limit >= 0. As written, an offset-only subquery over unbase64 falls back, and it also sets hasLimitAncestor. Would case g: GlobalLimitExec => g.limit >= 0 match the CollectLimitExec case? This is read from SparkStrategies and limit.scala, not run.

case topK: TakeOrderedAndProjectExec =>
SortOrder.orderingSatisfies(node.children.head.outputOrdering, topK.sortOrder)
case windowLimit
if ShimCometWindowGroupLimit.windowGroupLimitClass.exists(
_.isInstance(windowLimit)) =>
// Partitioned limits drain each group, but an outer LIMIT can still stop them.
// Keep the conservative behavior if a future rank function cannot be extracted.
ShimCometWindowGroupLimit.extract(windowLimit).forall(_.partitionSpec.isEmpty) &&
SortOrder.orderingSatisfies(
node.children.head.outputOrdering,
windowLimit.requiredChildOrdering.head)
Comment thread
sunchao marked this conversation as resolved.
case _ => false
}
// These operators consume their input before yielding rows. Still visit their children:
// an inner LocalLimit below an exchange must establish its own evaluation boundary.
val materializesInput = original match {
case _: SortExec | _: CometLocalTopKExec | _: HashAggregateExec |
_: ObjectHashAggregateExec | _: ShuffleExchangeLike | _: BroadcastExchangeLike |
_: QueryStageExec | _: ReusedExchangeExec =>
true
case _ => false
}
// A top-K is still a LIMIT even when its current input requires sorting.
val limitAncestor = hasLimitAncestor || startsLimit ||
original.isInstanceOf[TakeOrderedAndProjectExec]
val protectedChildren = node.children.map { child =>
protect(child, startsLimit || (belowLimit && !materializesInput), limitAncestor)
}
val childReason = protectedChildren.flatMap(_._2).headOption
val condition = original match {
case join: HashJoin if firstMatch(join.joinType) => join.condition
case join: SortMergeJoinExec if firstMatch(join.joinType) => join.condition
case join: BroadcastNestedLoopJoinExec if firstMatch(join.joinType) => join.condition
case _ => None
}
val finalAggregate = original match {
case agg: BaseAggregateExec
if (agg.isInstanceOf[HashAggregateExec] ||
agg.isInstanceOf[ObjectHashAggregateExec]) &&
agg.aggregateExpressions.map(_.mode).distinct == Seq(Final) =>
Some(agg)
case _ => None
}
val limitName = if (belowLimit) {
// Final merges buffers; it does not reevaluate the aggregate's original inputs.
val expressions =
finalAggregate.map(_.resultExpressions).getOrElse(evaluatedExpressions(original))
expressions.iterator.flatMap(findEvaluationMaskName).take(1).toSeq.headOption
} else {
None
}
// AQE can remove an intervening sort after a native Partial has materialized. Choose
// compatible buffers before that happens, even if a current operator drains its input.
val aggregateBufferName = if (hasLimitAncestor && conf.adaptiveExecutionEnabled) {
finalAggregate
.filterNot(agg =>
QueryPlanSerde.allAggsSupportNativePartialToSparkFinal(agg.aggregateExpressions))
.flatMap(
_.resultExpressions.iterator.flatMap(findEvaluationMaskName).take(1).toSeq.headOption)
} else {
None
}
val ownReason = limitName
.map(name => s"$name requires Spark evaluation below LIMIT")
.orElse(aggregateBufferName.map(name =>
s"$name requires Spark aggregate buffers below LIMIT with AQE"))
.orElse(
condition
.flatMap(findEvaluationMaskName)
.map(name => s"$name requires Spark evaluation in first-match join conditions"))
.orElse(node.getTagValue(CometExecRule.UNSAFE_EXPRESSION_EVALUATION))
// Exchanges can restart native execution after consuming a Spark row pipeline.
val restartsNative = original.isInstanceOf[ShuffleExchangeLike] ||
original.isInstanceOf[BroadcastExchangeLike]
val reason = ownReason.orElse(if (restartsNative) None else childReason)
// Early tags keep fresh aggregate buffers safe before AQE materialization. Reused native
// buffer chains are restored by revertUnsafePartialAggregates after conversion.
val children = protectedChildren.map(_._1)
val prepared = node match {
// Do not refill a batch between the row decoder and its short-circuiting consumer.
case _: RowToColumnarExec | _: CometSparkToColumnarExec if childReason.isDefined =>
children.head
case _: ColumnarToRowExec | _: CometColumnarToRowExec | _: CometNativeColumnarToRowExec
if childReason.isDefined && !children.head.supportsColumnar =>
children.head
// This inserted selector has no standalone Spark original. Its parent restores
// the full top-K when a child changes, applying the projection and offset once.
case _: CometLocalTopKExec if reason.isDefined || children != node.children =>
children.head
case _ if (original ne node) && (reason.isDefined || children != node.children) =>
// AQE can reuse an existing native subtree. Rebuild affected ancestors as well so
// their serialized native plans do not retain the decoder that just fell back.
val restored = original.withNewChildren(children)
node.getTagValue(SparkPlan.LOGICAL_PLAN_TAG).foreach(restored.setLogicalLink)
restored
case _ => node.withNewChildren(children)
}
reason.foreach(prepared.setTagValue(CometExecRule.UNSAFE_EXPRESSION_EVALUATION, _))
(prepared, reason)
}

protect(plan, belowLimit = false, hasLimitAncestor = false)._1
}

/** Convert a Spark plan to a Comet plan using the specified serde handler */
private def convertToComet(op: SparkPlan, handler: CometOperatorSerde[_]): Option[SparkPlan] = {
val converted = tryConvertToComet(op, handler)
Expand Down Expand Up @@ -1502,6 +1671,8 @@ case class CometExecRule(session: SparkSession)
private def canAggregateBeConverted(
agg: BaseAggregateExec,
expectedMode: AggregateMode): Boolean = {
if (agg.getTagValue(CometExecRule.UNSAFE_EXPRESSION_EVALUATION).isDefined) return false

val handler = allExecs.get(agg.getClass)
if (handler.isEmpty) return false
val serde = handler.get.asInstanceOf[CometOperatorSerde[SparkPlan]]
Expand Down
Loading
Loading