cloud-fan commented on code in PR #59467:
URL: https://github.com/apache/spark/pull/59467#discussion_r4280002942
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/v2/PushDownUtils.scala:
##########
@@ -717,9 +717,21 @@ object PushDownUtils extends Logging {
relation: DataSourceV2Relation): Seq[AttributeReference] = {
val nameToAttr = Utils.toMap(relation.output.map(_.name), relation.output)
val cleaned = CharVarcharUtils.replaceCharVarcharWithStringInSchema(schema)
- toAttributes(cleaned).map {
- // we have to keep the attribute id during transformation
- a => a.withExprId(nameToAttr(a.name).exprId)
+ toAttributes(cleaned).map { a =>
+ val orig = nameToAttr(a.name)
+ // Keep exprId. Do not re-stamp __CHAR_VARCHAR_TYPE_STRING after
+ // ApplyCharTypePadding cleaned it: that marker means "not yet padded",
+ // and this runs in the optimizer with no second padding pass. File
+ // sources usually already substituted STRING, so the stamp was latent;
+ // a V2 scan whose readSchema() still surfaces CHAR/VARCHAR would
+ // otherwise look unpadded. Key off the relation's own padding state,
+ // not the session conf: a still-stamped relation was not padded.
Review Comment:
**Nit (P3):** This sentence doesn't hold for versioned tables. In a
non-command query, `QueryExecution`'s table-version refresh runs
`AnalyzedSchemaProjection.rebindToAnalyzedSchema`, which re-creates the
relation output through `DataSourceV2Relation.create` and so re-stamps the
marker. `sameAttributeShape` compares metadata, so the cleaned attribute isn't
reused. The relation that reaches `toOutputAttrs` is then stamped even though
`ApplyCharTypePadding` already padded it. Your test comment in
`CharVarcharTestSuite` ('The InMemory prune path can still see a stamped
relation object') says the same thing.
Could you reword this to describe the rule the code applies? The marker is
dropped when the relation attribute passed in has none, and kept otherwise,
including for a padded versioned relation that the refresh rebound. The final
wording depends on the versioned-table decision challenge in the review body.
**Verification:**
- **Inspection:** The comment's stated rule matches the branch condition on
the relation attribute's marker. It claims no equivalence between the marker
and padding state that the versioned refresh rebind violates, and it is
consistent with the DSv2 test comments about the InMemory prune path.
##########
sql/core/src/test/scala/org/apache/spark/sql/CharVarcharTestSuite.scala:
##########
@@ -3999,4 +4085,78 @@ class DSV2CharVarcharTestSuite extends
CharVarcharTestSuite
}
}
}
+
+ private def withPrunedV2CharRelation(body: (DataFrame, DataSourceV2Relation)
=> Unit): Unit = {
+ withTable("std_v2_prune") {
+ sql(s"CREATE TABLE std_v2_prune (c CHAR(5), extra INT) USING $format")
+ sql("INSERT INTO std_v2_prune VALUES ('ab', 1)")
+ val df = sql("SELECT c FROM std_v2_prune")
+ val rel = df.queryExecution.analyzed.collect {
+ case r: DataSourceV2Relation => r
+ }.head
+ body(df, rel)
+ }
+ }
+
+ private def attrsFromCharReadSchema(rel: DataSourceV2Relation) =
+ PushDownUtils.toOutputAttrs(new StructType().add("c", CharType(5)), rel)
+
+ 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):** One configuration is missing from this matrix: standard
semantics on with `READ_SIDE_CHAR_PADDING=false`. There `ApplyCharTypePadding`
only logs a warning and still pads and cleans the relation
(`ApplyCharTypePadding.scala:59-70`), so the gate should drop the marker. None
of the three tests runs it, so a gate written as `if
(SQLConf.get.readSideCharPadding) cleanMetadata(...) else ...` would pass all
three. Could you add a case next to this one with standard semantics on and
read-side padding off? It should assert that the relation is cleaned, that the
direct `toOutputAttrs` call drops the marker, and that the query returns the
padded value.
This case doesn't separate the structural gate from the earlier
`charVarcharStandardSemantics` disjunct, because both strip there. That choice
is part of the decision challenge in the review body.
This extends an [existing
discussion](https://github.com/apache/spark/pull/59467#discussion_r4275151188).
**Verification:**
- **Regression:** Replacing the relation-state gate with a
readSideCharPadding-keyed condition makes the new case fail, while the three
existing prune cases keep passing.
- **Behavior:** With standard semantics on and read-side padding disabled,
the pruned single-column query over a CHAR(5) DSv2 column returns the padded
value.
--
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]
