cloud-fan commented on code in PR #59928:
URL: https://github.com/apache/spark/pull/59928#discussion_r4444280124
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/variant/VariantExpressionSuite.scala:
##########
@@ -100,7 +102,9 @@ class VariantExpressionSuite extends SparkFunSuite with
ExpressionEvalHelper {
val a = createArray[Byte](SIZE_LIMIT, 'a'.toByte)
val hugeMetadata = bigVersion ++ Array[Byte](2, 0, 0, 0, 0, 0, 0, 0, 0, 0,
0, 1, 1, 0, 0, 1) ++
a ++ Array[Byte]('b')
- check(Array(primitiveHeader(TRUE)), hugeMetadata,
"VARIANT_CONSTRUCTOR_SIZE_LIMIT")
+ val sizeLimitParams = Map("sizeLimit" -> "16 MiB")
Review Comment:
**Nit (P3):** Every test JVM runs with `spark.testing` set, so
`VariantUtil.SIZE_LIMIT` is 16 MiB here, and `"16 MiB"` is exactly the text the
old template hard-coded. If `variantConstructorSizeLimit()` went back to a
constant `"16 MiB"`, or to a format that is only right at 16 MiB, these three
assertions would still pass in both codegen modes, while production would again
report 16 MiB for the 128 MiB limit. That 128 MiB value is what this PR fixes,
and no test checks it.
Because `SIZE_LIMIT` is fixed at class init, one cheap option is to have
`variantConstructorSizeLimit` take the limit as an argument (with the `Variant`
constructor passing the same `SIZE_LIMIT` it checks), and add a small suite
under `common/variant/src/test/scala/org/apache/spark/types/variant` that
builds the exception for `128 * 1024 * 1024` and checks `getMessageParameters`
and the rendered message. A child JVM started without `spark.testing`, as
`VariantUtf8DecodeSuite` does, would also work without the signature change,
but it is slower and heavier.
**Verification:**
- **Regression:** For a 128 MiB limit, the exception's sizeLimit parameter
is "128 MiB" and its rendered message states 128 MiB in both places; this check
fails if the factory reports a constant "16 MiB" instead of the limit it was
given.
- **Behavior:** Under the test-mode limit, oversized metadata, an oversized
value, and both together still raise VARIANT_CONSTRUCTOR_SIZE_LIMIT with
sizeLimit "16 MiB" in both interpreted and generated evaluation.
- **Inspection:** The Variant constructor passes to the factory the same
SIZE_LIMIT value it compares the value and metadata lengths against, and no
other caller of the factory exists.
--
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]
