You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "vranes (via GitHub)" <gi...@apache.org> on 2026/06/22 17:40:39 UTC
[PR] [SPARK-57617][SQL] Clean up BIN BY error conditions and SQLSTATEs [spark]
vranes opened a new pull request, #56674:
URL: https://github.com/apache/spark/pull/56674
### What changes were proposed in this pull request?
This is a follow-up to the `BIN BY` relation operator (SPARK-57133) that cleans up its error conditions in `error-conditions.json` and the related analyzer code. Four groups of changes:
1. **SQLSTATE corrections** for conditions that carried placeholder states that did not match their semantics:
- `BIN_BY_ALIGN_TO_TYPE_MISMATCH`: `42804` -> `42K09`
- `BIN_BY_RANGE_TYPE_MISMATCH`: `42804` -> `42K09`
- `BIN_BY_DUPLICATE_DISTRIBUTE_COLUMN`: `42701` -> `42713`
- `BIN_BY_INVALID_ALIGN_TO`: `42K09` -> `42K08`
- `BIN_BY_REQUIRES_TOP_LEVEL_COLUMN`: `42K09` -> `0A000`
2. **Rename** `BIN_BY_DISTRIBUTE_TYPE_MISMATCH` -> `BIN_BY_INVALID_DISTRIBUTE_COLUMN_TYPE`. The check is an absolute required-type check with no second operand, so `*_TYPE_MISMATCH` was the wrong shape; this follows the `INVALID_<thing>_TYPE` convention (precedent: `INVALID_BUCKET_COLUMN_DATA_TYPE`). The genuine two-operand mismatches keep `*_TYPE_MISMATCH`.
3. **Split** the `BIN_BY_INVALID_BIN_WIDTH` catch-all, which fired at three sites (eval-failed, non-positive, wrong-type) under one SQLSTATE. SubClasses cannot carry per-subclass SQLSTATEs, so this splits into separate conditions mirroring the `ALIGN TO` structure:
- `BIN_BY_INVALID_BIN_WIDTH`: narrowed to constant-folding failure; message and SQLSTATE (`42K08`) now match `BIN_BY_INVALID_ALIGN_TO`.
- new `BIN_BY_NON_POSITIVE_BIN_WIDTH` (`42816`)
- new `BIN_BY_INVALID_BIN_WIDTH_TYPE` (`42K09`)
4. **Remove** `BIN_BY_COLUMN_NOT_FOUND`. A missing column now reports the standard `UNRESOLVED_COLUMN.WITH_SUGGESTION` (with column suggestions), matching every other operator. `BIN_BY_REQUIRES_TOP_LEVEL_COLUMN` still covers the nested-field case.
### Why are the changes needed?
The conditions were added with the operator in SPARK-57133 and some carried SQLSTATEs that do not describe the error (`42804` is "CASE result expressions incompatible"; `42701` is "same assignment target twice"). The `BIN WIDTH` catch-all reported three distinct failures under one state and message, and a missing column raised a bespoke condition where the standard `UNRESOLVED_COLUMN` already exists. This aligns the operator's error surface with the rest of Spark SQL before it ships.
### Does this PR introduce _any_ user-facing change?
Yes, but only within unreleased `master`. `BIN BY` was introduced on `master` (5.0.0), is gated off by default, and has not shipped in any released version. This changes the SQLSTATEs and condition names of analysis-time errors raised by `BIN BY`, and replaces `BIN_BY_COLUMN_NOT_FOUND` with `UNRESOLVED_COLUMN.WITH_SUGGESTION`. No released version is affected.
### How was this patch tested?
Updated and ran `ResolveBinBySuite` (bin-width split, distribute-type rename, missing-column now expects `UNRESOLVED_COLUMN.WITH_SUGGESTION`) and `SparkThrowableSuite`, which validates `error-conditions.json` (ordering, SQLSTATE invariants, no duplicates). Both suites pass.
### Was this patch authored or co-authored using generative AI tooling?
Generated-by: Claude Code (Claude Opus 4.8)
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-57617][SQL] Refine BIN BY error conditions and SQLSTATEs [spark]
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk commented on code in PR #56674:
URL: https://github.com/apache/spark/pull/56674#discussion_r3461020456
##########
common/utils/src/main/resources/error/error-conditions.json:
##########
@@ -411,13 +417,13 @@
"message" : [
"The range columns in BIN BY must be TIMESTAMP or TIMESTAMP_NTZ. Column <columnName> has type <columnType>. Both range columns must have the same type."
],
- "sqlState" : "42804"
+ "sqlState" : "42K09"
},
"BIN_BY_REQUIRES_TOP_LEVEL_COLUMN" : {
"message" : [
"BIN BY requires a top-level column, but <columnName> is a nested or computed field. Alias it to a top-level column in a select before BIN BY."
],
- "sqlState" : "42K09"
+ "sqlState" : "0A000"
Review Comment:
Optional / non-blocking consistency question. The existing `EVENT_TIME_MUST_BE_TOP_LEVEL_COLUMN` condition carries a near-identical message ("must be a top-level column, but `<...>` is a nested field ... alias it to a top-level column in a select before withWatermark") but uses SQLSTATE `42K09`. Here `BIN_BY_REQUIRES_TOP_LEVEL_COLUMN` adopts `0A000` ("feature not supported").
`0A000` is defensible and arguably more precise — the restriction is structural (nested/computed field not accepted), not a data-type mismatch. Just flagging the divergence from the established top-level-column peer in case matching it (`42K09`) is preferred for consistency. Either way is fine.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-57617][SQL] Refine BIN BY error conditions and SQLSTATEs [spark]
Posted by "vranes (via GitHub)" <gi...@apache.org>.
vranes commented on code in PR #56674:
URL: https://github.com/apache/spark/pull/56674#discussion_r3461208680
##########
common/utils/src/main/resources/error/error-conditions.json:
##########
@@ -411,13 +417,13 @@
"message" : [
"The range columns in BIN BY must be TIMESTAMP or TIMESTAMP_NTZ. Column <columnName> has type <columnType>. Both range columns must have the same type."
],
- "sqlState" : "42804"
+ "sqlState" : "42K09"
},
"BIN_BY_REQUIRES_TOP_LEVEL_COLUMN" : {
"message" : [
"BIN BY requires a top-level column, but <columnName> is a nested or computed field. Alias it to a top-level column in a select before BIN BY."
],
- "sqlState" : "42K09"
+ "sqlState" : "0A000"
Review Comment:
I originally put 42K09 because of EVENT_TIME_MUST_BE_TOP_LEVEL_COLUMN precedent but later I got feedback from Serge that it should be 0A000, and IMO we should prioritize accuracy over consistency in this case
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-57617][SQL] Refine BIN BY error conditions and SQLSTATEs [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan closed pull request #56674: [SPARK-57617][SQL] Refine BIN BY error conditions and SQLSTATEs
URL: https://github.com/apache/spark/pull/56674
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-57617][SQL] Refine BIN BY error conditions and SQLSTATEs [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on PR #56674:
URL: https://github.com/apache/spark/pull/56674#issuecomment-4781645612
thanks, merging to master/4.x!
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org