Skip to content

[SPARK-58252][SQL] Throw GROUP_BY_POS_OUT_OF_RANGE for out-of-range ordinal in pipe AGGREGATE GROUP BY#57423

Closed
LuciferYang wants to merge 1 commit into
apache:masterfrom
LuciferYang:pipe-groupby-ordinal-oob
Closed

[SPARK-58252][SQL] Throw GROUP_BY_POS_OUT_OF_RANGE for out-of-range ordinal in pipe AGGREGATE GROUP BY#57423
LuciferYang wants to merge 1 commit into
apache:masterfrom
LuciferYang:pipe-groupby-ordinal-oob

Conversation

@LuciferYang

Copy link
Copy Markdown
Contributor

What changes were proposed in this pull request?

An out-of-range GROUP BY ordinal in the SQL pipe AGGREGATE operator threw a raw java.lang.IndexOutOfBoundsException instead of a proper AnalysisException. For example:

select 3 as x, 4 as y
|> aggregate sum(y) group by 5;

resolvePipeAggregateExpressionOrdinal resolved the ordinal with an unguarded inputs(index - 1), so an out-of-range position (or 0) surfaced the internal JVM exception.

This PR adds the same bounds guard the regular GROUP BY ordinal resolver (resolveGroupByExpressionOrdinal) already uses, throwing GROUP_BY_POS_OUT_OF_RANGE (sqlState 42805). The UnresolvedPipeAggregateOrdinal node is now created with the ordinal literal's origin (via CurrentOrigin.withOrigin), so the error's queryContext points at the offending ordinal rather than the whole pipe statement, matching regular GROUP BY.

Why are the changes needed?

A raw IndexOutOfBoundsException escaping the analyzer for valid-syntax SQL is an internal error leak. User-facing errors should carry an error class, sqlState, and query context. The pipe AGGREGATE path was the only pipe operator with its own GROUP BY ordinal handling and the only one missing this guard; regular GROUP BY <ordinal> has thrown GROUP_BY_POS_OUT_OF_RANGE with precise context for years.

Does this PR introduce any user-facing change?

Yes, on the error path only. For an out-of-range or non-positive ordinal in pipe AGGREGATE GROUP BY:

Before:

java.lang.IndexOutOfBoundsException: Index 4 out of bounds for length 2

After:

[GROUP_BY_POS_OUT_OF_RANGE] GROUP BY position 5 is not in select list (valid range is [1, 2]). SQLSTATE: 42805

with a query context pointing at the ordinal. Valid (in-range) ordinals are unaffected.

How was this patch tested?

Added positive-range-adjacent negative cases to pipe-operators.sql (group by 5 above range, group by 0 non-positive) in the aggregation negative-tests section; regenerated the golden files. The regenerated goldens assert the error class, sqlState, index/size parameters, and the precise queryContext (fragment pointing at the ordinal). Existing positive pipe-ordinal cases (including the exact upper bound group by x, 2, 3) regenerated with no diff, confirming zero happy-path impact.

Was this patch authored or co-authored using generative AI tooling?

Generated-by: Claude Code (Opus 4.8)

…rdinal in pipe AGGREGATE GROUP BY

An out-of-range GROUP BY ordinal in the SQL pipe AGGREGATE operator
(e.g. `|> aggregate sum(y) group by 5`) leaked a raw
java.lang.IndexOutOfBoundsException from `inputs(index - 1)` instead of a
proper AnalysisException. This adds the same bounds guard the regular
GROUP BY ordinal resolver uses, throwing GROUP_BY_POS_OUT_OF_RANGE
(sqlState 42805). The ordinal node is also created with the literal's
origin so the error's query context points at the ordinal itself, matching
regular GROUP BY.

Generated-by: Claude Code (Opus 4.8)
@LuciferYang

Copy link
Copy Markdown
Contributor Author

cc @cloud-fan @dtenedor

@uros-b uros-b left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thank you @LuciferYang!

@dongjoon-hyun dongjoon-hyun left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1, LGTM.

This is much better although the new error message could be a little confusing in a way. Since it's inevitable when we reuse the existing error class, I agree with this PR.

dongjoon-hyun pushed a commit that referenced this pull request Jul 22, 2026
…rdinal in pipe AGGREGATE GROUP BY

### What changes were proposed in this pull request?

An out-of-range GROUP BY ordinal in the SQL pipe AGGREGATE operator threw a raw `java.lang.IndexOutOfBoundsException` instead of a proper `AnalysisException`. For example:

```sql
select 3 as x, 4 as y
|> aggregate sum(y) group by 5;
```

`resolvePipeAggregateExpressionOrdinal` resolved the ordinal with an unguarded `inputs(index - 1)`, so an out-of-range position (or `0`) surfaced the internal JVM exception.

This PR adds the same bounds guard the regular GROUP BY ordinal resolver (`resolveGroupByExpressionOrdinal`) already uses, throwing `GROUP_BY_POS_OUT_OF_RANGE` (sqlState `42805`). The `UnresolvedPipeAggregateOrdinal` node is now created with the ordinal literal's origin (via `CurrentOrigin.withOrigin`), so the error's `queryContext` points at the offending ordinal rather than the whole pipe statement, matching regular GROUP BY.

### Why are the changes needed?

A raw `IndexOutOfBoundsException` escaping the analyzer for valid-syntax SQL is an internal error leak. User-facing errors should carry an error class, sqlState, and query context. The pipe AGGREGATE path was the only pipe operator with its own GROUP BY ordinal handling and the only one missing this guard; regular `GROUP BY <ordinal>` has thrown `GROUP_BY_POS_OUT_OF_RANGE` with precise context for years.

### Does this PR introduce _any_ user-facing change?

Yes, on the error path only. For an out-of-range or non-positive ordinal in pipe AGGREGATE GROUP BY:

Before:
```
java.lang.IndexOutOfBoundsException: Index 4 out of bounds for length 2
```

After:
```
[GROUP_BY_POS_OUT_OF_RANGE] GROUP BY position 5 is not in select list (valid range is [1, 2]). SQLSTATE: 42805
```

with a query context pointing at the ordinal. Valid (in-range) ordinals are unaffected.

### How was this patch tested?

Added positive-range-adjacent negative cases to `pipe-operators.sql` (`group by 5` above range, `group by 0` non-positive) in the aggregation negative-tests section; regenerated the golden files. The regenerated goldens assert the error class, sqlState, `index`/`size` parameters, and the precise `queryContext` (fragment pointing at the ordinal). Existing positive pipe-ordinal cases (including the exact upper bound `group by x, 2, 3`) regenerated with no diff, confirming zero happy-path impact.

### Was this patch authored or co-authored using generative AI tooling?

Generated-by: Claude Code (Opus 4.8)

Closes #57423 from LuciferYang/pipe-groupby-ordinal-oob.

Authored-by: YangJie <yangjie01@baidu.com>
Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>
(cherry picked from commit 4462cb1)
Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>
@dongjoon-hyun

Copy link
Copy Markdown
Member

Merge Summary:

Posted by merge_spark_pr.py

@dongjoon-hyun

Copy link
Copy Markdown
Member

Merged for Apache Spark 4.3.0 whose Feature Freeze is scheduled on 1st August.

@LuciferYang

Copy link
Copy Markdown
Contributor Author

Thank you @dongjoon-hyun and @uros-b

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants