HyukjinKwon commented on code in PR #59467:
URL: https://github.com/apache/spark/pull/59467#discussion_r4280014422
##########
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 DSv2 tables. Between
analysis and optimization, `QueryExecution.tableVersionsRefreshed` runs
`V2TableRefreshUtil.refresh(versionedOnly = true)`. For a non-command query,
`AnalyzedSchemaProjection.rebindToAnalyzedSchema` then rebuilds the relation
with `DataSourceV2Relation.create`, which stamps `__CHAR_VARCHAR_TYPE_STRING`
again. `sameAttributeShape` compares metadata, so a column that
`ApplyCharTypePadding` already padded and cleaned is rebound to the fresh
stamped attribute (under an alias that keeps the captured metadata), while the
padding `Project` stays above it. So `toOutputAttrs` sees a stamped relation
for an already-padded column and keeps the marker.
`InMemoryBaseTable.version()` is non-null, so the suite's own `testcat` tables
take this path. That is presumably what the test comment "The InMemory prune
path can still see a stamped relation object" is seeing.
Could you reword this comment, and that test comment, to say when the marker
survives? For example: relations re-bound by the refresh phase after padding
arrive stamped and keep it. If you extend the fix to cover that path instead
(see the decision challenge), the comment should describe the new rule.
**Verification:**
- **Inspection:** Reading the toOutputAttrs comment and the
standard-semantics DSv2 prune test comment gives a description of when the
marker is kept that matches the pinned refresh path: versioned relations
rebound after padding keep it, and unstamped relations drop 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]
