fix: ORDER BY CASE WHEN expression fails with NPE on baseExpr.identifier

[robfrank]

Summary

Fixes #3777

• Added null check for baseExpr.identifier in SQLASTBuilder.visitOrderByItem - CASE WHEN produces a BaseExpression with identifier = null, causing NPE on .suffix access
• Added expression field to OrderByItem to store complex expressions (CASE WHEN, etc.)
• Updated OrderByItem.compare() to evaluate the expression directly for SELECT * queries
• Updated SelectExecutionPlanner.calculateAdditionalOrderByProjections() to inject a computed projection for SELECT fields queries

Test plan

• orderByCaseWhenExpression - inline CASE WHEN in ORDER BY ASC
• orderByCaseWhenExpressionDesc - inline CASE WHEN in ORDER BY DESC
• orderByCaseWhenWithSelectStar - CASE WHEN in ORDER BY with SELECT *
• orderByExtendedCaseWhenExpression - extended CASE expr WHEN val THEN result in ORDER BY
• All 13 SQLCaseTest tests pass, no regressions in ORDER BY related tests

🤖 Generated with Claude Code

[gemini-code-assist]

Code Review

This pull request fixes a NullPointerException occurring when complex expressions, such as CASE WHEN, are used in an ORDER BY clause. The changes involve updating SQLASTBuilder to store the Expression object directly within OrderByItem instead of relying on identifier extraction or string-based aliases. Additionally, SelectExecutionPlanner and OrderByItem were updated to handle the evaluation and projection of these expressions. Regression tests have been added to SQLCaseTest to verify the fix. I have no feedback to provide.

[claude]

Code Review

Overall, this is a clean and well-targeted fix for the NPE issue. The approach - storing the Expression object directly on OrderByItem instead of serializing it to a string alias - is the right design. Here are my observations:

What works well

• The root cause fix in SQLASTBuilder.visitOrderByItem is correct. Adding else if (baseExpr.identifier != null) guards the NPE cleanly.
• The two execution paths are handled correctly: SELECT * uses direct expression evaluation in compare(), while SELECT fields uses calculateAdditionalOrderByProjections() to inject a computed projection.
• copy(), extractSubQueries(), and toString() are all properly updated - no methods were missed.
• The early return guard in addOrderByProjections() (line 788-791) correctly skips calculateAdditionalOrderByProjections for SELECT *, so the SELECT * test case relies on the compare() path as intended.
• 4 regression tests covering ASC, DESC, SELECT *, and extended CASE forms.

Issues

1. refersToParent() not updated

The method currently checks only alias and modifier, but does not check expression:

public boolean refersToParent() {
  if (alias != null && alias.equalsIgnoreCase("$parent"))
    return true;
  return modifier != null && modifier.refersToParent();
}

If a CASE WHEN expression in ORDER BY references $parent (e.g. in a subquery context), this would return a false negative. Should add:

if (expression != null && expression.refersToParent())
  return true;

2. Inconsistent field access pattern

The new expression field is public and accessed directly (item.expression = null, item.expression != null) in SelectExecutionPlanner. The alias field has getAlias()/setAlias() and modifier has getModifier()/setModifier(), but expression has no accessor methods. This is inconsistent. Even though modifier and recordAttr are also public, adding getters/setters for expression would be more consistent with how alias is handled.

3. getName() returns null when expression != null

public String getName() {
  return alias != null ? alias : recordAttr != null ? recordAttr : null;
}

This returns null when only expression is set. This is used in calculateAdditionalOrderByProjections at:

if (!allAliases.contains(item.getName()))

allAliases.contains(null) evaluates to false in Java, so it works correctly - but silently relies on null semantics. A comment or an explicit null-safe check would make the intent clearer for future readers.

4. The else { continue; } in calculateAdditionalOrderByProjections

The new branch:

} else {
  continue;
}

This silently skips an ORDER BY item that has no alias, no expression, and no recordAttr. Previously this case couldn't occur (the expression was serialized to an alias string). With the new code it still shouldn't occur, but if it does (e.g. a bug elsewhere), the ORDER BY item would be silently ignored. A defensive log warning here would help debugging.

Minor

