Skip to content

Conversation

@jeffreyaven
Copy link
Member

Two fixes for window function execution:

  1. Parser fix: Fixed "order by order by" duplication in WindowSpec.Format()

    • The OrderBy.Format() method adds its own "order by" prefix
    • WindowSpec.Format() was also adding "order by" before calling OrderBy
    • Changed to iterate OrderBy items directly without the prefix
  2. Execution layer fix: Preserve OVER clause during AST function rewriting

    • Added node.Over = newNode.Over in 5 astvisit files: - query_rewriting.go - from_rewrite.go - fragment_rewriting.go - internally_routable_typing.go - provider_string_extract.go
    • Without this, window functions lost their OVER clause during rewriting

Description

Type of change

  • Bug fix (non-breaking change to fix a bug).
  • Feature (non-breaking change to add functionality).
  • Breaking change.
  • Other (eg: documentation change). Please explain.

Issues referenced.

Evidence

Checklist:

  • A full round of testing has been completed, and there are no test failures as a result of these changes.
  • The changes are covered with functional and/or integration robot testing.
  • The changes work on all supported platforms.
  • Unit tests pass locally, as per the developer guide.
  • Robot tests pass locally, as per the developer guide.
  • Linter passes locally, as per the developer guide.

Variations

Tech Debt

Two fixes for window function execution:

1. Parser fix: Fixed "order by order by" duplication in WindowSpec.Format()
   - The OrderBy.Format() method adds its own "order by" prefix
   - WindowSpec.Format() was also adding "order by" before calling OrderBy
   - Changed to iterate OrderBy items directly without the prefix

2. Execution layer fix: Preserve OVER clause during AST function rewriting
   - Added node.Over = newNode.Over in 5 astvisit files:
     - query_rewriting.go
     - from_rewrite.go
     - fragment_rewriting.go
     - internally_routable_typing.go
     - provider_string_extract.go
   - Without this, window functions lost their OVER clause during rewriting
@jeffreyaven jeffreyaven merged commit de05d7e into main Dec 1, 2025
19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants