perf(sql): share repeated computations in lateral join decorrelation

[kafka1991]

tandem: https://github.com/questdb/questdb-enterprise/pull/977

Summary

• Lateral join decorrelation duplicates the outer query model for each correlated reference. This PR replaces deep-cloning with lightweight QueryModelWrapper references that share the same underlying model, and introduces SharedRecordCursorFactory so the shared model executes only once at runtime.
• The wrapper/shared-cursor mechanism is designed as a general framework for materializing common table expressions — future CTE and view support can reuse it to eliminate duplicate execution across multiple references.
• Shared cursor support covers all GROUP BY factory variants: GroupByRecordCursorFactory, GroupByNotKeyedRecordCursorFactory, vectorized GroupByRecordCursorFactory / GroupByNotKeyedVectorRecordCursorFactory, AsyncGroupByRecordCursorFactory, AsyncGroupByNotKeyedRecordCursorFactory, and pass-through via SelectedRecordCursorFactory.

[kafka1991]

@kafka1991 — detailed review below.

@bluestreak01

Thanks for review!

All review findings addressed, including correctness fixes, resource cleanup consistency, defensive assertions, and vectorized GROUP BY test coverage. Ready for re-review.

[bluestreak01]

@kafka1991 — detailed review below.

---

PR #6941 Review: chore(sql): share repeated computations in lateral join decorrelation

Critical

1. Wrong index in extraOuterProjectionFunctions.set() — shared cursors get corrupted function lists

File: core/src/main/java/io/questdb/griffin/engine/groupby/GroupByUtils.java:303 and :330

Code at line 300 vs 303:

// Primary list — uses i (position in SELECT list) ✓
outerProjectionFunctions.set(i, createColumnFunction(baseMetadata, keyColumnIndex, type, index));
// Shared lists — uses index (column's metadata position) ✗
extraOuterProjectionFunctions.getQuick(d).set(index, createColumnFunction(baseMetadata, keyColumnIndex, type, index));

