@puzpuzpuz — Code review findings (verified). Each item was double-checked against the code; false positives from the initial pass were eliminated.

Confirmed Bug

GROUP BY validation is overly permissive

SqlOptimiser.java:10106-10174 — validateHorizonJoinGroupBy loop 2

expressionContainsLiteral checks whether a GROUP BY column appears anywhere in a SELECT expression tree, rather than verifying that all leaf literals are covered by GROUP BY. This is weaker than the regular GROUP BY validation (rewriteGroupBySelectExpression / replaceIfGroupByExpressionOrAggregate), which traverses to leaf nodes and rejects any bare literal not in the GROUP BY list.

Example that incorrectly passes:

SELECT a + b, sum(c) FROM t HORIZON JOIN ... GROUP BY a

Passes because a appears in a + b, even though b is not in GROUP BY. The regular GROUP BY path correctly rejects the equivalent SELECT x+y FROM t GROUP BY x with "column must appear in GROUP BY clause or aggregate function".

The fix would be to replace the expressionContainsLiteral fallback (line 10172) with leaf-level literal coverage checking, matching the strictness of the regular GROUP BY path.

UX Issue

Error positions hardcoded to 0 in buildHorizonColumnMappings

SqlCodeGenerator.java:813, 826, 833, 839, 865

Five SqlException.$(0, ...) calls. Position 0 points at the start of the query, not the offending column. The method receives no position information from its call site. Threading position through (e.g., from column expression nodes in the QueryModel) would fix this.

Defensive / Code Quality

Missing closeSlaveOnFailure = false after HORIZON JOIN factory creation

SqlCodeGenerator.java:~4940

After generateHorizonJoinFactory takes ownership of both master and slave, closeSlaveOnFailure is not set to false. Not a live bug today (no throwing code between the factory call and the break), but it breaks the convention established by ASOF JOIN (line ~4448) and SPLICE JOIN (line ~4467). Should be added for consistency and to prevent future regressions if code is added between the factory call and the break.

"horizon" keyword is a backward compatibility break

SqlParser.java:5176, 5205

Adding "horizon" to tableAliasStop and joinStartSet means it can no longer be used as a table alias or unqualified table name in implicit cross joins. Consistent with how other join keywords work, but worth documenting as a breaking change.

Missing explicit SELECT_MODEL_HORIZON_JOIN case in generateSelect

SqlCodeGenerator.java:~6240

The generateSelect switch falls through to default, which works correctly but is implicit. Compare with SELECT_MODEL_WINDOW_JOIN which has its own explicit case. Adding case SELECT_MODEL_HORIZON_JOIN -> generateSubQuery(model, executionContext); would improve clarity.

Positive Observations

• Concurrency architecture is sound — per-worker isolation via PerWorkerLocks, proper memory fences through dispatch mechanism, correct happens-before relationships.
• The PR fixes a pre-existing bug: missing recordAt() calls after frame transitions in AsyncWindowJoinFastRecordCursorFactory (4 locations).
• No allocations on hot paths — records, maps, iterators are all pre-allocated and reused.
• Clean extraction of GroupByShardingContext and GroupByMapFragment faithfully reproduces the original logic.
• recordAt(int, long) optimization to avoid Rows.toRowID encoding/decoding is correct.

@puzpuzpuz — Code review findings (verified). Each item was double-checked against the code; false positives from the initial pass were eliminated.

@bluestreak01 thanks for the thorough review! Here's what was addressed in daa7827:

Confirmed Bug: GROUP BY validation is overly permissive

Fixed. Removed expressionContainsLiteral from both validation loops in validateHorizonJoinGroupBy. HORIZON JOIN GROUP BY now requires exact expression match (via compareExpressionsWithTablePrefixes) or alias/index match. Since HORIZON JOIN doesn't extract a virtual model, GROUP BY t.qty cannot cover SELECT t.qty + p.price — the user must write GROUP BY t.qty + p.price or use a column alias.

The expressionContainsLiteral method had no other callers and was removed entirely.

Existing tests that relied on the loose matching (e.g., GROUP BY h.offset matching SELECT h.offset / 1000000 AS sec_offs) were updated to use either the full expression or the alias. Two new tests were added for the partial expression coverage case.

UX Issue: Error positions hardcoded to 0

Fixed. Added a queryPosition parameter to buildHorizonColumnMappings, sourced from slaveModel.getJoinKeywordPosition() at the call site. All five SqlException.$(0, ...) calls now use this position. Notice that these exception are defensive checks hit only when the metadata was generated incorrectly.

Defensive: Missing closeSlaveOnFailure = false

Fixed. Added after the generateHorizonJoinFactory call in the JOIN_HORIZON case, consistent with ASOF/SPLICE JOIN convention.

Defensive: Missing explicit SELECT_MODEL_HORIZON_JOIN case in generateSelect

Not applied. The default branch correctly routes HORIZON JOIN models to generateJoins(model, executionContext), which is where JOIN_HORIZON processing happens. An explicit case SELECT_MODEL_HORIZON_JOIN -> generateSubQuery(...) would bypass generateJoins entirely, breaking HORIZON JOIN.

Defensive: "horizon" keyword backward compatibility

No code change needed — consistent with how other join keywords (asof, splice, lt, window) work. Will mention in the PR description.

[PR Coverage check]

😍 pass : 3102 / 3496 (88.73%)

file detail

@bluestreak01 @mtopolnik thanks for your reviews!