• The test comment // 'active' (Eve) should come before 'inactive' (Frank) is helpful - the data setup in @BeforeEach should be checked to confirm these names/statuses match the fixture. If the test data setup ever changes, the assertion containsExactly("Eve", "Frank") would break with an opaque failure.
• In toString(), the modifier is not appended when expression != null. This is probably fine since these are mutually exclusive in practice, but worth a comment.

Summary

The fix is correct and the test coverage is solid. The main actionable items before merge are:

1. Update refersToParent() to check expression - this is a correctness issue for subquery scenarios.
2. Consider adding a getter/setter for expression for consistency (or at minimum document why public field access is intentional here).

[codacy-production]

Up to standards ✅

🟢 Issues 0 issues

Results:
0 new issues

View in Codacy

🟢 Metrics 6 complexity

Metric	Results
Complexity	6

View in Codacy

🟢 Coverage 100.00% diff coverage · -7.87% coverage variation

Metric	Results
Coverage variation	✅ -7.87% coverage variation
Diff coverage	✅ 100.00% diff coverage

View coverage diff in Codacy

Coverage variation details

	Coverable lines	Covered lines	Coverage
Common ancestor commit (6964490)	116313	85427	73.45%
Head commit (ce5a052)	147469 (+31156)	96700 (+11273)	65.57% (-7.87%)

Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: <coverage of head commit> - <coverage of common ancestor commit>

Diff coverage details

	Coverable lines	Covered lines	Diff coverage
Pull request (#3778)	6	6	100.00%

Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: <covered lines added or modified>/<coverable lines added or modified> * 100%

_{TIP This summary will be updated as you push new changes. Give us feedback}

[claude]

Code Review (follow-up correction)

I need to correct an error in my previous review comment: refersToParent() is properly updated in this PR (I misread the diff). The method correctly adds:

if (expression != null && expression.refersToParent())
  return true;

Accurate assessment of remaining points

expression field visibility - The field is public with no getter/setter, accessed directly as item.expression in SelectExecutionPlanner. This is inconsistent with alias (which has getAlias()/setAlias()), though consistent with modifier and recordAttr which are also public. Worth a follow-up for consistency, but not a blocker.

Mutation in calculateAdditionalOrderByProjections - After creating a projection for a complex expression, the code sets item.expression = null. This relies on the assumption that this method is called at most once per OrderByItem. Since SelectExecutionPlanner creates a fresh plan each time, this is safe in practice, but worth a note.

else { continue; } silent skip - The new fallthrough branch silently ignores an OrderByItem with no alias, no expression, and no recordAttr. This state shouldn't be reachable with the current code, but a defensive log at WARN/FINE level would help future debugging. Minor concern.

getName() null semantics - The existing comment in the code (// Returns null when only expression is set; callers that use List.contains(getName()) correctly treat null as "not found") is good. The behavior is correct.

Overall

The fix is correct and complete. All the right methods (copy(), extractSubQueries(), refersToParent(), toString(), compare()) are updated. The two execution paths (SELECT * using compare(), SELECT fields using projection injection) are properly handled. Test coverage with 4 cases (ASC, DESC, SELECT *, extended CASE) is solid. No blocking issues.

[robfrank]

Addressed the review in a follow-up commit:

• refersToParent() now checks expression.refersToParent() to handle CASE WHEN expressions referencing $parent in subquery contexts
• Added a comment to getName() clarifying the intentional null-return semantics when only expression is set

The getter/setter suggestion was skipped - modifier and recordAttr are also public fields in this class, so expression is consistent with the existing pattern.

[codecov]

Codecov Report

❌ Patch coverage is 83.33333% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 65.15%. Comparing base (6964490) to head (ce5a052).
⚠️ Report is 2 commits behind head on main.

Files with missing lines	Patch %	Lines
...edb/query/sql/executor/SelectExecutionPlanner.java	75.00%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3778      +/-   ##
==========================================
+ Coverage   64.61%   65.15%   +0.54%     
==========================================
  Files        1580     1580              
  Lines      116313   116354      +41     
  Branches    24669    24677       +8     
==========================================
+ Hits        75155    75813     +658     
+ Misses      30914    30224     -690     
- Partials    10244    10317      +73     

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

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.