Both lists are populated by .add() in the first loop (line 130-134), so position i in each list holds the function for SELECT column i. When overwriting a column-key function with a map-column-reference, the primary list correctly uses i but the shared list uses index (the column's position in the base table metadata, from findColumnKeyIndex() at line 289).

Trace: For SELECT sum(val), key FROM t GROUP BY key where key is metadata column 0:

• i=1 (key is second in SELECT), index=0 (key is first in metadata)
• Primary: .set(1, colRef) — correctly overwrites key's position
• Shared: .set(0, colRef) — overwrites position 0 (the sum aggregate), leaving position 1 as the original parsed key function

Impact: The shared cursor returns the key value where the aggregate should be, and the original unparsed key function where the key column reference should be. This produces wrong query results for any keyed GROUP BY where a key column appears at a different SELECT position than its metadata position.

Why tests pass: All 24 LateralJoinSharedCursorTest tests place the key column first in the SELECT list, matching its metadata position (i == index), so the bug never manifests.

Fix: Change index to i on both line 303 and line 330:

extraOuterProjectionFunctions.getQuick(d).set(i, createColumnFunction(...));

Add a test with key column NOT first in SELECT:

SELECT AVG(amount) AS avg_amount, category FROM tab2 GROUP BY tab2.category

Moderate

2. supportsSharedCursors() returns true unconditionally, even when sharedRecordFunctions is null

Files:

• GroupByRecordCursorFactory.java:174
• GroupByNotKeyedRecordCursorFactory.java:145

Both factories accept @Nullable ObjList<ObjList<Function>> sharedRecordFunctions in the constructor but return true from supportsSharedCursors() regardless. The getSharedCursor() methods guard with assert sharedRecordFunctions != null which is a no-op without -ea. Currently unreachable because SqlCodeGenerator only routes shared refs to factories created with shared functions, but violates the supportsSharedCursors() contract.

Fix: return sharedRecordFunctions != null;

3. NotKeyedVectorSharedCursor missing getSymbolTable() / newSymbolTable() overrides

File: GroupByNotKeyedVectorRecordCursorFactory.java:430-483

The RostiSharedCursor (keyed vectorized, line 864-905) properly delegates getSymbolTable() and newSymbolTable() to the primary cursor. NotKeyedVectorSharedCursor does not, inheriting the default that throws UnsupportedOperationException. If a not-keyed vectorized GROUP BY includes a symbol-typed aggregate result and uses a shared cursor, this would crash.

Fix: Add overrides delegating to cursor.getSymbolTable(columnIndex) and cursor.newSymbolTable(columnIndex).

4. Implicit initialization ordering — keyed and vectorized factories assume getCursor() was called first

Files:

• GroupByRecordCursorFactory.getSharedCursor() (line 146) — cursor.managedCursor (needed by buildDataMap()) is only set in cursor.of() during getCursor()
• vect/GroupByRecordCursorFactory.getSharedCursor() (line 231) — frameCursor only set during getCursor()
• vect/GroupByNotKeyedVectorRecordCursorFactory.getSharedCursor() (line 137) — same issue

Only GroupByNotKeyedRecordCursorFactory.getSharedCursor() (line 124-127) proactively creates the base cursor if needed. The other variants rely on the lateral join's execution order (master getCursor() before slave getCursor()). Correct today but fragile — a future refactoring of join execution order or reuse of shared cursors in non-lateral contexts would silently NPE.

Fix: Add defensive initialization in keyed getSharedCursor() methods, mirroring the not-keyed pattern, or add an assertion documenting the contract.

5. PR title uses chore — should be perf

The PR replaces deep-cloning with shared execution to avoid duplicate computation. This is a performance optimization, not a maintenance chore. Per conventions, the title should be perf(sql): share repeated computations in lateral join decorrelation.

Minor

6. Most shared cursor tests lack plan assertions

File: LateralJoinSharedCursorTest.java

Only testAsyncKeyedGroupByOuter (line 61) asserts the query plan contains "(Shared)". The other 23 tests verify data correctness only. Without plan assertions, these tests pass even if the shared cursor optimization is not triggered.

7. Unsafe casts from IQueryModel to QueryModel guarded only by assert

File: LateralJoinRewriter.java at lines 525-526, 1149, 3014-3015

Three casts are guarded by assert which is a no-op in production. instanceof pattern variables would be more robust.

8. Typo in comment

File: LateralJoinRewriter.java:2667 — "Defensive programing" → "Defensive programming"

Downgraded (false positives)

• "SharedRecordCursorFactory leaks primaryFactory" — By design, non-owning reference. Primary is owned by the main factory tree node.
• "Race condition between shared and primary cursors" — All cursor operations execute on the same SQL execution thread, documented at vect/GroupByRecordCursorFactory.java:935-937.
• "Misc.clear(sharedCursors) doesn't close individual cursors" — Shared cursor close() implementations are no-ops or minimal. The factory already frees all owned resources before clearing the list.
• "allowsNestedColumnsChange() NPE" (previous review) — Fixed with nestedModel == null || guard at QueryModel.java:370.
• "Missing sharedCursors cleanup in _close()" (previous review) — Fixed. Both factories now include Misc.clear(sharedCursors).
• "hasSharedRefs()/getSharedRefs() inconsistency on wrapper" (previous review) — Fixed. QueryModelWrapper.hasSharedRefs() returns false, so getSharedRefs() is never called on wrappers.
• "Map cursor allocation in ShardedMapCursor.ofShared()" — One-time allocation cached via initCursor() on subsequent calls.

Summary

Verdict: Request changes.

Finding #1 (wrong index in extraOuterProjectionFunctions.set()) is a confirmed data correctness bug that produces wrong results from shared cursors when GROUP BY key columns appear at different positions in the SELECT list vs the base table metadata. The fix is a one-character change (index → i) on two lines, plus a regression test.

All three critical issues from the previous review are confirmed fixed.

21 findings reviewed, 7 false positives removed, 1 critical confirmed, 4 moderate, 3 minor.

[kafka1991]

All critical and moderate review items addressed. @bluestreak01

1. Wrong index in extraOuterProjectionFunctions.set() — shared cursors get corrupted function lists

Fixed. (index → i at GroupByUtils.java:303, 330) + regression test with key column not first in SELECT.

2. supportsSharedCursors() returns true unconditionally, even when sharedRecordFunctions is null

supportsSharedCursors() returning true was an implicit convention; changed to sharedRecordFunctions != null anyway, no harm done.

3. NotKeyedVectorSharedCursor missing getSymbolTable() / newSymbolTable() overrides

NotKeyedVectorSharedCursor doesn't need getSymbolTable() / newSymbolTable(): the primary cursor doesn't override them either, and not-keyed vectorized GROUP BY emits only numeric aggregates, no symbol columns.

4. Implicit initialization ordering — keyed and vectorized factories assume getCursor() was called first

Not needed. getSharedCursor() doesn't dereference any state set by getCursor() — it just stashes a reference to cursor.dataMap (final, non-null from construction) or flips isExhausted. Primary state (managedCursor / frameCursor) is only touched lazily inside hasNext() via primaryCursor.buildMapConditionally() / cursor.buildFunctionsConditionally().

[azure-pipelines]

Azure Pipelines:
Successfully started running 1 pipeline(s).

[azure-pipelines]

Azure Pipelines:
Successfully started running 1 pipeline(s).

[azure-pipelines]

Azure Pipelines:
Successfully started running 1 pipeline(s).

[mtopolnik]

[PR Coverage check]

😍 pass : 1676 / 2098 (79.89%)

file detail

	path	covered line	new line	coverage
🔵	io/questdb/griffin/model/QueryModelWrapper.java	26	268	09.70%
🔵	io/questdb/cairo/sql/RecordCursorFactory.java	1	2	50.00%
🔵	io/questdb/griffin/engine/table/AsyncGroupBySharedCursor.java	22	39	56.41%
🔵	io/questdb/griffin/engine/table/AsyncGroupByNotKeyedSharedCursor.java	17	28	60.71%
🔵	io/questdb/griffin/engine/groupby/GroupByUtils.java	18	28	64.29%
🔵	io/questdb/cairo/map/OrderedMap.java	6	9	66.67%
🔵	io/questdb/griffin/engine/groupby/vect/GroupByRecordCursorFactory.java	47	69	68.12%
🔵	io/questdb/griffin/engine/AbstractVirtualFunctionRecordCursor.java	3	4	75.00%
🔵	io/questdb/griffin/engine/table/AsyncGroupByRecordCursor.java	16	21	76.19%
🔵	io/questdb/griffin/engine/groupby/vect/GroupByNotKeyedVectorRecordCursorFactory.java	28	36	77.78%
🔵	io/questdb/griffin/engine/groupby/GroupByNotKeyedRecordCursorFactory.java	54	70	77.14%
🔵	io/questdb/griffin/model/IQueryModel.java	28	35	80.00%
🔵	io/questdb/griffin/engine/groupby/GroupByRecordCursorFactory.java	34	41	82.93%
🔵	io/questdb/griffin/SqlOptimiser.java	474	524	90.46%
🔵	io/questdb/griffin/engine/table/SelectedRecordCursorFactory.java	12	13	92.31%
🔵	io/questdb/griffin/model/QueryModel.java	219	234	93.59%
🔵	io/questdb/griffin/LateralJoinRewriter.java	143	148	96.62%
🔵	io/questdb/griffin/SqlParser.java	105	106	99.06%
🔵	io/questdb/griffin/engine/functions/groupby/StringAggVarcharGroupByFunction.java	7	7	100.00%
🔵	io/questdb/griffin/engine/functions/groupby/ApproxPercentileLongPackedGroupByFunction.java	6	6	100.00%
🔵	io/questdb/cairo/map/Unordered4Map.java	9	9	100.00%
🔵	io/questdb/griffin/engine/join/HashOuterJoinFilteredRecordCursorFactory.java	3	3	100.00%
🔵	io/questdb/griffin/engine/join/HashOuterJoinLightRecordCursorFactory.java	3	3	100.00%
🔵	io/questdb/griffin/engine/table/AsyncGroupByNotKeyedRecordCursor.java	3	3	100.00%
🔵	io/questdb/griffin/model/ExpressionNode.java	5	5	100.00%
🔵	io/questdb/griffin/engine/functions/groupby/ApproxPercentileDoubleGroupByFunction.java	7	7	100.00%
🔵	io/questdb/griffin/SqlCompilerImpl.java	21	21	100.00%
🔵	io/questdb/griffin/engine/table/FilterOnExcludedValuesRecordCursorFactory.java	2	2	100.00%
🔵	io/questdb/griffin/SqlCodeGenerator.java	117	117	100.00%
🔵	io/questdb/cairo/map/UnorderedVarcharMap.java	4	4	100.00%
🔵	io/questdb/griffin/engine/functions/groupby/CountDistinctVarcharGroupByFunction.java	6	6	100.00%
🔵	io/questdb/griffin/engine/table/AsyncGroupByNotKeyedRecordCursorFactory.java	15	15	100.00%
🔵	io/questdb/griffin/ExpressionTreeBuilder.java	1	1	100.00%
🔵	io/questdb/griffin/engine/functions/groupby/StringDistinctAggVarcharGroupByFunction.java	6	6	100.00%
🔵	io/questdb/griffin/engine/functions/groupby/CountDistinctStringGroupByFunction.java	6	6	100.00%
🔵	io/questdb/griffin/engine/groupby/SampleByInterpolateRecordCursorFactory.java	1	1	100.00%
🔵	io/questdb/cairo/map/Unordered8Map.java	9	9	100.00%
🔵	io/questdb/griffin/engine/functions/groupby/ApproxPercentileDoublePackedGroupByFunction.java	6	6	100.00%
🔵	io/questdb/griffin/engine/functions/GroupByFunction.java	2	2	100.00%
🔵	io/questdb/griffin/model/WithClauseModel.java	1	1	100.00%
🔵	io/questdb/griffin/engine/table/FilterOnValuesRecordCursorFactory.java	2	2	100.00%
🔵	io/questdb/griffin/engine/functions/groupby/StringAggGroupByFunction.java	7	7	100.00%
🔵	io/questdb/griffin/ExpressionParser.java	95	95	100.00%
🔵	io/questdb/griffin/engine/functions/groupby/StringDistinctAggSymbolGroupByFunction.java	6	6	100.00%
🔵	io/questdb/cairo/map/ShardedMapCursor.java	9	9	100.00%
🔵	io/questdb/griffin/engine/groupby/VirtualFunctionSkewedSymbolRecordCursor.java	1	1	100.00%
🔵	io/questdb/griffin/engine/table/AsyncGroupByRecordCursorFactory.java	15	15	100.00%
🔵	io/questdb/griffin/engine/join/SharedRecordCursorFactory.java	9	9	100.00%
🔵	io/questdb/griffin/engine/functions/groupby/ApproxPercentileLongGroupByFunction.java	6	6	100.00%
🔵	io/questdb/griffin/engine/join/HashOuterJoinFilteredLightRecordCursorFactory.java	4	4	100.00%
🔵	io/questdb/griffin/engine/join/HashOuterJoinRecordCursorFactory.java	6	6	100.00%
🔵	io/questdb/griffin/engine/functions/groupby/StringDistinctAggGroupByFunction.java	6	6	100.00%
🔵	io/questdb/griffin/SqlUtil.java	17	17	100.00%