You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "SEPURI-SAI-KRISHNA (via GitHub)" <gi...@apache.org> on 2026/08/17 10:10:33 UTC
[PR] [SPARK-58821][SQL] Return the recomputed length instead of an internal error in Sequence [spark]
SEPURI-SAI-KRISHNA opened a new pull request, #58047:
URL: https://github.com/apache/spark/pull/58047
### What changes were proposed in this pull request?
`Sequence.sequenceLength` computes the sequence length in `Long` and falls back to `BigInt` when that arithmetic overflows. The fallback recomputes the length exactly, raises `COLLECTION_SIZE_LIMIT_EXCEEDED` if it is too large to allocate, and then throws `internalError("Unreachable code reached.")` on the assumption that no other outcome is possible.
That assumption does not hold. `Math.subtractExact(stop, start)` overflows whenever `stop - start` exceeds the `Long` range, which says nothing about the length, because the length also depends on `step`. For a large step the exact length is small, no limit error is raised, and control reaches the `internalError` with the correct length sitting unused in `safeLen`.
This PR returns `safeLen.toInt` instead of throwing. The check immediately above already bounds `safeLen` by `ByteArrayMethods.MAX_ROUNDED_ARRAY_LENGTH`, and the caller has already rejected boundaries whose step points the wrong way, so the value is positive and the narrowing conversion is exact. The now-unused `SparkException.internalError` import is removed.
No other behaviour changes: an overflow with a length that really is too large still raises `COLLECTION_SIZE_LIMIT_EXCEEDED.PARAMETER` from the same fallback.
### Why are the changes needed?
`sequence()` over BIGINT raises an `INTERNAL_ERROR` (SQLSTATE XX000) for queries that have a small, well-defined result. `INTERNAL_ERROR` is reserved for conditions that indicate a bug in Spark, so surfacing it for ordinary user input is wrong on both counts: the query should have succeeded, and the error tells the user to report a bug rather than fix their query.
```sql
SELECT sequence(-9223372036854775808L, 9223372036854775807L, 9223372036854775807L);
-- [INTERNAL_ERROR] Unreachable code reached. SQLSTATE: XX000
```
The element-filling code already handles steps of this magnitude correctly, so only the length computation was at fault: `sequence(-4611686018427387904L, 4611686018427387903L, 4611686018427387903L)`, whose endpoints are close enough together to avoid the overflow, returns `[-4611686018427387904, -1, 4611686018427387902]` on master today.
`sequenceLength` was introduced in this shape by SPARK-43393 (`afc4c49927cb`), which fixed a real correctness bug where the `Long` length computation overflowed silently and `sequence` returned an empty array. The overflow cases that fix was written against all had huge lengths, which is why the tail looked unreachable. The method has not changed since, so every release containing SPARK-43393 is affected.
### Does this PR introduce _any_ user-facing change?
Yes. Queries that failed with `INTERNAL_ERROR` now return their result. Both the interpreted and the codegen path are affected, since both call `Sequence.sequenceLength`.
Before:
```sql
SELECT sequence(-9223372036854775808L, 9223372036854775807L, 9223372036854775807L);
-- [INTERNAL_ERROR] Unreachable code reached. SQLSTATE: XX000
SELECT sequence(-9223372036854775808L, 0L, 4611686018427387904L);
-- [INTERNAL_ERROR] Unreachable code reached. SQLSTATE: XX000
```
After:
```sql
SELECT sequence(-9223372036854775808L, 9223372036854775807L, 9223372036854775807L);
-- [-9223372036854775808, -1, 9223372036854775806]
SELECT sequence(-9223372036854775808L, 0L, 4611686018427387904L);
-- [-9223372036854775808, -4611686018427387904, 0]
```
No query that previously succeeded changes its result, and no query that previously raised `COLLECTION_SIZE_LIMIT_EXCEEDED` stops raising it. Since the previous behaviour was an internal error, no migration guide entry is needed.
### How was this patch tested?
Three cases added to the `Sequence of numbers` test in `CollectionExpressionsSuite`, next to the existing SPARK-43393 overflow cases. `checkEvaluation` exercises both the interpreted and the codegen path:
* a positive step at both `Long` extremes,
* a positive step where only `stop - start` overflows,
* the negative-step mirror.
`build/sbt 'catalyst/testOnly *CollectionExpressionsSuite'` passes (62 tests). The existing SPARK-43393 cases, which cover the fallback still raising the limit error, pass unchanged.
Also verified end to end against a local `SparkSession`, including a non-foldable form reading the arguments from a temp view, to confirm the fix applies at runtime and not only through constant folding.
### Was this patch authored or co-authored using generative AI tooling?
Generated-by: Claude Code (Claude Opus 5)
--
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-58821][SQL] Return the recomputed length instead of an internal error in Sequence [spark]
Posted by "uros-b (via GitHub)" <gi...@apache.org>.
uros-b commented on PR #58047:
URL: https://github.com/apache/spark/pull/58047#issuecomment-5587050666
**Merge Summary:**
- merged into master https://github.com/apache/spark/commit/de60883859d4427544def5c78a47124b07d5c40f
- merged into branch-4.x https://github.com/apache/spark/commit/96fd566c362e4f92627b418801257763f69051d8
- merged into branch-4.3 https://github.com/apache/spark/commit/4116a7e449b84ac18641b5f52ea18d319706c205
- merged into branch-4.2 https://github.com/apache/spark/commit/3544f46159d652e3c6ec0f87302c393baa77a6d8
- merged into branch-4.1 https://github.com/apache/spark/commit/1094e6860d5efaa311560348de74c490ef6ed8a7
- merged into branch-4.0 https://github.com/apache/spark/commit/ce7f27c16f8380dc5cf4edf23513d1f3953c753e
*Posted by `merge_spark_pr.py`*
--
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-58821][SQL] Return the recomputed length instead of an internal error in Sequence [spark]
Posted by "SEPURI-SAI-KRISHNA (via GitHub)" <gi...@apache.org>.
SEPURI-SAI-KRISHNA commented on PR #58047:
URL: https://github.com/apache/spark/pull/58047#issuecomment-5599359674
Thank you @uros-b for the review and for taking this across all six branches. Asking about the affected versions was the right call, the trace put the origin at SPARK-43393 rather than 4.3.
--
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-58821][SQL] Return the recomputed length instead of an internal error in Sequence [spark]
Posted by "uros-b (via GitHub)" <gi...@apache.org>.
uros-b commented on PR #58047:
URL: https://github.com/apache/spark/pull/58047#issuecomment-5583324472
Code changes look good, thank you @SEPURI-SAI-KRISHNA for working on this! Can you just please clarify the affected Spark versions? Jira ticket (https://issues.apache.org/jira/browse/SPARK-58821) states `Affects Version/s` 4.3+, but I think that older versions might be affected as well - and that we should probably consider backports accordingly. Could you please check and verify?
--
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-58821][SQL] Return the recomputed length instead of an internal error in Sequence [spark]
Posted by "uros-b (via GitHub)" <gi...@apache.org>.
uros-b closed pull request #58047: [SPARK-58821][SQL] Return the recomputed length instead of an internal error in Sequence
URL: https://github.com/apache/spark/pull/58047
--
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-58821][SQL] Return the recomputed length instead of an internal error in Sequence [spark]
Posted by "SEPURI-SAI-KRISHNA (via GitHub)" <gi...@apache.org>.
SEPURI-SAI-KRISHNA commented on PR #58047:
URL: https://github.com/apache/spark/pull/58047#issuecomment-5584378663
@uros-b good catch, you are right that this is older than 4.3. I traced it.
The faulty path was introduced by SPARK-43393 ("Sequence expression can overflow", #41072), which replaced the old `getSequenceLength` helper with the current `sequenceLength` and its `try`/`catch`. That change was backported at the time, so its fix versions are 3.3.4, 3.4.2, 3.5.1 and 4.0.0. Checking the release tags matches exactly: v3.3.3 does not contain the unreachable throw and v3.3.4 does, v3.4.1 does not and v3.4.2 does, v3.5.0 does not and v3.5.1 does, and every 4.x release has it. Before that change the length came from `getSequenceLength`, which used `require` and had no `catch` block, so the path did not exist.
From then to master the body of `sequenceLength` is unchanged apart from cosmetics: the 3.x line raises `QueryExecutionErrors.unreachableError(...)` where 4.x raises `internalError("Unreachable code reached.")`, and `createArrayWithElementsExceedLimitError` later gained a `prettyName` parameter. The overflow path and the inputs that reach it are the same throughout, so every release since 3.3.4 returns the internal error for these endpoints.
I have updated `Affects Version/s` on SPARK-58821 to 3.5.8, 4.0.4, 4.1.3, 4.2.0 and 4.3.0, the latest released version of each maintained line. I left the 3.3 and 3.4 lines off as they are out of maintenance, but happy to add them if you would rather record the full range.
Happy to prepare backports for whichever branches you would like to take this to.
--
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-58821][SQL] Return the recomputed length instead of an internal error in Sequence [spark]
Posted by "uros-b (via GitHub)" <gi...@apache.org>.
uros-b commented on PR #58047:
URL: https://github.com/apache/spark/pull/58047#issuecomment-5587059577
Merged for Spark 5.0.0, 4.4.0, 4.3.0, 4.2.1, 4.1.4, and 4.0.5 - thank you @SEPURI-SAI-KRISHNA!
--
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-58821][SQL] Return the recomputed length instead of an internal error in Sequence [spark]
Posted by "uros-b (via GitHub)" <gi...@apache.org>.
uros-b commented on PR #58047:
URL: https://github.com/apache/spark/pull/58047#issuecomment-5315371172
LGTM, thank you @SEPURI-SAI-KRISHNA!
--
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-58821][SQL] Return the recomputed length instead of an internal error in Sequence [spark]
Posted by "SEPURI-SAI-KRISHNA (via GitHub)" <gi...@apache.org>.
SEPURI-SAI-KRISHNA commented on PR #58047:
URL: https://github.com/apache/spark/pull/58047#issuecomment-5559691807
@uros-b this has been approved since Aug 17. The one non-green check was `Build modules: sql - other tests`, which had been cancelled at the 2h30m job time limit rather than failing anything, every other job in that run passed.
I merged master to refresh the branch, and the run on d0fd31b is fully green: 29 passed, 2 skipped, nothing failed. That shard completed its tests in 1h36m this time, well inside the limit.
The code and test changes are unchanged from your review. Let me know if anything else is needed.
--
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