Summary of Changes

Hello, I'm Gemini Code Assist¹! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request resolves a bug where SQL queries using count(*) in conjunction with other projected items (like literals or LET variables) against an empty data type would incorrectly yield no results. The fix ensures that such queries now consistently return a single row, correctly showing a count(*) of zero along with the evaluated values for the non-aggregate projections, thereby improving the robustness and predictability of aggregate query behavior.

Highlights

• SQL count(*) behavior on empty types: SQL queries combining count(*) with non-aggregate projections (like literals or LET variables) on empty types now correctly return a single row containing count(*)=0 and the evaluated non-aggregate values, resolving an issue where no results were returned.
• findCountStarItem() utility: A new helper method, findCountStarItem(), was introduced to accurately identify count(*) as the sole aggregate function within a projection, even when other non-aggregate pass-through items are present.
• GuaranteeEmptyCountStep enhancement: The GuaranteeEmptyCountStep was extended to properly evaluate and include preAggregateProjection items (such as literals and LET variables) in the synthetic result generated when a count(*) query on an empty type produces zero rows.

🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console.

Code Review

Good fix for a clear regression — the logic is sound and the approach of broadening the GuaranteeEmptyCountStep trigger condition is the right direction. A few things worth addressing:

Bug: isCountOnly() is now dead code

isCountOnly() (line 484 in SelectExecutionPlanner.java) was the only call site replaced by findCountStarItem(). It is now unreferenced and should be removed to avoid confusion and future maintenance burden.

Minor: Redundant null check + double call in findCountStarItem()

Current code calls getMathExpression() twice with an unnecessary null guard:

if (exp.getMathExpression() != null && exp.getMathExpression() instanceof final BaseExpression base
    && base.isCount() && base.getModifier() == null)

Since instanceof already returns false for null, simplify to:

if (exp.getMathExpression() instanceof final BaseExpression base
    && base.isCount() && base.getModifier() == null)

Missing test: non-empty type with mixed projections

The PR only tests the empty-type path. A test verifying SELECT count(*), 2 FROM non_empty_type (with actual records inserted) would guard against the pre-aggregate items being double-applied or overwritten in the non-empty path.

Minor: Test style inconsistency

The two new tests do not use the try-catch pattern used by the adjacent tests (countStarEmptyNoIndex, countStarEmptyNoIndexWithAlias). JUnit 5 makes try-catch unnecessary for exception propagation, so the newer style is actually cleaner — worth noting for a follow-up cleanup of the older tests.

Behaviour note (not a bug, worth a comment)

In GuaranteeEmptyCountStep.next():

preAggItem.execute((Result) null, context)

This works correctly for literals and LET/global variables (resolved via context), but a field reference like SELECT count(*), someField FROM empty_type would silently produce null for someField. That is the correct semantic for an empty result set, but a short inline comment explaining the null-Result contract would help future readers.

Summary

The core fix is correct and the regression tests cover the reported cases. The main actionable items are:

1. Remove the now-unused isCountOnly() method.
2. Simplify the redundant double getMathExpression() call.
3. Add a non-empty-type test for the mixed count(*) + literal projection.

Addressed the review feedback in c78fea6:

• Removed dead isCountOnly() — no callers remain after replacing it with findCountStarItem().
• Simplified getMathExpression() check — dropped the redundant != null guard since instanceof already handles null.
• Added non-empty-type test — countStarWithLiteralProjectionOnNonEmptyType verifies SELECT count(*), 2 FROM type_with_records returns {count(*): 5, 2: 2}.

Did not extract isCountStarExpression helper (Gemini suggestion) — the check is used exactly once in findCountStarItem(), so a single-use helper adds indirection without value.

Code Review

The fix is correct and the approach is sound. A few observations:

Overall assessment

The core fix is solid. findCountStarItem() correctly replaces the old isCountOnly() with broader semantics — it scans aggregateProjection for a single count(*) aggregate while ignoring non-aggregate pass-through items, and returns the item rather than a boolean so the caller can use it directly. The GuaranteeEmptyCountStep extension cleanly evaluates preAggregateProjection items to populate the synthetic zero-count result.

Minor: undocumented null-Result contract in GuaranteeEmptyCountStep

result.setProperty(preAggItem.getProjectionAliasAsString(), preAggItem.execute((Result) null, context));

This works correctly for literals and LET/context variables because their values are resolved through context regardless of the current record. For a field reference (e.g. SELECT count(*), someField FROM empty_type), it will silently return null — which is actually the correct SQL semantics for zero rows, but is non-obvious to a future reader. A short inline comment would help:

// null record is intentional: on empty results, literals/LET vars resolve via context;
// field references return null, which is correct SQL semantics for an empty set
result.setProperty(preAggItem.getProjectionAliasAsString(), preAggItem.execute((Result) null, context));

Test coverage

Three tests are added and they cover the key paths well:

• countStarWithLiteralProjectionOnEmptyType — the reported bug case ✓
• countStarWithLetVariableProjectionOnEmptyType — LET variable variant ✓
• countStarWithLiteralProjectionOnNonEmptyType — verifies the non-empty path is not broken ✓

One additional edge case worth considering: SELECT count(*), 2, 3 FROM empty_type (multiple non-aggregate projections). The loop in GuaranteeEmptyCountStep should handle it, but a quick test would confirm the alias iteration is exhaustive.

Logic trace for edge cases in findCountStarItem()

The guard logic is correct for all relevant combinations:

Summary

• One actionable item: add the inline comment explaining the null Result contract.
• Nice to have: test for multiple non-aggregate projections in the same query.
• The fix itself is production-ready.

Coverage summary from Codacy

See diff coverage on Codacy

See your quality gate settings    Change summary preferences

🧪 CI Insights

Here's what we observed from your CI run for c78fea6.

🟢 All jobs passed!

But CI Insights is watching 👀

Codecov Report

❌ Patch coverage is 82.60870% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 65.37%. Comparing base (2768254) to head (c78fea6).
⚠️ Report is 4 commits behind head on main.

@@            Coverage Diff             @@
##             main    #3587      +/-   ##
==========================================
- Coverage   65.71%   65.37%   -0.34%     
==========================================
  Files        1514     1514              
  Lines      103661   103672      +11     
  Branches    21454    21457       +3     
==========================================
- Hits        68118    67776     -342     
- Misses      26306    26660     +354     
+ Partials     9237     9236       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.