dongjoon-hyun commented on code in PR #58333:
URL: https://github.com/apache/spark/pull/58333#discussion_r3875990432
##########
common/utils/src/main/resources/error/error-conditions.json:
##########
@@ -495,6 +495,18 @@
],
"sqlState" : "22001"
},
+ "BUCKET_COLUMN_IN_PARTITION_COLUMNS" : {
+ "message" : [
+ "Bucketing column '<bucketCol>' should not be part of partition columns
'<normalizedPartCols>'."
Review Comment:
Now that these graduate to named conditions, should the identifiers be
rendered via `toSQLId()` with bare placeholders (backtick quoting), per rule 3
in `QueryErrorsBase`'s scaladoc? Some recent graduations modernized the quoting
(e.g. SPARK-58155 added `toSQLId` for `RESERVED_DATABASE_NAME`), while others
kept the legacy text verbatim like this PR does — `CLUSTERING_COLUMNS_MISMATCH`
from this same throw site included. Keeping it word-for-word is defensible;
flagging it only because changing the style later means altering released
message text under the permanent names.
##########
common/utils/src/main/resources/error/error-conditions.json:
##########
@@ -495,6 +495,18 @@
],
"sqlState" : "22001"
},
+ "BUCKET_COLUMN_IN_PARTITION_COLUMNS" : {
+ "message" : [
+ "Bucketing column '<bucketCol>' should not be part of partition columns
'<normalizedPartCols>'."
+ ],
+ "sqlState" : "42601"
Review Comment:
The description's case for `42601` is well argued and matches the five
closest siblings. One counterpoint worth weighing before this becomes
permanent: `42713` reads in full "A duplicate object was detected in a list or
is the same as an existing object" — the second half covers a bucket column
being the same as a partition column, without requiring a duplicate within
either list, and `STATIC_PARTITION_COLUMN_IN_INSERT_COLUMN_LIST` /
`BIN_BY_DUPLICATE_DISTRIBUTE_COLUMN` use it for similar cross-list conflicts.
Fine either way given the in-file precedent; just making the alternative
explicit.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/errors/QueryCompilationErrors.scala:
##########
@@ -2230,7 +2230,7 @@ private[sql] object QueryCompilationErrors extends
QueryErrorsBase with Compilat
def bucketingColumnCannotBePartOfPartitionColumnsError(
bucketCol: String, normalizedPartCols: Seq[String]): Throwable = {
new AnalysisException(
- errorClass = "_LEGACY_ERROR_TEMP_1166",
+ errorClass = "BUCKET_COLUMN_IN_PARTITION_COLUMNS",
messageParameters = Map(
"bucketCol" -> bucketCol,
"normalizedPartCols" -> normalizedPartCols.mkString(", ")))
Review Comment:
Since the parameter keys now become part of the named condition's public
surface (generated docs, `getMessageParameters`), would `partitionColumns` read
better than the internal-sounding `normalizedPartCols`? It's a three-line
change at this point (two builders plus the test) and free before release,
whereas renaming after 4.x ships would break anything matching on the key.
Prior graduations went both ways on this, so feel free to keep as-is.
--
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]
