Skip to content

[Refactor] Add new AST node types and resolve AST TODOs#99

Open
colinthebomb1 wants to merge 6 commits intomainfrom
feature/improve-ast-todos
Open

[Refactor] Add new AST node types and resolve AST TODOs#99
colinthebomb1 wants to merge 6 commits intomainfrom
feature/improve-ast-todos

Conversation

@colinthebomb1
Copy link
Collaborator

Overview:

This PR adds new node types and adds functionality to existing ones to improve the constructed ASTs for test files, resolving most of the TODOs identified in PR #97.

Code Changes:

  • Added TypeNode for SQL type keywords (TEXT, DATE, NULL, etc.) — replaces FunctionNode("TEXT", _args=[LiteralNode("{}")])
  • Added ListNode for value lists (e.g. the RHS of IN expressions) — replaces raw Python lists
  • Added IntervalNode for INTERVAL expressions — replaces FunctionNode("INTERVAL", ...)
  • Added CaseNode for CASE WHEN ... THEN ... ELSE ... END — replaces nested FunctionNode("CASE"/"WHEN"/"THEN"/"ELSE")
  • Added _distinct and _distinct_on parameters to SelectNode for SELECT DISTINCT / DISTINCT ON
  • Added NodeType.TYPE, LIST, INTERVAL, CASE to enums
  • Updated expected ASTs in data/asts.py to use the new node types

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR refactors the internal AST representation to better model several SQL constructs (types/keywords, value lists, intervals, CASE expressions, and DISTINCT variants) and updates the expected AST fixtures accordingly.

Changes:

  • Introduces new AST node types: TypeNode, ListNode, IntervalNode, and CaseNode, plus new NodeType enum values.
  • Extends SelectNode to represent DISTINCT and DISTINCT ON.
  • Updates data/asts.py expected ASTs (and tweaks formatter tests) to use the new node types.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 7 comments.

File Description
core/ast/node.py Adds new node classes and extends SelectNode to model DISTINCT/DISTINCT ON.
core/ast/enums.py Adds NodeType enum values for the new node types.
data/asts.py Updates expected AST fixtures to use TypeNode, ListNode, IntervalNode, CaseNode, and DISTINCT metadata.
tests/test_query_formatter.py Adjusts/relaxes formatter tests by commenting out some formatter calls and minor assertion key change.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

def __init__(self, _items: List['Node'], **kwargs):
"""SELECT clause node. _distinct_on is the list of expressions for DISTINCT ON (e.g. ListNode of columns)."""
def __init__(self, _items: List['Node'], _distinct: bool = False, _distinct_on: Optional['Node'] = None, **kwargs):
super().__init__(NodeType.SELECT, children=_items, **kwargs)
Copy link

Copilot AI Feb 28, 2026

Choose a reason for hiding this comment

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

SelectNode tracks _distinct_on but does not include it in children. Any generic AST traversal that relies on children will miss DISTINCT ON expressions, which can lead to incorrect rewrites/formatting/analysis. Consider representing DISTINCT ON as part of the node’s subtree (e.g., include it in children with a dedicated wrapper node/field-aware traversal) and update consumers accordingly.

Suggested change
super().__init__(NodeType.SELECT, children=_items, **kwargs)
# Include DISTINCT ON expressions in children so generic AST traversals see them.
children: List[Node] = list(_items) if _items is not None else []
if _distinct_on is not None:
children.append(_distinct_on)
super().__init__(NodeType.SELECT, children=children, **kwargs)

Copilot uses AI. Check for mistakes.
colinthebomb1 and others added 3 commits March 3, 2026 12:30
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
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.

2 participants