dongjoon-hyun commented on PR #58333:
URL: https://github.com/apache/spark/pull/58333#issuecomment-5449639565
Nice cleanup — the naming rationale and the SQLSTATE choice both look right
to me. Two notes.
**`toSQLId(String)` vs `toSQLId(Seq(...))`**
`bucketCol` and the elements of `normalizedPartCols` are already-resolved,
top-level column names, but the `String` overload of `toSQLId` doesn't merely
quote them — it parses them first:
```scala
def toSQLId(parts: String): String = {
toSQLId(AttributeNameParser.parseAttributeName(parts))
}
```
Two consequences:
- a column literally named `a.b` renders as `` `a`.`b` ``, which reads like
a qualified/nested name rather than one column;
- a column name containing a backtick (e.g. ``a`b``) makes
`parseAttributeName` throw `INVALID_ATTRIBUTE_NAME_SYNTAX` *while the message
is being built*, so the user sees that instead of this error.
`normalizeCatalogTable` matches partition/bucket columns against schema field
names by plain resolution, so such names do reach these throw sites, and the
legacy templates didn't have this issue since they interpolated the raw string.
The `Seq` overload treats the name as a single part, and
`QuotingUtils.quoteIdentifier` escapes embedded backticks:
```scala
"bucketCol" -> toSQLId(Seq(bucketCol)),
"partitionColumns" -> normalizedPartCols.map(c =>
toSQLId(Seq(c))).mkString(", ")
```
The output is identical for the normal case, so the test assertions stay as
they are. This is also the established idiom for already-resolved column names
— `TableOutputResolver` uses `expected.map(col => toSQLId(Seq(col.name)))`, and
`ResolveDefaultColumnsUtil` uses `toSQLId(Seq(field.name))`.
**nit: parameter name.** `partitionColumns` is new to
`error-conditions.json`; `partitionColumnNames` is already used twice there and
lines up with `BucketSpec`'s `bucketColumnNames`/`sortColumnNames`. Either one
is a clear improvement over `normalizedPartCols` — just flagging it while the
name is still being settled.
The rest checks out on my side: the two entries are correctly ordered in the
JSON, no references to the legacy conditions remain, the docs are generated
from the JSON so nothing there needs updating, and `42713` matches
`STATIC_PARTITION_COLUMN_IN_INSERT_COLUMN_LIST`, which is the same shape.
--
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]
