Skip to content

Fix OLAP window function SQL generation when preceded by groupBy with computed expressions#4362

Closed
peledni wants to merge 1 commit intofinos:masterfrom
peledni:fix/olap-groupby-computed-expression-isolation
Closed

Fix OLAP window function SQL generation when preceded by groupBy with computed expressions#4362
peledni wants to merge 1 commit intofinos:masterfrom
peledni:fix/olap-groupby-computed-expression-isolation

Conversation

@peledni
Copy link
Copy Markdown

@peledni peledni commented Jan 17, 2026

Summary

  • Fixes invalid SQL generation when using groupBy with computed expressions (e.g., monthNumber(), year()) followed by olapGroupBy
  • The PARTITION BY clause was referencing raw expressions instead of column aliases, causing "Column must be in GROUP BY list" errors
  • Extended subselect isolation logic to wrap GROUP BY queries when followed by window functions

Problem

When using groupBy with computed expressions followed by olapGroupBy, the generated SQL was invalid:

Trade.all()
   ->project([col(t|$t.date->monthNumber(), 'month'), col(t|$t.quantity, 'quantity')])
   ->groupBy(['month'], agg('totalQty', x|$x.getFloat('quantity'), y|$y->sum()))
   ->olapGroupBy(['month'], func('totalQty', y|$y->count()), 'monthCount')

Before (invalid SQL):

SELECT extract(month from "root".tradeDate) as "month", sum("root".quantity) as "totalQty", 
       count(sum("root".quantity)) over (partition by extract(month from "root".tradeDate)) as "monthCount" 
FROM tradeTable as "root" 
GROUP BY "month"

After (valid SQL):

SELECT "subselect"."month" as "month", "subselect"."totalQty" as "totalQty", 
       count("subselect"."totalQty") over (partition by "subselect"."month") as "monthCount" 
FROM (SELECT extract(month from "root".tradeDate) as "month", sum("root".quantity) as "totalQty" 
      FROM tradeTable as "root" GROUP BY "month") as "subselect"

Test plan

  • Added new test case testGroupByWithComputedExpressionFollowedByOlapGroupBy
  • Updated existing test testExecutionPlanGeneration to reflect new expected SQL structure
  • Verified SQL generation produces valid syntax for PostgreSQL and H2

@peledni peledni requested a review from a team as a code owner January 17, 2026 22:19
@linux-foundation-easycla
Copy link
Copy Markdown

linux-foundation-easycla Bot commented Jan 17, 2026

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: peledni / name: Nir Peled (0022ca3)

@peledni peledni force-pushed the fix/olap-groupby-computed-expression-isolation branch from f0d8d34 to 95eb6c0 Compare January 17, 2026 22:24
gs-ssh16
gs-ssh16 previously approved these changes Jan 23, 2026
@github-actions
Copy link
Copy Markdown

github-actions Bot commented Jan 23, 2026

Test Results

  1 057 files   -      18    1 057 suites   - 18   2h 54m 53s ⏱️ - 1h 0m 27s
13 592 tests  -      80  13 423 ✔️  -      80  169 💤 ±0  0 ±0 
29 994 runs   - 2 727  29 825 ✔️  - 2 727  169 💤 ±0  0 ±0 

Results for commit 0022ca3. ± Comparison against base commit 4955997.

♻️ This comment has been updated with latest results.

… computed expressions

When using groupBy with computed expressions (e.g., monthNumber(), year()) followed by
olapGroupBy, the generated SQL was invalid because the PARTITION BY clause referenced
the raw computed expression instead of the column alias from the GROUP BY.

This caused "Column must be in GROUP BY list" errors in databases like Postgres.

The fix extends the existing subselect isolation logic in processTdsWindowColumn to also
wrap GROUP BY queries in a subselect when the window's partition/order columns reference
computed expressions (non-TableAliasColumn elements). Simple column references are left
unchanged to avoid unnecessary nesting.
@peledni peledni force-pushed the fix/olap-groupby-computed-expression-isolation branch from 95eb6c0 to 0022ca3 Compare January 26, 2026 13:24
@peledni peledni requested a review from gs-ssh16 January 26, 2026 20:13
@finos-admin
Copy link
Copy Markdown
Member

This PR is stale because it has been open for 30 days with no activity. Please remove stale label or add any comment to keep this open. Otherwise this will be closed in 5 days.

@finos-admin
Copy link
Copy Markdown
Member

This PR was closed because it has been inactive for 35 days. Please re-open if this PR is still relevant.

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants