You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "andygrove (via GitHub)" <gi...@apache.org> on 2026/05/27 23:23:29 UTC
[PR] chore(audit): audit hash expressions across Spark 3.4.3, 3.5.8, 4.0.1, 4.1.1 [datafusion-comet]
andygrove opened a new pull request, #4476:
URL: https://github.com/apache/datafusion-comet/pull/4476
## Which issue does this PR close?
Closes #.
## Rationale for this change
Continuation of the per-category expression audit. Same pattern as #4475 (conditional), #4474 (misc), #4473 (collection), #4470 (json), #4469 (struct), using the updated `audit-comet-expression` skill in #4468.
## What changes are included in this PR?
### Support-doc audit notes
Add per-version audit sub-bullets to `crc32`, `hash`, `md5`, `sha`, `sha1`, `sha2`, and `xxhash64`. `sha` is a registry alias of `Sha1`. Spark 4.0 only adds the `DefaultStringProducingExpression` trait and the `nullIntolerant: Boolean` field refactor on the four `String`-producing expressions (`Md5`, `Sha1`, `Sha2`, `Crc32`); no runtime behaviour change across the category.
### Support-level consistency fixes (in `hash.scala`)
- Refactor `HashUtils` to return reasons (`unsupportedReasonFor`, `supportLevelForChildren`, `unsupportedReasons`) instead of calling `withInfo` from inside the helper. The recursive type check no longer side-effects on the expression tree at type-check time, which the audit skill calls out as the canonical antipattern.
- `CometXxHash64`, `CometMurmur3Hash`, `CometSha1`, `CometSha2`: override `getSupportLevel` and `getUnsupportedReasons` so the unsupported-child-type and (for `Sha2`) the non-foldable-`numBits` restrictions reach both the dispatcher's EXPLAIN message and the compatibility doc generator.
### Tracking issues filed for follow-up
None. The `TimeType` gap (Spark 4.0+) is covered by the existing #4418 EPIC; the `DecimalType`-precision-18 gap is a documented semantic difference (Spark hashes via Java `BigDecimal`), already declared by the new `HashUtils.unsupportedReasons`.
### Audit process
Audited directly using the `audit-comet-expression` skill (4 Spark versions per #4468). Four serde objects plus the shared `HashUtils` helper.
## How are these changes tested?
- `./mvnw test -Dsuites="org.apache.comet.CometHashExpressionSuite" -Dtest=none` (37 tests pass)
- `make core` succeeds with the serde refactor.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] chore(audit): audit hash expressions across Spark 3.4.3, 3.5.8, 4.0.1, 4.1.1 [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on PR #4476:
URL: https://github.com/apache/datafusion-comet/pull/4476#issuecomment-4595479840
Merged. Thanks @kazuyukitanimura
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] chore(audit): audit hash expressions across Spark 3.4.3, 3.5.8, 4.0.1, 4.1.1 [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on code in PR #4476:
URL: https://github.com/apache/datafusion-comet/pull/4476#discussion_r3329417790
##########
spark/src/main/scala/org/apache/comet/serde/hash.scala:
##########
@@ -68,72 +73,76 @@ object CometMurmur3Hash extends CometExpressionSerde[Murmur3Hash] {
}
object CometSha2 extends CometExpressionSerde[Sha2] {
- override def convert(
- expr: Sha2,
- inputs: Seq[Attribute],
- binding: Boolean): Option[ExprOuterClass.Expr] = {
- if (!HashUtils.isSupportedType(expr)) {
- return None
- }
- // It's possible for spark to dynamically compute the number of bits from input
- // expression, however DataFusion does not support that yet.
+ private val nonFoldableNumBitsReason =
+ "The `numBits` argument must be a foldable literal value"
+
+ override def getUnsupportedReasons(): Seq[String] =
+ HashUtils.unsupportedReasons :+ nonFoldableNumBitsReason
+
+ override def getSupportLevel(expr: Sha2): SupportLevel = {
if (!expr.right.foldable) {
- withInfo(expr, "For Sha2, non literal numBits is not supported")
- return None
+ Unsupported(Some(nonFoldableNumBitsReason))
+ } else {
+ HashUtils.supportLevelForChildren(expr)
}
+ }
+ override def convert(
+ expr: Sha2,
+ inputs: Seq[Attribute],
+ binding: Boolean): Option[ExprOuterClass.Expr] = {
val leftExpr = exprToProtoInternal(expr.left, inputs, binding)
val numBitsExpr = exprToProtoInternal(expr.right, inputs, binding)
scalarFunctionExprToProtoWithReturnType("sha2", StringType, false, leftExpr, numBitsExpr)
}
}
object CometSha1 extends CometExpressionSerde[Sha1] {
+
+ override def getUnsupportedReasons(): Seq[String] = HashUtils.unsupportedReasons
+
+ override def getSupportLevel(expr: Sha1): SupportLevel =
+ HashUtils.supportLevelForChildren(expr)
+
override def convert(
expr: Sha1,
inputs: Seq[Attribute],
binding: Boolean): Option[ExprOuterClass.Expr] = {
- if (!HashUtils.isSupportedType(expr)) {
- withInfo(expr, s"HashUtils doesn't support dataType: ${expr.child.dataType}")
- return None
- }
val childExpr = exprToProtoInternal(expr.child, inputs, binding)
scalarFunctionExprToProtoWithReturnType("sha1", StringType, false, childExpr)
}
}
private object HashUtils {
- def isSupportedType(expr: Expression): Boolean = {
- for (child <- expr.children) {
- if (!isSupportedDataType(expr, child.dataType)) {
- return false
- }
+
+ private val unsupportedDecimalReason =
+ "`DecimalType` with precision > 18 is not supported (Spark hashes via Java `BigDecimal`)"
+ private val unsupportedTimeTypeReason = "`TimeType` is not supported"
+
+ val unsupportedReasons: Seq[String] =
+ Seq(unsupportedDecimalReason, unsupportedTimeTypeReason, "Unsupported child data type")
+
+ def supportLevelForChildren(expr: Expression): SupportLevel = {
+ expr.children.iterator
+ .flatMap(c => unsupportedReasonFor(c.dataType).iterator)
+ .toSeq
+ .headOption match {
+ case Some(reason) => Unsupported(Some(reason))
+ case None => Compatible()
}
- true
}
- private def isSupportedDataType(expr: Expression, dt: DataType): Boolean = {
- dt match {
- case d: DecimalType if d.precision > 18 =>
- // Spark converts decimals with precision > 18 into
- // Java BigDecimal before hashing
- withInfo(expr, s"Unsupported datatype: $dt (precision > 18)")
- false
- case s: StructType =>
- s.fields.forall(f => isSupportedDataType(expr, f.dataType))
- case a: ArrayType =>
- isSupportedDataType(expr, a.elementType)
- case m: MapType =>
- isSupportedDataType(expr, m.keyType) && isSupportedDataType(expr, m.valueType)
- case dt if isTimeType(dt) =>
- withInfo(expr, s"Unsupported datatype $dt")
- false
- case _ if !supportedDataType(dt, allowComplex = true) =>
- withInfo(expr, s"Unsupported datatype $dt")
- false
- case _ =>
- true
- }
+ private def unsupportedReasonFor(dt: DataType): Option[String] = dt match {
+ case d: DecimalType if d.precision > 18 => Some(unsupportedDecimalReason)
+ case s: StructType =>
+ s.fields.iterator.flatMap(f => unsupportedReasonFor(f.dataType).iterator).toSeq.headOption
Review Comment:
This will return the first `Some` item, or `None` if they are all `None`
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] chore(audit): audit hash expressions across Spark 3.4.3, 3.5.8, 4.0.1, 4.1.1 [datafusion-comet]
Posted by "kazuyukitanimura (via GitHub)" <gi...@apache.org>.
kazuyukitanimura commented on code in PR #4476:
URL: https://github.com/apache/datafusion-comet/pull/4476#discussion_r3327100269
##########
spark/src/main/scala/org/apache/comet/serde/hash.scala:
##########
@@ -68,72 +73,76 @@ object CometMurmur3Hash extends CometExpressionSerde[Murmur3Hash] {
}
object CometSha2 extends CometExpressionSerde[Sha2] {
- override def convert(
- expr: Sha2,
- inputs: Seq[Attribute],
- binding: Boolean): Option[ExprOuterClass.Expr] = {
- if (!HashUtils.isSupportedType(expr)) {
- return None
- }
- // It's possible for spark to dynamically compute the number of bits from input
- // expression, however DataFusion does not support that yet.
+ private val nonFoldableNumBitsReason =
+ "The `numBits` argument must be a foldable literal value"
+
+ override def getUnsupportedReasons(): Seq[String] =
+ HashUtils.unsupportedReasons :+ nonFoldableNumBitsReason
+
+ override def getSupportLevel(expr: Sha2): SupportLevel = {
if (!expr.right.foldable) {
- withInfo(expr, "For Sha2, non literal numBits is not supported")
- return None
+ Unsupported(Some(nonFoldableNumBitsReason))
+ } else {
+ HashUtils.supportLevelForChildren(expr)
}
+ }
+ override def convert(
+ expr: Sha2,
+ inputs: Seq[Attribute],
+ binding: Boolean): Option[ExprOuterClass.Expr] = {
val leftExpr = exprToProtoInternal(expr.left, inputs, binding)
val numBitsExpr = exprToProtoInternal(expr.right, inputs, binding)
scalarFunctionExprToProtoWithReturnType("sha2", StringType, false, leftExpr, numBitsExpr)
}
}
object CometSha1 extends CometExpressionSerde[Sha1] {
+
+ override def getUnsupportedReasons(): Seq[String] = HashUtils.unsupportedReasons
+
+ override def getSupportLevel(expr: Sha1): SupportLevel =
+ HashUtils.supportLevelForChildren(expr)
+
override def convert(
expr: Sha1,
inputs: Seq[Attribute],
binding: Boolean): Option[ExprOuterClass.Expr] = {
- if (!HashUtils.isSupportedType(expr)) {
- withInfo(expr, s"HashUtils doesn't support dataType: ${expr.child.dataType}")
- return None
- }
val childExpr = exprToProtoInternal(expr.child, inputs, binding)
scalarFunctionExprToProtoWithReturnType("sha1", StringType, false, childExpr)
}
}
private object HashUtils {
- def isSupportedType(expr: Expression): Boolean = {
- for (child <- expr.children) {
- if (!isSupportedDataType(expr, child.dataType)) {
- return false
- }
+
+ private val unsupportedDecimalReason =
+ "`DecimalType` with precision > 18 is not supported (Spark hashes via Java `BigDecimal`)"
+ private val unsupportedTimeTypeReason = "`TimeType` is not supported"
+
+ val unsupportedReasons: Seq[String] =
+ Seq(unsupportedDecimalReason, unsupportedTimeTypeReason, "Unsupported child data type")
+
+ def supportLevelForChildren(expr: Expression): SupportLevel = {
+ expr.children.iterator
+ .flatMap(c => unsupportedReasonFor(c.dataType).iterator)
+ .toSeq
+ .headOption match {
+ case Some(reason) => Unsupported(Some(reason))
+ case None => Compatible()
}
- true
}
- private def isSupportedDataType(expr: Expression, dt: DataType): Boolean = {
- dt match {
- case d: DecimalType if d.precision > 18 =>
- // Spark converts decimals with precision > 18 into
- // Java BigDecimal before hashing
- withInfo(expr, s"Unsupported datatype: $dt (precision > 18)")
- false
- case s: StructType =>
- s.fields.forall(f => isSupportedDataType(expr, f.dataType))
- case a: ArrayType =>
- isSupportedDataType(expr, a.elementType)
- case m: MapType =>
- isSupportedDataType(expr, m.keyType) && isSupportedDataType(expr, m.valueType)
- case dt if isTimeType(dt) =>
- withInfo(expr, s"Unsupported datatype $dt")
- false
- case _ if !supportedDataType(dt, allowComplex = true) =>
- withInfo(expr, s"Unsupported datatype $dt")
- false
- case _ =>
- true
- }
+ private def unsupportedReasonFor(dt: DataType): Option[String] = dt match {
+ case d: DecimalType if d.precision > 18 => Some(unsupportedDecimalReason)
+ case s: StructType =>
+ s.fields.iterator.flatMap(f => unsupportedReasonFor(f.dataType).iterator).toSeq.headOption
Review Comment:
If `unsupportedReasonFor(f.dataType).iterator` returns `None` for the first element of `fields`, will `s.fields.iterator.flatMap(f => unsupportedReasonFor(f.dataType).iterator).toSeq.headOption` return `None`?
Do we need to make sure all `fields` return `None`?
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] chore(audit): audit hash expressions across Spark 3.4.3, 3.5.8, 4.0.1, 4.1.1 [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove merged PR #4476:
URL: https://github.com/apache/datafusion-comet/pull/4476
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] chore(audit): audit hash expressions across Spark 3.4.3, 3.5.8, 4.0.1, 4.1.1 [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on PR #4476:
URL: https://github.com/apache/datafusion-comet/pull/4476#issuecomment-4569004838
No deferred follow-up work from this audit. The two known limitations (TimeType for Spark 4.0+, DecimalType precision > 18) are already declared via `HashUtils.unsupportedReasons` and flip the support level to `Unsupported` per skill rule 12; TimeType is tracked by the existing #4288 EPIC. The Sha2 non-foldable `numBits` restriction is now modeled in `getSupportLevel` rather than `convert`-time `withInfo`, satisfying skill rule 10.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org