Skip to content

Commit 717d7c2

Browse files
yyanyyIsaac
andcommitted
[SPARK-59015][SQL] Match rebinding names with the fold resolution uses
Rebinding compared captured and current names with the resolver, while name resolution and refresh validation both key on the `toLowerCase` fold and only filter resolver candidates afterwards. The two rules disagree: `equalsIgnoreCase` equates U+017F LONG S with `s`, the fold does not. Capturing `s`, renaming it to `S` and adding U+017F is therefore a change validation accepts and a fresh query resolves, but rebinding saw two candidates for `s` and failed the query with INTERNAL_ERROR. Match on a shared `SchemaUtils.foldName` instead. This removes the exact-name preference and the ambiguity branch rather than adding to them: the fold is single-valued because the duplicate-name check rejects a schema holding two names that fold alike, so the ambiguity error is now unreachable. Also: - Assert `assertNotCached` after the second refresh in the SPARK-54424 cache test. `numCachedEntries == 1` held whether the entry was usable or not, so it did not record that the entry stops being reused. - Assert the read schema and the filters reaching the source in a nested-refresh test, pinning the nested pruning and pushdown the rebuilt struct currently costs. Verified by reverting the match to the resolver: the new end-to-end and unit tests fail with `captured name s matches multiple current names [S, ſ]`. Co-authored-by: Isaac <no-reply@databricks.com>
1 parent cd39507 commit 717d7c2

5 files changed

Lines changed: 178 additions & 73 deletions

File tree

sql/catalyst/src/main/scala/org/apache/spark/sql/connector/catalog/V2TableUtil.scala

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -17,8 +17,6 @@
1717

1818
package org.apache.spark.sql.connector.catalog
1919

20-
import java.util.Locale
21-
2220
import org.apache.spark.sql.catalyst.SQLConfHelper
2321
import org.apache.spark.sql.catalyst.analysis.Resolver
2422
import org.apache.spark.sql.catalyst.expressions.MetadataAttributeWithLogicalName
@@ -198,7 +196,7 @@ private[sql] object V2TableUtil extends SQLConfHelper {
198196
}
199197

200198
private def normalize(name: String): String = {
201-
if (conf.caseSensitiveAnalysis) name else name.toLowerCase(Locale.ROOT)
199+
SchemaUtils.foldName(name, conf.caseSensitiveAnalysis)
202200
}
203201

204202
private def resolver: Resolver = conf.resolver

sql/catalyst/src/main/scala/org/apache/spark/sql/util/SchemaUtils.scala

Lines changed: 16 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -529,12 +529,23 @@ private[spark] object SchemaUtils {
529529
if (field.nullable) s"$name $dataType" else s"$name $dataType NOT NULL"
530530
}
531531

532+
/**
533+
* Folds a name to the key used to decide whether two names refer to the same column or field.
534+
*
535+
* This is the identity rule name resolution is built on: `AttributeSeq` looks attributes up by
536+
* this key and only then filters the candidates with the resolver, and the duplicate-name checks
537+
* above reject a schema that holds two names folding to one key. Matching by folded name
538+
* therefore finds exactly the field resolution would, while comparing with the resolver alone
539+
* can match several fields a schema is allowed to keep apart (`equalsIgnoreCase` equates U+017F
540+
* LONG S with `s`, which this fold does not).
541+
*/
542+
def foldName(name: String, caseSensitiveAnalysis: Boolean): String = {
543+
if (caseSensitiveAnalysis) name else name.toLowerCase(Locale.ROOT)
544+
}
545+
532546
private def index(fields: Array[StructField], resolver: Resolver): Map[String, StructField] = {
533-
if (isCaseSensitiveAnalysis(resolver)) {
534-
fields.map(field => field.name -> field).toMap
535-
} else {
536-
fields.map(field => field.name.toLowerCase(Locale.ROOT) -> field).toMap
537-
}
547+
val caseSensitive = isCaseSensitiveAnalysis(resolver)
548+
fields.map(field => foldName(field.name, caseSensitive) -> field).toMap
538549
}
539550

