terrymanu commented on PR #38942: URL: https://github.com/apache/shardingsphere/pull/38942#issuecomment-4826438136
### Summary **Review Result: Mergeable** Reason: The latest PR head fixes the linked empty-result cardinality bug at the correct propagation point: PostgreSQL/openGauss parser now preserves `OVER` metadata, binder classifies windowed aggregates as expression projections instead of normal aggregation projections, and the sharding merge empty-result fallback is no longer triggered for the window-aggregate case. ### Evidence - Linked issue #38642 reports `max(...) OVER (...)` on an empty PostgreSQL sharded table returning one `NULL` row instead of zero rows. The failure path is closed because `ProjectionEngine` now maps `AggregationProjectionSegment` and `AggregationDistinctProjectionSegment` with `window` metadata to `ExpressionProjection`, so they are excluded from `ProjectionsContext#getAggregationProjections()` and from the `GroupByMemoryMergedResult` empty aggregate-row fallback. - PostgreSQL parser coverage is aligned with PostgreSQL official semantics: PostgreSQL documents that ordinary aggregates act as window functions only when an `OVER` clause follows the call. See [PostgreSQL window functions](https://www.postgresql.org/docs/current/functions-window.html) and [PostgreSQL tutorial window functions](https://www.postgresql.org/docs/current/tutorial-window.html). - The copied/related openGauss visitor path is also handled in the latest commit. openGauss official docs document window functions used with `OVER`, and the PR adds openGauss parser and binder regression coverage. See [openGauss window functions](https://docs.opengauss.org/en/docs/3.1.0/docs/Developerguide/window-functions.html). - `PostgreSQLStatementVisitor` and `OpenGaussStatementVisitor` preserve `OVER` metadata for `AggregationProjectionSegment`, `AggregationDistinctProjectionSegment`, and `FunctionSegment`, while keeping named/inline window item handling in the visitor. - `PostgreSQLProjectionIdentifierExtractor` preserves PostgreSQL projection-label behavior for windowed aggregate expression projections, so unaliased `pg_catalog.max(...) over (...)` resolves through the aggregate function label path instead of falling back to `?column?`. - Regression tests cover the root path: - parser preserves windowed aggregate projection metadata for PostgreSQL and openGauss; - binder creates `ExpressionProjection` for windowed aggregate projections; - PostgreSQL/openGauss projection-label extraction returns `max`; - sharding merge returns no rows for empty window-aggregate results while ordinary aggregate/group-by empty-result behavior remains covered. ### Review Details Reviewed Scope: Latest PR head `f7272b5d4e35e2a53dc5cc23de217cca0041fcaa`; local merge-base `75474b1aa85f9f2e1ad72b2482fadb9dabb18253`; local triple-dot file list matched GitHub `/pulls/38659/files` exactly, 10/10 files. Reviewed all changed files in parser, binder, and sharding merge tests. Target dialects checked: PostgreSQL and openGauss. Related parser-family note: PostgreSQL has no branch dialect in the repository convention, and openGauss is a separate dialect, but its copied/similar visitor path was checked because this PR changes shared binder behavior and prior review requested that counterpart. Not Reviewed Scope: Broader distributed correctness of non-empty cross-shard window-function execution is outside this PR's linked issue scope. This review is limited to the reported empty-result cardinality bug and the parser/binder propagation needed for that bug. Verification: Ran focused latest-head Maven verification with exit code `0`: `./mvnw -pl parser/sql/engine/dialect/postgresql,parser/sql/engine/dialect/opengauss,infra/binder/core,infra/binder/dialect/postgresql,infra/binder/dialect/opengauss,features/sharding/core -am -T1C -DskipITs -Dtest=PostgreSQLStatementVisitorTest,OpenGaussStatementVisitorTest,ProjectionEngineTest,PostgreSQLProjectionIdentifierExtractorTest,OpenGaussProjectionIdentifierExtractorTest,GroupByMemoryMergedResultTest -Dsurefire.failIfNoSpecifiedTests=false test`. The run completed with `BUILD SUCCESS`; the targeted test classes reported 0 failures and 0 errors. GitHub check-runs at review time had no completed failures; 76 completed checks were successful and 3 E2E operation matrix jobs were still in progress. Release Note / User Docs: Not required. This is an internal correctness fix restoring PostgreSQL/openGauss window-aggregate projection handling and sharding merge cardinality behavior; it does not introduce a new user-facing configuration key, SQL syntax, migration step, or documented feature surface. -- 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]
