cloud-fan commented on code in PR #59467:
URL: https://github.com/apache/spark/pull/59467#discussion_r4275151188
##########
sql/core/src/test/scala/org/apache/spark/sql/CharVarcharTestSuite.scala:
##########
@@ -3999,4 +4087,36 @@ class DSV2CharVarcharTestSuite extends
CharVarcharTestSuite
}
}
}
+
+ test("SPARK-59016: V2 column prune does not re-stamp CHAR padding
breadcrumb") {
+ withSQLConf(SQLConf.CHAR_VARCHAR_STANDARD_SEMANTICS.key -> "true") {
Review Comment:
**Nit (P3):** This test sets `CHAR_VARCHAR_STANDARD_SEMANTICS` for its whole
body, so `SQLConf.get.charVarcharStandardSemantics` short-circuits the new
condition in `toOutputAttrs`. The change also affects the default mode
(standard semantics off), though. With the default `readSideCharPadding=true`,
the relation is padded and cleaned, and the structural
`getRawTypeString(orig.metadata).isEmpty` check now drops the marker where the
merge base kept it, which matters for connectors like JDBC v2 whose
`readSchema()` carries it. With `readSideCharPadding=false`, the relation stays
stamped and the marker is kept. Neither branch is asserted here: deleting the
structural disjunct, or making the else branch always clean, keeps all three
assertions green. Could you add default-mode cases next to this one?
**Verification:**
- **Regression:** Reverting the default-mode structural check to the
merge-base always-keep behavior must make the default-semantics padded case
fail through the direct toOutputAttrs call with a CHAR(5) read schema; the
end-to-end InMemory scan check alone is not sensitive to that revert.
- **Behavior:** With standard semantics off and read-side padding disabled,
a pruned DSv2 scan of a still-stamped relation keeps the marker, and changing
the keep branch to always clean must fail this case.
##########
sql/core/src/test/scala/org/apache/spark/sql/CharVarcharTestSuite.scala:
##########
@@ -1540,6 +1544,41 @@ class BasicCharVarcharTestSuite extends
SharedSparkSession {
"%s(c, c, c)", "%s(array(array(c)))", "%s(named_struct('x', c))",
"%s(map(c, 1))", "%s(map(1, c))")
+ // Signatures the scalar/nested templates do not typecheck: 3-string
(translate),
+ // (string, string, int) overlay/split_part/substring_index, format_string's
format
+ // arg, HOF lambdas, and window OVER. '%%s' survives String.format as '%s'.
+ private val inventoryGatedShapes = Seq(
+ "%s(c, 'x', 1)", "%s(c, '-', 1)", "%s(c, c, 1)", "%s(c, 'ab', 'xy')",
+ "%s('%%s', c)", "%s(array(c), x -> x)", "%s(array(c), x -> reverse(x))",
+ "%s(c) OVER (ORDER BY 1)", "%s(c, 1) OVER (ORDER BY 1)")
+
+ // These must typecheck (not Try.toOption). split_part / substring_index /
+ // translate reject case-insensitive collations (NonCSAI).
Review Comment:
**Nit (P3):** `StringTypeNonCSAICollation.acceptsStringType` is
`other.isCaseInsensitive || !other.isAccentInsensitive`, so it rejects only
case-sensitive, accent-insensitive collations (e.g. `UNICODE_AI`). `UTF8_LCASE`
is accepted, because `CollationFactory.isCaseInsensitive` and
`isAccentInsensitive` are both false for non-ICU collations.
`collations-string-functions.sql.out` also shows `translate`, `split_part`, and
`substring_index` all analyzing with `string collate UTF8_LCASE` results.
Because of this comment, the UTF8_LCASE run in 'typecheck-gated transforming
calls return STRING' uses `requiredTransformingCallsOnAnyCollation` and skips
these three calls. On that column they are only probed through the
`Try(...).toOption` gated inventory, so a regression that made them fail
analysis on a collated CHAR column would be silently skipped. Can we run the
full `requiredTransformingCalls` list on the UTF8_LCASE column too, and fix the
comment? If one of them really does fail there
, the comment should give the actual reason.
**Verification:**
- **Behavior:** Every required transforming call analyzes and returns plain
STRING on CHAR, VARCHAR, UNICODE-collated, and UTF8_LCASE-collated CHAR columns
under standard semantics.
- **Regression:** If any of the three calls failed analysis or kept CHAR on
the UTF8_LCASE column, the required-call test fails, not just the Try-masked
inventory.
##########
sql/core/src/test/scala/org/apache/spark/sql/CharVarcharTestSuite.scala:
##########
@@ -1540,6 +1544,41 @@ class BasicCharVarcharTestSuite extends
SharedSparkSession {
"%s(c, c, c)", "%s(array(array(c)))", "%s(named_struct('x', c))",
"%s(map(c, 1))", "%s(map(1, c))")
+ // Signatures the scalar/nested templates do not typecheck: 3-string
(translate),
Review Comment:
**Nit (P3):** This rationale doesn't match the existing templates.
`inventoryNestedShapes` already has `"%s(c, c, c)"`, which typechecks
`translate(c, c, c)`. Through the ANSI implicit cast of the string third
argument to INT, it also typechecks `overlay`/`split_part`/`substring_index(c,
c, c)`. `inventoryScalarShapes` has `"%s('x', c)"`, which typechecks
`format_string`/`printf`. Only the HOF lambda and window `OVER` shapes are
really out of reach of the earlier templates. For the other functions, the
gated shapes add literal-argument positions (`'ab', 'xy'`, `'-', 1`) and a
format string that actually consumes `c` (`'%%s'`). Could you reword the
comment along those lines?
**Verification:**
- **Inspection:** Every signature the comment says only the gated shapes
typecheck fails to typecheck under the scalar and nested templates.
--
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]
