urosstan-db commented on PR #58762:
URL: https://github.com/apache/spark/pull/58762#issuecomment-5654070848
## Code review — Claude Opus (xhigh reasoning)
**Verdict: 0 blocking, 0 non-blocking, 0 nits.** A clean, well-scoped,
backward-compatible change with good test coverage.
### What the change does
Invalid JDBC `lowerBound`/`upperBound` bounds on a date/timestamp partition
column previously threw a plain `IllegalArgumentException` ("Cannot parse the
bound value X as timestamp") — no error condition, no SQLSTATE, and no
indication of which option was wrong. This PR reports them through a structured
condition `INVALID_JDBC_PARTITION_BOUND` (SQLSTATE `42616`, "Invalid options
specified"): `Cannot parse the value <value> for JDBC option <option> as
<dataType>.` A new factory
`QueryCompilationErrors.invalidJdbcPartitionBoundError` builds it, and
`JDBCRelation.toInternalBoundValue` now receives the option name so the failure
identifies which bound failed.
### Verification (static)
- `SparkIllegalArgumentException extends IllegalArgumentException`, so the
exception-type change stays source/binary-compatible for existing
`catch`/`intercept[IllegalArgumentException]` callers; only the message and the
added condition/SQLSTATE change.
- SQLSTATE `42616` is defined in `error-states.json` and is the code the
Avro/Protobuf option-validation errors use; the new condition is placed in
correct alphabetical order.
- The factory's `toDSOption`/`toSQLConfVal`/`toSQLType` each double-quote
their argument, matching the `checkError` parameters in both suites;
`JDBC_LOWER_BOUND`/`JDBC_UPPER_BOUND` equal `"lowerBound"`/`"upperBound"`.
- The error is raised inside a by-name `getOrElse`, so only on parse
failure; the success return is unchanged.
- Scope is intentionally limited to `DATE`/`TIMESTAMP`/`TIMESTAMP_NTZ`;
numeric partition columns keep their existing `value.toLong`
(`NumberFormatException`) path, unchanged.
This is a static review — the two affected suites were not executed here
(the description notes they weren't rerun after the SQLSTATE-only update), but
the code and test expectations agree on `42616` and the message parameters.
### Notes (non-blocking)
- Title is still `[WIP]` and the description says the PR is pending an
Apache Spark JIRA — worth resolving both before merge.
<sub>Automated review generated by Claude Opus at xhigh reasoning
effort.</sub>
--
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]
