You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@shardingsphere.apache.org by "ym0506 (via GitHub)" <gi...@apache.org> on 2026/04/29 05:04:16 UTC
[PR] Fix PostgreSQL window aggregate merge on empty sharded table [shardingsphere]
ym0506 opened a new pull request, #38659:
URL: https://github.com/apache/shardingsphere/pull/38659
Fixes #38642.
## Summary
This PR fixes PostgreSQL window aggregate queries on empty sharded tables returning one `NULL` row instead of zero rows.
For a query like:
```sql
select pg_catalog.max(ref_0.c36) over (partition by ref_0.c39 order by ref_0.vkey desc) as c_5
from t24 as ref_0;
```
PostgreSQL should return zero rows when the input table is empty, because window functions produce one result per input row.
## Root Cause
PostgreSQL `OVER` clauses were not preserved on parsed aggregate projection segments. As a result, `max(...) over (...)` was bound as a normal aggregate projection, causing the sharding merge layer to use the aggregate merge path.
For empty routed results, `GroupByMemoryMergedResult` synthesizes one aggregate row, which is correct for plain aggregate queries like `select max(...) from t24`, but incorrect for window aggregate queries.
## Changes
- Preserve PostgreSQL `OVER` clauses on function and aggregate projection segments.
- Bind windowed aggregate projection segments as expression projections instead of aggregate projections.
- Add regression coverage for PostgreSQL window aggregate parsing.
- Add binder coverage for windowed aggregate projection classification.
- Add sharding merge coverage to ensure empty window aggregate results stay empty.
## Tests
```bash
./mvnw -pl parser/sql/engine/dialect/postgresql -am -DskipITs -Dtest=PostgreSQLStatementVisitorTest -Dsurefire.failIfNoSpecifiedTests=false test
./mvnw -pl infra/binder/core -am -DskipITs -Dtest=ProjectionEngineTest -Dsurefire.failIfNoSpecifiedTests=false test
./mvnw -pl features/sharding/core -am -DskipITs -Dtest=GroupByMemoryMergedResultTest -Dsurefire.failIfNoSpecifiedTests=false test
./mvnw -Pcheck -pl infra/binder/core,parser/sql/engine/dialect/postgresql,features/sharding/core -DskipTests -DskipITs spotless:check
./mvnw -Pcheck -pl infra/binder/core,parser/sql/engine/dialect/postgresql,features/sharding/core -DskipTests -DskipITs validate
```
--
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: notifications-unsubscribe@shardingsphere.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix PostgreSQL window aggregate merge on empty sharded table [shardingsphere]
Posted by "ym0506 (via GitHub)" <gi...@apache.org>.
ym0506 commented on PR #38659:
URL: https://github.com/apache/shardingsphere/pull/38659#issuecomment-4589966967
> ### Decision
> * **Merge Verdict: Not Mergeable**
> * **Reviewed Scope:** Latest PR head `c25fd46533c5f72343e5e00959611eeb87461e8f`; local merge-base `75474b1aa85f9f2e1ad72b2482fadb9dabb18253`; local triple-dot file list matched GitHub `/pulls/38659/files` exactly, 5/5 files. Reviewed `infra/binder/core/.../ProjectionEngine.java`, PostgreSQL visitor/test changes, and the sharding merge regression test. SQL parser target dialect is PostgreSQL; repo parser-family convention lists no PostgreSQL branch dialect. Shared binder blast-radius checked against MySQL as a non-target dialect that already attaches `WindowItemSegment` to aggregate functions. Official PostgreSQL docs checked: [window function calls](https://www.postgresql.org/docs/current/sql-expressions.html#SYNTAX-WINDOW-FUNCTIONS) and [select-list column labels](https://www.postgresql.org/docs/current/queries-select-lists.html#QUERIES-COLUMN-LABELS).
> * **Not Reviewed Scope:** I did not run local Maven verification or the Proxy Docker reproduction, and I did not inspect GitHub Actions/check-run status.
> * **Need Expert Review:** Parser/binder maintainer re-review is needed after the metadata regression below is fixed; no security, supply-chain, or concurrency-specific review is triggered by this patch.
>
> ### Positive Feedback
> * The root-cause direction is good: preserving PostgreSQL `OVER` metadata and preventing windowed aggregates from entering the normal aggregate merge path directly addresses the reported empty-table cardinality bug.
>
> ### Major Issues
> * **[Blocker] Preserve PostgreSQL column labels for unaliased windowed aggregates** (`infra/binder/core/src/main/java/org/apache/shardingsphere/infra/binder/context/segment/select/projection/engine/ProjectionEngine.java:148`)
>
> * **Symptom:** The PR converts any windowed `AggregationProjectionSegment` into an `ExpressionProjection`. That works for the reported aliased SQL, but for an adjacent valid SQL without `AS c_5`, the wrapped `ExpressionProjectionSegment` keeps an `AggregationProjectionSegment` as `expr`, not a `FunctionSegment`. PostgreSQL label extraction only returns a function name for `FunctionSegment`; otherwise it falls back to `?column?` at `infra/binder/dialect/postgresql/src/main/java/org/apache/shardingsphere/infra/binder/postgresql/PostgreSQLProjectionIdentifierExtractor.java:43`. Proxy query headers then use `projection.getColumnLabel()` at `proxy/backend/core/src/main/java/org/apache/shardingsphere/proxy/backend/response/header/query/QueryHeaderBuilderEngine.java:69`, and JDBC can use the projection label on derived/enhanced paths at `jdbc/src/main/java/org/apache/shardingsphere/driver/jdbc/core/resultset/ShardingSphereResultSetMetaData.java:93`.
> * **Risk:** `select pg_catalog.max(ref_0.c36) over (...) from t24` can now return the corrected row cardinality but expose `?column?` instead of PostgreSQL's function-derived default label such as `max`. PostgreSQL documents that unaliased function calls default to the function name, while window function calls preserve row cardinality. This is an adjacent no-alias counterexample missed by the current tests.
> * **Action:** Please keep the windowed-aggregate cardinality fix, but preserve PostgreSQL projection-label semantics for unaliased windowed aggregates. For example, make the PostgreSQL projection identifier path recognize `ExpressionProjectionSegment` whose `expr` is an `AggregationProjectionSegment`, or otherwise retain the pre-existing aggregate function label. Please add regression coverage for the no-alias window aggregate path, ideally through parser/binder plus Proxy/JDBC header or `ProjectionsContext` label behavior.
>
> ### Next Steps
> * Preserve the expected PostgreSQL column label for unaliased windowed aggregate projections, e.g. `max` rather than `?column?`.
> * Add a regression test for `select pg_catalog.max(ref_0.c36) over (partition by ref_0.c39 order by ref_0.vkey desc) from t24` without an alias.
> * Keep the current aliased empty-result regression, since it covers the reported issue path.
> * Re-run the scoped tests with dependent modules, for example `./mvnw -pl parser/sql/engine/dialect/postgresql,infra/binder/core,features/sharding/core -am -DskipITs -Dtest=PostgreSQLStatementVisitorTest,ProjectionEngineTest,GroupByMemoryMergedResultTest -Dsurefire.failIfNoSpecifiedTests=false test`, then run the repository Spotless/Checkstyle gates required for the touched modules.
Updated the PR to preserve PostgreSQL projection-label semantics for unaliased window aggregations.
The fix keeps the existing window aggregation cardinality behavior, and extends PostgreSQL expression column-name extraction to handle an `AggregationProjectionSegment` nested inside an `ExpressionProjectionSegment`. This reuses the existing aggregate function label path, so `pg_catalog.max(...) over (...)` now resolves to `max` instead of `?column?`.
Added a regression test through `ProjectionEngine` to cover the exact `AggregationProjectionSegment -> ExpressionProjection -> getColumnLabel()` path.
Verified with the targeted binder/sharding/parser tests, `spotless:check`, and `validate`.
--
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: notifications-unsubscribe@shardingsphere.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix PostgreSQL window aggregate merge on empty sharded table [shardingsphere]
Posted by "terrymanu (via GitHub)" <gi...@apache.org>.
terrymanu commented on PR #38659:
URL: https://github.com/apache/shardingsphere/pull/38659#issuecomment-4741634531
Hi @ym0506, thanks again for working on this issue and for the previous update.
The PostgreSQL fix direction looks reasonable, and the previous projection-label blocker has been addressed. However, this PR is still not ready to merge because the copied openGauss window-aggregate parser path remains unresolved, as described in the latest review. Since the shared binder behavior is affected, we need either the corresponding openGauss fix and regression coverage, or clear evidence that the openGauss path is not applicable.
This PR has had no response or update for a while after the latest change request. Could you please update it or share your thoughts on the openGauss counterpart? To keep the review queue clear, we may close this PR soon due to inactivity if there is still no response or update.
Thanks again for your contribution.
--
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: notifications-unsubscribe@shardingsphere.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix PostgreSQL window aggregate merge on empty sharded table [shardingsphere]
Posted by "ym0506 (via GitHub)" <gi...@apache.org>.
ym0506 commented on PR #38659:
URL: https://github.com/apache/shardingsphere/pull/38659#issuecomment-4794676251
The latest rerun reached a different failure mode from the original openGauss pipeline failure.
The current failed job is attempt 2, job `83199441131`, and it did not run the openGauss pipeline test. It failed in `Download Source Snapshot` before restore/setup/test execution:
```text
Unable to download artifact(s): Artifact not found for name: source-snapshot
```
The original `E2E - Operation` run was created on 2026-06-19, while this rerun happened on 2026-06-24. The workflow uploads `source-snapshot` with `retention-days: 3`, so rerunning only the failed matrix job after the artifact expired cannot download the source snapshot. The actual `Run pipeline on opengauss/opengauss:3.1.0` step was skipped in this latest attempt.
Could the full `E2E - Operation` workflow/run be rerun, or fresh CI be triggered for this PR, so `Prepare E2E Artifacts` uploads a new `source-snapshot` first?
--
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: notifications-unsubscribe@shardingsphere.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix PostgreSQL window aggregate merge on empty sharded table [shardingsphere]
Posted by "ym0506 (via GitHub)" <gi...@apache.org>.
ym0506 commented on PR #38659:
URL: https://github.com/apache/shardingsphere/pull/38659#issuecomment-4747334288
Implemented the openGauss counterpart in `f7272b5d4`.
This update preserves `OVER` metadata in `OpenGaussStatementVisitor#visitFuncExpr` by applying the same window append path used by PostgreSQL for `AggregationProjectionSegment`, `AggregationDistinctProjectionSegment`, and `FunctionSegment`. It also reuses openGauss window specification handling for both `OVER (...)` and `WINDOW ... AS (...)`.
Added openGauss regression coverage for:
- parsing `pg_catalog.max(...) over (...)` as a windowed `AggregationProjectionSegment` with the full expression and window metadata;
- openGauss binder label extraction through `AggregationProjectionSegment -> ProjectionEngine -> ExpressionProjection#getColumnLabel()`, verifying the unaliased label resolves to `max`.
I also removed unnecessary Mockito usage from the touched binder tests where concrete segment instances are sufficient.
Verified locally with:
- parser/binder scoped regression test: BUILD SUCCESS
- `spotless:check`: BUILD SUCCESS
- `validate`: BUILD SUCCESS, Checkstyle violations: 0
GitHub CI is running for the new head; Required - Check Spotless/CheckStyle/License are already passing at the time of this update.
--
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: notifications-unsubscribe@shardingsphere.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix PostgreSQL window aggregate merge on empty sharded table [shardingsphere]
Posted by "terrymanu (via GitHub)" <gi...@apache.org>.
terrymanu merged PR #38659:
URL: https://github.com/apache/shardingsphere/pull/38659
--
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: notifications-unsubscribe@shardingsphere.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix PostgreSQL window aggregate merge on empty sharded table [shardingsphere]
Posted by "ym0506 (via GitHub)" <gi...@apache.org>.
ym0506 commented on PR #38659:
URL: https://github.com/apache/shardingsphere/pull/38659#issuecomment-4589968484
Updated the PR to preserve PostgreSQL projection-label semantics for unaliased window aggregations.
The fix keeps the existing window aggregation cardinality behavior, and extends PostgreSQL expression column-name extraction to handle an `AggregationProjectionSegment` nested inside an `ExpressionProjectionSegment`. This reuses the existing aggregate function label path, so `pg_catalog.max(...) over (...)` now resolves to `max` instead of `?column?`.
Added a regression test through `ProjectionEngine` to cover the exact `AggregationProjectionSegment -> ExpressionProjection -> getColumnLabel()` path.
Verified with the targeted binder/sharding/parser tests, `spotless:check`, and `validate`.
--
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: notifications-unsubscribe@shardingsphere.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix PostgreSQL window aggregate merge on empty sharded table [shardingsphere]
Posted by "ym0506 (via GitHub)" <gi...@apache.org>.
ym0506 commented on PR #38659:
URL: https://github.com/apache/shardingsphere/pull/38659#issuecomment-4742218167
> Hi @ym0506, thanks again for working on this issue and for the previous update.
>
>
>
> The PostgreSQL fix direction looks reasonable, and the previous projection-label blocker has been addressed. However, this PR is still not ready to merge because the copied openGauss window-aggregate parser path remains unresolved, as described in the latest review. Since the shared binder behavior is affected, we need either the corresponding openGauss fix and regression coverage, or clear evidence that the openGauss path is not applicable.
>
>
>
> This PR has had no response or update for a while after the latest change request. Could you please update it or share your thoughts on the openGauss counterpart? To keep the review queue clear, we may close this PR soon due to inactivity if there is still no response or update.
>
>
>
> Thanks again for your contribution.
Thank you. Let me check again!
--
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: notifications-unsubscribe@shardingsphere.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix PostgreSQL window aggregate merge on empty sharded table [shardingsphere]
Posted by "ym0506 (via GitHub)" <gi...@apache.org>.
ym0506 commented on PR #38659:
URL: https://github.com/apache/shardingsphere/pull/38659#issuecomment-4747526957
All checks have completed except `E2E - pipeline on opengauss/opengauss:3.1.0`.
I inspected the failed job log. The failure is in `PostgreSQLMigrationGeneralE2EIT.assertMigrationSuccess`, during the second `t_order_item` migration, where the job stays in `PREPARING` and does not reach `EXECUTE_INCREMENTAL_TASK`:
```text
IllegalStateException: Job status not reached: EXECUTE_INCREMENTAL_TASK
```
The repeated job status record contains this openGauss pipeline error:
```text
PipelineInternalException: org.opengauss.util.PSQLException: ERROR: canceling statement due to user request
```
The other operation pipeline jobs for `mysql:5.7`, `mariadb:11`, and `postgres:12-alpine` passed, and the PR changes are limited to the PostgreSQL/openGauss parser and binder projection paths rather than `kernel/data-pipeline` or `test/e2e/operation/pipeline`.
I tried to rerun the failed workflow job, but GitHub rejected it with:
```text
run 27798590663 cannot be rerun; Must have admin rights to Repository.
```
Could a maintainer rerun the failed openGauss pipeline job?
--
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: notifications-unsubscribe@shardingsphere.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org