dongjoon-hyun commented on code in PR #60089:
URL: https://github.com/apache/spark/pull/60089#discussion_r4499950555
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/variant/VariantExpressionEvalUtils.scala:
##########
@@ -321,13 +321,25 @@ object VariantExpressionEvalUtils {
}
/** Cast a Spark value from `dataType` into the variant type. */
- def castToVariant(input: Any, dataType: DataType): VariantVal = {
+ def castToVariant(input: Any, dataType: DataType): VariantVal =
+ castToVariant(input, dataType, "cast")
+
+ /**
+ * Cast a Spark value from `dataType` into the variant type. A result over
the size limit raises
+ * `VARIANT_SIZE_LIMIT` with `functionName`.
+ */
+ def castToVariant(input: Any, dataType: DataType, functionName: String):
VariantVal = {
// Enforce strict check because it is illegal for input struct/map/variant
to contain duplicate
// keys.
val builder = new VariantBuilder(false)
- buildVariant(builder, input, dataType)
- val v = builder.result()
- new VariantVal(v.getValue, v.getMetadata)
+ try {
+ buildVariant(builder, input, dataType)
+ val v = builder.result()
Review Comment:
**[2] `variant_from_entries` has the same gap.**
`variantFromEntries` (L385-414, outside this diff) calls `buildVariant`
(L408) and `builder.result()` (L412) with no catch either, so `SELECT
variant_from_entries(array(struct('a', repeat('x', 16777216))))` also fails
with the unclassified exception. Could it raise `VARIANT_SIZE_LIMIT` naming
`variant_from_entries`?
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/variant/VariantExpressionEvalUtils.scala:
##########
@@ -321,13 +321,25 @@ object VariantExpressionEvalUtils {
}
/** Cast a Spark value from `dataType` into the variant type. */
- def castToVariant(input: Any, dataType: DataType): VariantVal = {
+ def castToVariant(input: Any, dataType: DataType): VariantVal =
+ castToVariant(input, dataType, "cast")
+
+ /**
+ * Cast a Spark value from `dataType` into the variant type. A result over
the size limit raises
+ * `VARIANT_SIZE_LIMIT` with `functionName`.
+ */
+ def castToVariant(input: Any, dataType: DataType, functionName: String):
VariantVal = {
// Enforce strict check because it is illegal for input struct/map/variant
to contain duplicate
// keys.
val builder = new VariantBuilder(false)
- buildVariant(builder, input, dataType)
- val v = builder.result()
- new VariantVal(v.getValue, v.getMetadata)
+ try {
+ buildVariant(builder, input, dataType)
Review Comment:
**[1] `variant_from_arrays` still throws the bare
`VariantSizeLimitException`.**
`variantFromArrays` (L351-375, outside this diff) calls the same
`buildVariant` (L369) and `builder.result()` (L373) with no catch, so `SELECT
variant_from_arrays(array('a'), array(repeat('x', 16777216)))` still fails with
the unclassified exception instead of `VARIANT_SIZE_LIMIT`. It came with
SPARK-51975 on master and branch-4.x after v4.3.0, so it is not released yet.
Could this PR map it too, naming `variant_from_arrays`?
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/variant/VariantExpressionEvalUtils.scala:
##########
@@ -321,13 +321,25 @@ object VariantExpressionEvalUtils {
}
/** Cast a Spark value from `dataType` into the variant type. */
- def castToVariant(input: Any, dataType: DataType): VariantVal = {
+ def castToVariant(input: Any, dataType: DataType): VariantVal =
+ castToVariant(input, dataType, "cast")
+
+ /**
+ * Cast a Spark value from `dataType` into the variant type. A result over
the size limit raises
+ * `VARIANT_SIZE_LIMIT` with `functionName`.
+ */
+ def castToVariant(input: Any, dataType: DataType, functionName: String):
VariantVal = {
// Enforce strict check because it is illegal for input struct/map/variant
to contain duplicate
// keys.
val builder = new VariantBuilder(false)
- buildVariant(builder, input, dataType)
- val v = builder.result()
- new VariantVal(v.getValue, v.getMetadata)
+ try {
+ buildVariant(builder, input, dataType)
+ val v = builder.result()
+ new VariantVal(v.getValue, v.getMetadata)
+ } catch {
+ case _: VariantSizeLimitException =>
Review Comment:
**[8] Nit: keep the caught exception as the cause.**
`case _ =>` drops the `VariantSizeLimitException`, so the trace no longer
shows whether the value buffer (`checkCapacity`) or the key dictionary
(`result()`) went over the limit. `QueryExecutionErrors.variantSizeLimitError`
has no `cause` parameter yet, and the sibling catches (L52, L177, L227, L277)
drop it the same way. Could it take one, here or in a follow-up for all of them?
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/variant/VariantExpressionEvalUtils.scala:
##########
@@ -321,13 +321,25 @@ object VariantExpressionEvalUtils {
}
/** Cast a Spark value from `dataType` into the variant type. */
- def castToVariant(input: Any, dataType: DataType): VariantVal = {
+ def castToVariant(input: Any, dataType: DataType): VariantVal =
Review Comment:
**[5] Minor: the Python data source filter path now reports `cast`.**
UserDefinedPythonDataSource.scala:465 goes through this overload, so a
pushed-down filter whose literal is over 16 MiB now fails the query with
`VARIANT_SIZE_LIMIT` naming `cast`, which the user never wrote. This is not a
regression: the bare exception is not the `MatchError` that `construct`
catches, so the query failed before this PR too. But `construct` already treats
a value it cannot cast as an unsupported filter, so could it return `None` for
this error as well and let Spark evaluate the filter?
UserDefinedPythonDataSource.scala is outside this diff, so I left this here.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/variant/VariantExpressionEvalUtils.scala:
##########
@@ -321,13 +321,25 @@ object VariantExpressionEvalUtils {
}
/** Cast a Spark value from `dataType` into the variant type. */
- def castToVariant(input: Any, dataType: DataType): VariantVal = {
+ def castToVariant(input: Any, dataType: DataType): VariantVal =
+ castToVariant(input, dataType, "cast")
Review Comment:
**[4] Minor: make `functionName` required instead of a hidden `"cast"`
default.**
Only `Cast` (Cast.scala:1574 and Cast.scala:1689) wants `cast` here. The
other callers of this overload are `Literal` (literals.scala:248, a constant
`0` that cannot overflow) and the Python data source filter serialization (item
5), which is not a cast. Could you remove this overload and pass `"cast"`
explicitly in `Cast`, so that a new caller cannot report `cast` by accident?
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/variant/VariantExpressionEvalUtils.scala:
##########
@@ -162,7 +162,7 @@ object VariantExpressionEvalUtils {
legacyPathBackslashAsLiteral: Boolean): VariantVal = {
val v = new Variant(input.getValue, input.getMetadata)
try {
- val valVal = castToVariant(value, valueDataType)
+ val valVal = castToVariant(value, valueDataType, functionName)
Review Comment:
**[7] Nit: the docs of the mutation functions still credit the shared try.**
The docs of `insertAtPath` (L148) and `setAtPath` (L199), both outside this
diff, say the cast and the mutation share one try, so any size overflow maps to
`VARIANT_SIZE_LIMIT`. Now `castToVariant` maps its own overflow with
`functionName`, and the outer `case _: VariantSizeLimitException` (L177, L227)
only covers `VariantBuilder.insertAtPath`/`setAtPath`. Could the docs say that,
so that nobody drops the `functionName` argument here thinking the outer catch
still renames the error?
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/variant/VariantExpressionSuite.scala:
##########
@@ -1176,6 +1176,17 @@ class VariantExpressionSuite extends SparkFunSuite with
ExpressionEvalHelper {
checkFailure(true, toVariantObject = true)
checkFailure(Literal.create(Literal.create(Period.ofMonths(0))),
toVariantObject = true)
checkFailure(Map(1 -> 1), toVariantObject = true)
+
+ // A result over the size limit raises VARIANT_SIZE_LIMIT naming the
function.
+ val tooBig = "x".repeat(16 * 1024 * 1024)
+ checkErrorInExpression[SparkRuntimeException](
Review Comment:
**[6] Minor: test `try_cast` and an overflow in `builder.result()`.**
The new checks cover one 16 MiB string value. `try_cast` should still return
null (`Cast.cast` catches every `Exception` in TRY mode), and
`VariantBuilder.result()` throws the same exception when the key dictionary is
over the limit, after `buildVariant` has finished, e.g. `to_variant_object` of
a map with two 9 MiB keys. Could you add both? They would fail if the catch
wrapped only `buildVariant`, or if `try_cast` started to raise.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/variant/VariantExpressionEvalUtils.scala:
##########
@@ -321,13 +321,25 @@ object VariantExpressionEvalUtils {
}
/** Cast a Spark value from `dataType` into the variant type. */
- def castToVariant(input: Any, dataType: DataType): VariantVal = {
+ def castToVariant(input: Any, dataType: DataType): VariantVal =
+ castToVariant(input, dataType, "cast")
+
+ /**
+ * Cast a Spark value from `dataType` into the variant type. A result over
the size limit raises
+ * `VARIANT_SIZE_LIMIT` with `functionName`.
+ */
+ def castToVariant(input: Any, dataType: DataType, functionName: String):
VariantVal = {
Review Comment:
**[3] Map the size error once for every builder in this object.**
`castToVariant`, `variantFromArrays` and `variantFromEntries` all create a
`VariantBuilder(false)`, write into it and finish with `builder.result()`, but
only this one maps the size error, which is how items 1 and 2 were missed.
Could a small helper, e.g. `buildVariantVal(functionName)(write: VariantBuilder
=> Unit): VariantVal`, own the builder, the `result()` call and this catch, so
that all three use it?
--
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]
