Skip to content

Handle Multiple ORDER BYs with DEFAULTs - #127

Merged
gbeeley merged 10 commits into
masterfrom
add-default-keyword
Aug 31, 2026
Merged

Handle Multiple ORDER BYs with DEFAULTs#127
gbeeley merged 10 commits into
masterfrom
add-default-keyword

Conversation

@Lightning11wins

Copy link
Copy Markdown
Contributor

This PR will add functionality for handling multiple ORDER BY statements. Later statements append to earlier ones, or overwrite them if they use the DEFAULT keyword, as suggested by Greg.

This PR includes both docs and testing for the new functionality and the DEFAULT keyword.

@Lightning11wins Lightning11wins self-assigned this Jun 17, 2026
@Lightning11wins Lightning11wins added testing Includes testing, either new tests or updates to existing tests. ai-review Request AI review for PRs. documentation Changes, improvements, or fixes to documentation files. size: trivial Easy to review, probably ~100 lines or fewer. labels Jun 17, 2026
@greptile-apps

greptile-apps Bot commented Jun 17, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR implements support for multiple ORDER BY clauses in Centrallix SQL, where later clauses append their sort keys to earlier ones unless a DEFAULT keyword marks a clause as overridable by any subsequent ORDER BY. The change covers parser support, post-processing merge logic, a new bitmask flag, documentation, and seven test cases.

  • Parser (mq_internal_SyntaxParse): default is added to the reserved-words list and, when encountered immediately after ORDER BY, sets the new MQ_SF_DEFAULTORDER flag on the clause node.
  • Post-processor (mq_internal_PostProcess): A new two-pass merge loop first finds the last ORDER BY, then removes any DEFAULT-flagged clauses that are not last, and finally folds all surviving clauses' children into the first one before expression compilation.
  • Tests: Seven CSV-query cases validate single ORDER BY, two-clause append, DEFAULT override, lone DEFAULT, double-DEFAULT, and a three-clause mixed scenario.

Confidence Score: 5/5

  • The change is safe to merge. The merge logic is pointer-safe, memory management is correct, and all seven test cases validate the new semantics.
  • The core merge algorithm correctly uses pointer identity (not indices) to track the last ORDER BY through removals, children are detached before FreeQS is called so there is no double-free risk, and the ob parameter is cleanly re-derived inside PostProcess rather than relying on the stale parser-side pointer. The only finding is a mildly inaccurate sentence in the documentation.
  • The new paragraph in centrallix-doc/SQL.txt has a sentence that inaccurately describes the append behavior for non-DEFAULT ORDER BY clauses and is worth correcting before the docs ship.

Important Files Changed

Filename Overview
centrallix-doc/SQL.txt Adds documentation for multi-ORDER BY and DEFAULT keyword; one sentence in the new paragraph inaccurately implies only the last ORDER BY clause is ever appended.
centrallix/multiquery/multiquery.c Adds ORDER BY merge logic in PostProcess and DEFAULT keyword parsing in SyntaxParse. Memory management is handled correctly (children are moved before FreeQS), pointer-based last_orderby tracking is stable through removals, and the ob parameter is safely re-derived from scratch.
centrallix/include/multiquery.h Adds MQ_SF_DEFAULTORDER flag (65536) with no conflicts in the existing bitmask sequence.
centrallix/tests/test_sql_orderby_05.to Seven test cases covering single ORDER BY, multi ORDER BY merge, DEFAULT override, lone DEFAULT, double-DEFAULT, and mixed three-clause scenarios.
centrallix/tests/test_sql_orderby_05.cmp Expected output for all seven test cases; results are consistent with the described DEFAULT-overwrite semantics.
centrallix-sysdoc/BeeleyCodingStyle.md Minor prose clarification in the comment-style rule; no functional impact.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[Parse SQL tokens] --> B{ORDER BY token?}
    B -- No --> C[Other clause handling]
    B -- Yes --> D[Allocate MQ_T_ORDERBYCLAUSE node]
    D --> E{Next token == DEFAULT?}
    E -- Yes --> F[Set MQ_SF_DEFAULTORDER flag]
    E -- No --> G[mlxHoldToken - push back]
    F --> H[Parse ORDER BY items]
    G --> H
    H --> I[Add clause to qs->Children]
    I --> J[Continue parsing...]

    J --> K[mq_internal_PostProcess]
    K --> L[Pass 1: find last ORDER BY node]
    L --> M[Reset ob = NULL, iterate children]
    M --> N{NodeType == ORDERBYCLAUSE?}
    N -- No --> O[Skip, i++]
    N -- Yes --> P{DEFAULT flag AND not last?}
    P -- Yes --> Q[xaRemoveItem + FreeQS, cnt--]
    P -- No --> R{ob == NULL?}
    R -- Yes --> S[ob = subtree, i++]
    R -- No --> T[Move children to ob, xaClear, FreeQS, cnt--]
    S --> U[Compile ORDER BY expressions]
    T --> U
    Q --> M
Loading

Reviews (2): Last reviewed commit: "Update default to be a reserved word." | Re-trigger Greptile

@Lightning11wins

Copy link
Copy Markdown
Contributor Author

This PR is ready for human review.

@gbeeley gbeeley left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A couple of changes needed - thanks!

Comment thread centrallix/multiquery/multiquery.c Outdated

/** Drop a DEFAULT clause that isn't the last one. **/
if ((subtree->Flags & MQ_SF_DEFAULTORDER) && !is_last)
goto remove_order_by_element;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Avoid the goto since this isn't a retry mechanism or an error condition handler, and since this can be easily refactored as an if() block.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

goto removed.

Comment thread centrallix/multiquery/multiquery.c
@Lightning11wins

Copy link
Copy Markdown
Contributor Author

@gbeeley I think I've resolved your comments.

@gbeeley

gbeeley commented Aug 21, 2026

Copy link
Copy Markdown
Member

Let's resolve PR 117 first, moving to XArray, and then revisit this one.

@Lightning11wins Lightning11wins added size: medium Might be hard to review, usually less than ~5000 lines. and removed size: trivial Easy to review, probably ~100 lines or fewer. labels Aug 24, 2026
@Lightning11wins

Copy link
Copy Markdown
Contributor Author

This PR no longer seems to be of trivial size.

@Lightning11wins

Copy link
Copy Markdown
Contributor Author

Let's resolve PR 117 first, moving to XArray, and then revisit this one.

The move to XArray for OrderBy in PR #117 has been completed.

@gbeeley gbeeley left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good - thanks!

@gbeeley
gbeeley merged commit c69ac39 into master Aug 31, 2026
2 checks passed
@Lightning11wins
Lightning11wins deleted the add-default-keyword branch August 31, 2026 18:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ai-review Request AI review for PRs. documentation Changes, improvements, or fixes to documentation files. size: medium Might be hard to review, usually less than ~5000 lines. testing Includes testing, either new tests or updates to existing tests.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants