uros-b commented on code in PR #59073:
URL: https://github.com/apache/spark/pull/59073#discussion_r4126861717
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/variant/variantExpressions.scala:
##########
@@ -391,6 +391,71 @@ object VariantPathParser extends RegexParsers {
}
}
+sealed trait VariantPathArg
+
+case class FoldableVariantPath(segments: Array[VariantPathSegment], pathStr:
String)
+ extends VariantPathArg {
+ // Java segments are not Serializable. Rebuild this cache once per executor
task.
+ @transient lazy val javaSegments: Array[VariantBuilder.PathSegment] =
+ VariantExpressionEvalUtils.toJavaSegments(segments)
+}
+
+case class DynamicVariantPath(expr: Expression) extends VariantPathArg
+
+object VariantPathArg {
+ def fromExpression(
+ child: Expression,
+ functionName: String,
+ allowRoot: Boolean = false): Option[VariantPathArg] = {
+ if (child.foldable) {
+ val value = child.eval()
+ if (value == null) {
+ None
+ } else {
+ val pathStr = value.asInstanceOf[UTF8String].toString
+ Some(FoldableVariantPath(
+ VariantExpressionEvalUtils.parseVariantPath(pathStr, functionName,
allowRoot), pathStr))
+ }
+ } else {
+ Some(DynamicVariantPath(child))
+ }
+ }
+}
+
+object VariantManipulationExpressionUtils extends QueryErrorsBase {
+ // Callers cache this result and force parsing before their NULL
short-circuit.
+ // A dynamic path or constant NULL has no cached parsed path.
+ def foldablePath(
Review Comment:
Nit: `fromExpression` wraps a non-foldable path in `DynamicVariantPath`,
then `collect` throws that away. Fine for a lazy val, but a foldable-only
helper would match how insert/set/append actually use this (`None` means
dynamic or constant NULL).
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/variant/variantExpressions.scala:
##########
@@ -391,6 +391,71 @@ object VariantPathParser extends RegexParsers {
}
}
+sealed trait VariantPathArg
+
+case class FoldableVariantPath(segments: Array[VariantPathSegment], pathStr:
String)
+ extends VariantPathArg {
+ // Java segments are not Serializable. Rebuild this cache once per executor
task.
+ @transient lazy val javaSegments: Array[VariantBuilder.PathSegment] =
+ VariantExpressionEvalUtils.toJavaSegments(segments)
+}
+
+case class DynamicVariantPath(expr: Expression) extends VariantPathArg
+
+object VariantPathArg {
+ def fromExpression(
+ child: Expression,
+ functionName: String,
+ allowRoot: Boolean = false): Option[VariantPathArg] = {
+ if (child.foldable) {
+ val value = child.eval()
+ if (value == null) {
+ None
+ } else {
+ val pathStr = value.asInstanceOf[UTF8String].toString
+ Some(FoldableVariantPath(
+ VariantExpressionEvalUtils.parseVariantPath(pathStr, functionName,
allowRoot), pathStr))
+ }
+ } else {
+ Some(DynamicVariantPath(child))
+ }
+ }
+}
+
+object VariantManipulationExpressionUtils extends QueryErrorsBase {
+ // Callers cache this result and force parsing before their NULL
short-circuit.
+ // A dynamic path or constant NULL has no cached parsed path.
+ def foldablePath(
+ path: Expression,
+ functionName: String,
+ allowRoot: Boolean = false): Option[FoldableVariantPath] =
+ VariantPathArg.fromExpression(path, functionName, allowRoot).collect {
+ case parsed: FoldableVariantPath => parsed
+ }
+
+ def checkValueType(dataType: DataType): TypeCheckResult = {
+ if (dataType == NullType || VariantGet.checkDataType(dataType,
allowStructsAndMaps = false)) {
+ TypeCheckResult.TypeCheckSuccess
+ } else {
+ DataTypeMismatch(
+ errorSubClass = "CAST_WITHOUT_SUGGESTION",
+ messageParameters =
+ Map("srcType" -> toSQLType(dataType), "targetType" ->
toSQLType(VariantType)))
+ }
+ }
+
+ def assignResult(ev: ExprCode, call: String, failOnError: Boolean): String =
{
Review Comment:
This is equivalent for these expressions today because `nullable =
!failOnError || children.exists(_.nullable)`, so `failOnError = false` always
declares `ev.isNull`.
`nullSafeCodeGen` only makes `ev.isNull` a local when `nullable` is true;
otherwise it is `FalseLiteral`. `VariantFromEntries.doGenCode` in this file
already documents that and keys the assignment off `nullable`.
Could `assignResult` take `nullable` (or `markNull`) instead of
`failOnError`? Reusing this helper with `failOnError = false` on a non-nullable
expression would generate invalid code. Direct assignment is fine for
`VariantType`; this should not be used for primitives.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/variant/variantExpressions.scala:
##########
@@ -391,6 +391,71 @@ object VariantPathParser extends RegexParsers {
}
}
+sealed trait VariantPathArg
+
+case class FoldableVariantPath(segments: Array[VariantPathSegment], pathStr:
String)
+ extends VariantPathArg {
+ // Java segments are not Serializable. Rebuild this cache once per executor
task.
+ @transient lazy val javaSegments: Array[VariantBuilder.PathSegment] =
+ VariantExpressionEvalUtils.toJavaSegments(segments)
+}
+
+case class DynamicVariantPath(expr: Expression) extends VariantPathArg
+
+object VariantPathArg {
+ def fromExpression(
+ child: Expression,
+ functionName: String,
+ allowRoot: Boolean = false): Option[VariantPathArg] = {
+ if (child.foldable) {
+ val value = child.eval()
+ if (value == null) {
+ None
+ } else {
+ val pathStr = value.asInstanceOf[UTF8String].toString
+ Some(FoldableVariantPath(
+ VariantExpressionEvalUtils.parseVariantPath(pathStr, functionName,
allowRoot), pathStr))
+ }
+ } else {
+ Some(DynamicVariantPath(child))
+ }
+ }
+}
+
+object VariantManipulationExpressionUtils extends QueryErrorsBase {
+ // Callers cache this result and force parsing before their NULL
short-circuit.
+ // A dynamic path or constant NULL has no cached parsed path.
+ def foldablePath(
+ path: Expression,
+ functionName: String,
+ allowRoot: Boolean = false): Option[FoldableVariantPath] =
+ VariantPathArg.fromExpression(path, functionName, allowRoot).collect {
+ case parsed: FoldableVariantPath => parsed
+ }
+
+ def checkValueType(dataType: DataType): TypeCheckResult = {
Review Comment:
Now that `checkValueType` lives here, `VariantInsert` and
`VariantArrayAppend` no longer call `toSQL*`. Please drop their unused
`QueryErrorsBase` mixins. `VariantSet` still needs it for the
`create_if_missing` error.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/variant/variantExpressions.scala:
##########
@@ -391,6 +391,71 @@ object VariantPathParser extends RegexParsers {
}
}
+sealed trait VariantPathArg
+
+case class FoldableVariantPath(segments: Array[VariantPathSegment], pathStr:
String)
Review Comment:
Nit: this type is now shared with insert/set/append, which still pass
`pathStr` into eval helpers for error messages. The old `ParsedInsertPath` /
`ParsedSetPath` comments explained that, and that `VariantBuilder.PathSegment`
is not `Serializable` so `javaSegments` has to be `@transient` and rebuilt per
task.
Worth keeping both notes here. Delete/pick do not use `pathStr`; carrying it
is harmless, but otherwise it looks unused.
--
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]
