Fix ORDER BY Seg Fault (again...)

[Lightning11wins]

This PR fixes a segmentation fault caused by some queries that used order by.

Sorry if this gives you de ja vu from #107. It turns out there were more seg faults in that area of the code than we thought. I've done a review, and I'm reasonably sure that I've found all of them.

[greptile-apps]

Greptile Summary

This PR adds null-pointer guards to mqobAnalyzeBeforeGroup and mqobAnalyzeAfterGroup in multiq_orderby.c to prevent segmentation faults when ORDER BY or GROUP BY child items (or their Expr fields) are NULL.

• In mqobAnalyzeBeforeGroup, two loops that previously short-circuit–combined item->Expr into one condition now split into explicit item == NULL || item->Expr == NULL guards followed by the AggLevel check.
• In mqobAnalyzeAfterGroup, order_item/group_item are now declared as loop-local variables; the comparison loop extracts order_expr/group_expr with null safety, skipping both-NULL pairs via continue and treating one-NULL pairs as a mismatch (sep_groupby = 1). A previously unguarded order_item access in the post-comparison loop is also protected.

Confidence Score: 5/5

Safe to merge — the changes are narrow, targeted null guards that eliminate confirmed crash paths without altering any query-result semantics.

All three dereference sites that could segfault on a NULL item or item->Expr are now guarded. The expCompareExpressions return-value convention (1=equal, 0=different) confirms that rewriting !func() as func()==0 is semantically identical. The both-NULL continue and one-NULL sep_groupby=1 choices are conservative and correct. No existing logic paths are altered beyond the new early-exit guards.

No files require special attention.

Important Files Changed

Filename	Overview
centrallix/multiquery/multiq_orderby.c	Adds NULL guards for ORDER BY/GROUP BY child items in both analysis functions; logic is preserved correctly, with expCompareExpressions (which returns 1=equal, 0=different) used consistently with the original !func() idiom now written as == 0.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A["mqobAnalyzeAfterGroup()\nfound ORDER BY + GROUP BY\nwith equal item counts"] --> B["Loop i over order_qs->Children"]
    B --> C["Fetch order_item, group_item\nfrom Children.Items[i]"]
    C --> D["Extract order_expr / group_expr\n(NULL-safe via ternary)"]
    D --> E{both NULL?}
    E -- yes --> F["continue\n(skip pair, no mismatch)"]
    F --> B
    E -- no --> G{one NULL?}
    G -- yes --> H["sep_groupby = 1\nbreak"]
    G -- no --> I["expCompareExpressions\norder_expr, group_expr"]
    I -- "returns 0 (different)" --> H
    I -- "returns 1 (same)" --> B
    B -- loop done --> J["Second loop:\norder_qs->Children"]
    J --> K{order_item == NULL\nor Expr == NULL?}
    K -- yes --> L["continue"]
    L --> J
    K -- no --> M{sep_groupby == 1\nor AggLevel == 1?}
    M -- yes --> N["Add to post-group\nOrderBy list"]
    N --> J
    M -- no --> J

Loading

_{Reviews (2): Last reviewed commit: "Clean up fixed code." | Re-trigger Greptile}

[Lightning11wins]

PR is ready for human review.

[gbeeley]

I'd like to take a deeper look at this one and review the assumptions in the code about the order by item list, since this issue has come up in more than one place. We may want to replace the fixed size order by items array with an XArray.

[Lightning11wins]

Maybe that's the correct choice, especially if it means we don't need to have nulls in the array.