540551
/**

sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/v2/CapturedSchemaProjection.scala

Lines changed: 25 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -19,11 +19,11 @@ package org.apache.spark.sql.execution.datasources.v2
1919

2020
import org.apache.spark.SparkException
2121
import org.apache.spark.sql.catalyst.SQLConfHelper
22-
import org.apache.spark.sql.catalyst.analysis.Resolver
2322
import org.apache.spark.sql.catalyst.expressions.{Alias, ArrayTransform, AttributeReference, CreateNamedStruct, Expression, GetStructField, If, IsNull, KnownNotNull, LambdaFunction, Literal, MetadataAttributeWithLogicalName, NamedLambdaVariable, TaggingExpression, TransformKeys, TransformValues, UnresolvedNamedLambdaVariable}
2423
import org.apache.spark.sql.catalyst.plans.logical.{LogicalPlan, Project}
2524
import org.apache.spark.sql.catalyst.util.MetadataColumnHelper
2625
import org.apache.spark.sql.types.{ArrayType, DataType, MapType, Metadata, StructType}
26+
import org.apache.spark.sql.util.SchemaUtils
2727

2828
/**
2929
* Rebinds a relation that reads a current table schema to output attributes captured from an
@@ -46,7 +46,7 @@ private[sql] object CapturedSchemaProjection extends SQLConfHelper {
4646
// The relation still carries the output captured at analysis time; only its table has been
4747
// swapped for the current one.
4848
val capturedOutput = relation.output
49-
val resolver = conf.resolver
49+
val caseSensitive = conf.caseSensitiveAnalysis
5050
val current = DataSourceV2Relation.create(
5151
relation.table,
5252
relation.catalog,
@@ -56,7 +56,7 @@ private[sql] object CapturedSchemaProjection extends SQLConfHelper {
5656
val currentMetadataOutput = current.metadataOutput
5757
val currentMetadata = capturedOutput.filter(_.isMetadataCol).map { captured =>
5858
val logicalName = metadataLogicalName(captured)
59-
matchName(currentMetadataOutput, logicalName, resolver)(metadataLogicalName)
59+
matchName(currentMetadataOutput, logicalName, caseSensitive)(metadataLogicalName)
6060
.map(pos => currentMetadataOutput(pos))
6161
.getOrElse {
6262
// The connector still reports this metadata column, so it can only be absent here
@@ -81,13 +81,13 @@ private[sql] object CapturedSchemaProjection extends SQLConfHelper {
8181
return relation
8282
}
8383

84-
val capturedIndex = new AttributeIndex(capturedOutput, resolver)
84+
val capturedIndex = new AttributeIndex(capturedOutput, caseSensitive)
8585
val reboundOutput = currentOutput.map { currentAttr =>
8686
capturedIndex.get(currentAttr).filter(canReuse(_, currentAttr)).getOrElse(currentAttr)
8787
}
8888
val reboundRelation = relation.copy(output = reboundOutput)
8989

90-
val reboundIndex = new AttributeIndex(reboundOutput, resolver)
90+
val reboundIndex = new AttributeIndex(reboundOutput, caseSensitive)
9191
val projectList = capturedOutput.map { capturedAttr =>
9292
val currentAttr = reboundIndex.get(capturedAttr).getOrElse {
9393
unexpectedSchemaChange(
@@ -102,7 +102,7 @@ private[sql] object CapturedSchemaProjection extends SQLConfHelper {
102102
s"nullability changed for captured column ${capturedAttr.name} in ${relation.name}")
103103
}
104104
val projected = projectToType(
105-
currentAttr, currentAttr.dataType, capturedAttr.dataType, resolver)
105+
currentAttr, currentAttr.dataType, capturedAttr.dataType, caseSensitive)
106106
if (projected.dataType != capturedAttr.dataType ||
107107
projected.nullable != capturedAttr.nullable) {
108108
unexpectedSchemaChange(
@@ -122,7 +122,7 @@ private[sql] object CapturedSchemaProjection extends SQLConfHelper {
122122
input: Expression,
123123
from: DataType,
124124
to: DataType,
125-
resolver: Resolver): Expression = {
125+
caseSensitive: Boolean): Expression = {
126126
if (from == to) {
127127
return input
128128
}
@@ -131,7 +131,7 @@ private[sql] object CapturedSchemaProjection extends SQLConfHelper {
131131
case (fromStruct: StructType, toStruct: StructType) =>
132132
val structInput = if (input.nullable) KnownNotNull(input) else input
133133
val fields = toStruct.fields.iterator.flatMap { targetField =>
134-
val index = matchName(fromStruct, targetField.name, resolver)(_.name).getOrElse {
134+
val index = matchName(fromStruct, targetField.name, caseSensitive)(_.name).getOrElse {
135135
unexpectedSchemaChange(
136136
s"captured struct field ${targetField.name} is missing from $fromStruct")
137137
}
@@ -140,7 +140,7 @@ private[sql] object CapturedSchemaProjection extends SQLConfHelper {
140140
GetStructField(structInput, index, Some(sourceField.name)),
141141
sourceField.dataType,
142142
targetField.dataType,
143-
resolver)
143+
caseSensitive)
144144
val namedValue = if (targetField.metadata == Metadata.empty) {
145145
// An empty captured value is still an explicit instruction not to inherit metadata
146146
// from the current GetStructField. CleanupAliases removes an empty-metadata Alias, so
@@ -172,7 +172,7 @@ private[sql] object CapturedSchemaProjection extends SQLConfHelper {
172172
ArrayTransform(
173173
input,
174174
LambdaFunction(
175-
projectToType(element, fromElement, toElement, resolver), Seq(element)))
175+
projectToType(element, fromElement, toElement, caseSensitive), Seq(element)))
176176

177177
case (
178178
MapType(fromKey, fromValue, fromValueContainsNull),
@@ -197,7 +197,7 @@ private[sql] object CapturedSchemaProjection extends SQLConfHelper {
197197
// duplicate keys.
198198
TransformKeys(
199199
input,
200-
LambdaFunction(projectToType(key, fromKey, toKey, resolver), Seq(key, value)))
200+
LambdaFunction(projectToType(key, fromKey, toKey, caseSensitive), Seq(key, value)))
201201
} else {
202202
input
203203
}
@@ -214,7 +214,7 @@ private[sql] object CapturedSchemaProjection extends SQLConfHelper {
214214
TransformValues(
215215
withProjectedKeys,
216216
LambdaFunction(
217-
projectToType(value, fromValue, toValue, resolver), Seq(key, value)))
217+
projectToType(value, fromValue, toValue, caseSensitive), Seq(key, value)))
218218
} else {
219219
withProjectedKeys
220220
}
@@ -231,37 +231,31 @@ private[sql] object CapturedSchemaProjection extends SQLConfHelper {
231231
}
232232

233233
/**
234-
* Returns the position of the entry whose name matches `target`, if any.
234+
* Returns the position of the first entry whose folded name matches `target`, if any.
235235
*
236-
* An exact match wins so that a name binds to itself even when the resolver cannot tell it apart
237-
* from another name in the same schema. Duplicate names are rejected by folding with
238-
* `toLowerCase` while resolution compares with `equalsIgnoreCase`, so a schema can legally hold
239-
* several names the resolver considers equal. Without an exact match the resolver match has to be
240-
* unique: nothing here can decide which of two indistinguishable names the captured plan read.
236+
* Matching on [[SchemaUtils.foldName]] rather than comparing with the resolver keeps rebinding on
237+
* the identity rule the rest of resolution uses, so a captured name binds to the field that
238+
* validation matched it to and the field a fresh query would resolve it to. The fold is also
239+
* single-valued where the resolver is not: refresh validation rejects a schema holding two names
240+
* that fold alike, so at most one candidate can match here.
241241
*/
242242
private def matchName[T](
243243
candidates: Seq[T],
244244
target: String,
245-
resolver: Resolver)(name: T => String): Option[Int] = {
246-
val exact = candidates.indexWhere(candidate => name(candidate) == target)
247-
if (exact >= 0) {
248-
return Some(exact)
245+
caseSensitive: Boolean)(name: T => String): Option[Int] = {
246+
val foldedTarget = SchemaUtils.foldName(target, caseSensitive)
247+
val pos = candidates.indexWhere { candidate =>
248+
SchemaUtils.foldName(name(candidate), caseSensitive) == foldedTarget
249249
}
250-
val matches = candidates.indices.filter(pos => resolver(name(candidates(pos)), target))
251-
if (matches.length > 1) {
252-
unexpectedSchemaChange(
253-
s"captured name $target matches multiple current names " +
254-
matches.map(pos => name(candidates(pos))).mkString("[", ", ", "]"))
255-
}
256-
matches.headOption
250+
if (pos >= 0) Some(pos) else None
257251
}
258252

259253
/**
260254
* Indexes attributes by name for the rebinding lookups. Data and metadata attributes are indexed
261255
* separately because a metadata attribute matches on its logical name, which a data column may
262256
* also carry.
263257
*/
264-
private class AttributeIndex(attributes: Seq[AttributeReference], resolver: Resolver) {
258+
private class AttributeIndex(attributes: Seq[AttributeReference], caseSensitive: Boolean) {
265259
private val dataAttrs = attributes.filterNot(_.isMetadataCol)
266260
private val metadataAttrs = attributes.filter(_.isMetadataCol)
267261

@@ -275,7 +269,7 @@ private[sql] object CapturedSchemaProjection extends SQLConfHelper {
275269

276270
private def find(attrs: Seq[AttributeReference], targetName: String)(
277271
name: AttributeReference => String): Option[AttributeReference] = {
278-
matchName(attrs, targetName, resolver)(name).map(pos => attrs(pos))
272+
matchName(attrs, targetName, caseSensitive)(name).map(pos => attrs(pos))
279273
}
280274
}
281275

sql/core/src/test/scala/org/apache/spark/sql/connector/DataSourceV2DataFrameSuite.scala

Lines changed: 94 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1585,6 +1585,12 @@ class DataSourceV2DataFrameSuite
15851585
stripAQEPlan(qe.executedPlan).asInstanceOf[T]
15861586
}
15871587

1588+
private def scanRelationOf(df: DataFrame): DataSourceV2ScanRelation = {
1589+
df.queryExecution.optimizedPlan.collectFirst {
1590+
case scan: DataSourceV2ScanRelation => scan
1591+
}.get
1592+
}
1593+
15881594
private def checkDefaultValues(
15891595
columns: Array[Column],
15901596
expectedDefaultValues: Array[ColumnDefaultValue],
@@ -1726,6 +1732,36 @@ class DataSourceV2DataFrameSuite
17261732
}
17271733
}
17281734

1735+
test("rebind a captured column renamed beside an addition the resolver cannot tell apart") {
1736+
// U+017F LONG S folds to itself under `toLowerCase`, so it is a distinct column name to the
1737+
// fold that name resolution and refresh validation key on, while `equalsIgnoreCase` equates it
1738+
// with `s`. Renaming `s` to `S` and adding U+017F is therefore a compatible change, and the
1739+
// captured `s` must keep reading the renamed column rather than becoming ambiguous.
1740+
val longS = new String(Character.toChars(0x17f))
1741+
val t = "testcat.ns1.ns2.tbl"
1742+
withTable(t) {
1743+
sql(s"CREATE TABLE $t (id INT, s INT) USING foo")
1744+
1745+
// Analyze the plan but do not execute it: a Dataset memoizes its optimized plan, so a collect
1746+
// before the change would refresh once and never see the change below.
1747+
val df = spark.table(t).filter("id > 0")
1748+
assert(df.queryExecution.analyzed.resolved)
1749+
1750+
// Spark's own DDL refuses both changes because it checks for an existing field with the
1751+
// resolver, so apply them through the catalog the way another engine would. Data is written
1752+
// afterwards because this fixture migrates rows by exact field name and so drops the values
1753+
// of a renamed column.
1754+
val cat = catalog("testcat")
1755+
cat.alterTable(testIdent, TableChange.renameColumn(Array("s"), "S"))
1756+
cat.alterTable(testIdent, TableChange.addColumn(Array(longS), IntegerType, true))
1757+
externalAppend(cat, testIdent, InternalRow(2, 20, 99))
1758+
1759+
// Reading 99 instead of 20 would mean the captured name bound to the addition.
1760+
checkAnswer(df, Seq(Row(2, 20)))
1761+
assert(df.queryExecution.optimizedPlan.output.map(_.name) == Seq("id", "s"))
1762+
}
1763+
}
1764+
17291765
test("refresh reconciles a wider partially-pruned scan with stored temp view output") {
17301766
val t = "testcat.ns1.ns2.tbl"
17311767
withTable(t) {
@@ -1780,6 +1816,57 @@ class DataSourceV2DataFrameSuite
17801816
}
17811817
}
17821818

1819+
test("refresh recreates a captured nested schema without nested pruning or filter pushdown") {
1820+
val t = "testcat.ns1.ns2.tbl"
1821+
withTable(t) {
1822+
sql(s"CREATE TABLE $t (id INT, person STRUCT<name: STRING, city: STRING>) USING foo")
1823+
sql(s"INSERT INTO $t VALUES (1, named_struct('name', 'Alice', 'city', 'SF'))")
1824+
1825+
def nestedQuery(): DataFrame =
1826+
spark.table(t).where("person.name = 'Alice'").select("person.name")
1827+
1828+
def personFieldsRead(df: DataFrame): Seq[String] = {
1829+
val readSchema = scanRelationOf(df).scan.readSchema()
1830+
readSchema("person").dataType.asInstanceOf[StructType].fieldNames.toSeq
1831+
}
1832+
1833+
// Every filter this source reports is one Spark managed to translate and hand to
1834+
// `pushFilters`. The scan relation's own `pushedFilters` cannot be used here: it keeps only
1835+
// fully-pushed filters, and this source evaluates none on an unpartitioned table.
1836+
def filtersReachingSource(df: DataFrame): Seq[String] = {
1837+
scanRelationOf(df).scan match {
1838+
case scan: InMemoryBaseTable#InMemoryBatchScan =>
1839+
scan.pushedFilters.map(_.toString).toSeq
1840+
case other =>
1841+
fail(s"unexpected scan type ${other.getClass.getName}")
1842+
}
1843+
}
1844+
1845+
// capture the plan, then add a field inside the struct
1846+
val captured = nestedQuery()
1847+
assert(captured.queryExecution.analyzed.resolved)
1848+
sql(s"ALTER TABLE $t ADD COLUMN person.age INT FIRST")
1849+
sql(s"INSERT INTO $t VALUES (2, named_struct('age', 25, 'name', 'Bob', 'city', 'NY'))")
1850+
1851+
val fresh = nestedQuery()
1852+
checkAnswer(captured, Seq(Row("Alice")))
1853+
checkAnswer(fresh, Seq(Row("Alice")))
1854+
1855+
// a plan analyzed after the change prunes to the one field it reads and translates the
1856+
// predicate down to the source
1857+
assert(personFieldsRead(fresh) == Seq("name"))
1858+
assert(filtersReachingSource(fresh).nonEmpty)
1859+
1860+
// Rebinding rebuilds the struct as `If(IsNull(person), null, CreateNamedStruct(...))`, which
1861+
// the extraction, schema-pruning and pushdown rules cannot see through, so the captured plan
1862+
// reads the whole current struct and its predicate never reaches the source. Results stay
1863+
// correct, only wider than necessary. Tighten both assertions to match `fresh` once the
1864+
// projection uses an optimizer-friendly null-preserving form.
1865+
assert(personFieldsRead(captured) == Seq("age", "name", "city"))
1866+
assert(filtersReachingSource(captured).isEmpty)
1867+
}
1868+
}
1869+
17831870
test("refresh recreates a captured schema nested through a map and an array") {
17841871
val t = "testcat.ns1.ns2.tbl"
17851872
withTable(t) {
@@ -3988,9 +4075,13 @@ class DataSourceV2DataFrameSuite
39884075
assert(spark.sharedState.cacheManager.numCachedEntries == 1)
39894076

39904077
// Rebinding an already rebound plan adds a second projection rather than replacing the first,
3991-
// so this entry no longer matches the single projection a query rebuilds from its own
3992-
// captured output and stops being reused. Derived queries must still return the captured
3993-
// schema and the latest data.
4078+
// so the retained entry no longer matches the single projection a derived query rebuilds from
4079+
// its own captured output, and stops being reused. Assert that directly: the entry surviving
4080+
// is not the same claim as the entry being usable. Flip this back to `assertCached` once
4081+
// refresh replaces the generated projection instead of nesting inside it.
4082+
assertNotCached(df.filter("id > 0"))
4083+
4084+
// Derived queries must still return the captured schema and the latest data.
39944085
checkAnswer(df.filter("id > 0"), Seq(Row(1, 10), Row(2, 20), Row(3, 30), Row(4, 40)))
39954086

39964087
// verify latest schema is propagated again

0 commit comments

Comments
 (0)