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).

Up to standards ✅

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

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.

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 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.

@@            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.