[Refactor] Basic Query Parser #90

HazelYuAhiru · 2025-11-21T20:05:45Z

Overview:

This PR implements the query parser and adds tests for converting an input SQL query string into our custom AST structure (SQL → JSON via mosql → custom AST). The base test case passes, and all additional test cases run without errors.

Code Changes:

Added a base parsing test in tests/test_query_parser.py.
Implemented parse() and its helper functions.

colinthebomb1

LGTM! let one small comment

tests/test_query_parser.py

Copilot

Pull request overview

This PR implements a query parser that converts SQL strings to a custom AST structure via the mosql library. It adds a basic parsing test and implements the core parse() method along with helper functions for parsing different SQL clauses.

Key Changes:

Implements QueryParser.parse() method that converts SQL to AST via mosql intermediate JSON
Adds helper methods for parsing SELECT, FROM, WHERE, GROUP BY, HAVING, and ORDER BY clauses
Implements expression parsing, alias resolution, and operator/join type normalization
Adds comprehensive test case test_basic_parse() that validates parsing of complex queries with JOINs, aggregations, and multiple clauses

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 20 comments.

File	Description
tests/test_query_parser.py	Adds test_basic_parse() function with comprehensive test case covering JOINs, WHERE, GROUP BY, HAVING, ORDER BY, LIMIT, and OFFSET; updates imports to include new node types
core/query_parser.py	Implements complete query parser with parse() method and 10+ helper methods for clause parsing, expression parsing, alias resolution, and type normalization

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

Copilot · 2025-12-04T00:08:50Z

core/query_parser.py

+    def resolve_aliases(self, expr: Node) -> Node:
+        if isinstance(expr, OperatorNode):
+            # Recursively resolve aliases in operator operands
+            left = self.resolve_aliases(expr.children[0])
+            right = self.resolve_aliases(expr.children[1])
+            return OperatorNode(left, expr.name, right)
+        elif isinstance(expr, FunctionNode):
+            # Check if this function matches an aliased function from SELECT
+            if expr.alias is None:
+                for alias, aliased_expr in self.aliases.items():
+                    if isinstance(aliased_expr, FunctionNode):
+                        if (expr.name == aliased_expr.name and 
+                            len(expr.children) == len(aliased_expr.children) and
+                            all(expr.children[i] == aliased_expr.children[i] 
+                                for i in range(len(expr.children)))):
+                            # This function matches an aliased one, use the alias
+                            expr.alias = alias
+                            break
+            return expr
+        elif isinstance(expr, ColumnNode):
+            # Check if this column matches an aliased column from SELECT
+            if expr.alias is None:
+                for alias, aliased_expr in self.aliases.items():
+                    if isinstance(aliased_expr, ColumnNode):
+                        if (expr.name == aliased_expr.name and 
+                            expr.parent_alias == aliased_expr.parent_alias):
+                            # This column matches an aliased one, use the alias
+                            expr.alias = alias
+                            break
+            return expr
+        else:
+            return expr


[nitpick] The alias resolution in the HAVING clause iterates through all aliases to find matching FunctionNode or ColumnNode instances (lines 233-241, 246-252). For large SELECT clauses with many aliases, this could be inefficient. Consider optimizing by creating a separate lookup structure or only checking relevant aliases.

That is OK for now.

core/query_parser.py

tests/test_query_parser.py

core/query_parser.py

Copilot · 2025-12-04T00:08:55Z

core/query_parser.py


        # [2] Our new code
        # Any (JSON) -> AST (QueryNode)
+        self.aliases = {}


The self.aliases instance variable is initialized within the parse() method rather than in __init__. This means the aliases dictionary persists between parse calls and could lead to stale alias references affecting subsequent parses. Consider either: 1) initializing self.aliases = {} in a proper __init__ method, or 2) passing aliases as a parameter through the helper methods instead of using instance state.

This is a good point which I also noticed. If we use the instance member to store aliases, this instance is not safe for multi-threading. [1] One way to solve this issue is through a helper parameter being passed through call the following function calls through the function arguments, similar to what we use as the memo in the rewriter engine. [2] Or we can make sure the QueryParser is declared as non-thread-safe, and every place we need to use the parse function, we create a new instance of the parser, i.e., parser = QueryParser(), and if we use this approach, it makes more sense to init the self.aliases = {} in the __init__ function instead of this parse function. @HazelYuAhiru , please make a decision with coordination with @colinthebomb1 's PR, if he uses the same approach (some internal state shared by different functions as an instance-level state), we can use [2]. If he already used [1], you may change to use [1]. If you decide to use [2], please find a proper annotation tag in Python to say it is non-thread-safe.

Thanks! I'v updated the parser to use a thread-safe approach

core/query_parser.py

Copilot · 2025-12-04T00:08:55Z

core/query_parser.py

+from core.ast.node import (
+    Node, QueryNode, SelectNode, FromNode, WhereNode, TableNode, ColumnNode, 
+    LiteralNode, OperatorNode, FunctionNode, GroupByNode, HavingNode,
+    OrderByNode, OrderByItemNode, LimitNode, OffsetNode, SubqueryNode, VarNode, VarSetNode, JoinNode


Import of 'SubqueryNode' is not used.
Import of 'VarNode' is not used.
Import of 'VarSetNode' is not used.

Suggested change

OrderByNode, OrderByItemNode, LimitNode, OffsetNode, SubqueryNode, VarNode, VarSetNode, JoinNode

OrderByNode, OrderByItemNode, LimitNode, OffsetNode, JoinNode

I also noticed this version of implementation does not consider subquery. We will consider it in the next iteration.

Got it, in this case I will leave a TODO comment above as reminders.

baiqiushi

Thanks @HazelYuAhiru for the PR! It looks fantastic overall. Ony a few small changes are needed. Please take care of them. Thanks!

baiqiushi · 2025-12-07T18:33:47Z

core/query_parser.py

+from core.ast.node import (
+    Node, QueryNode, SelectNode, FromNode, WhereNode, TableNode, ColumnNode, 
+    LiteralNode, OperatorNode, FunctionNode, GroupByNode, HavingNode,
+    OrderByNode, OrderByItemNode, LimitNode, OffsetNode, SubqueryNode, VarNode, VarSetNode, JoinNode


I also noticed this version of implementation does not consider subquery. We will consider it in the next iteration.

core/query_parser.py

baiqiushi · 2025-12-07T18:45:02Z

core/query_parser.py


        # [2] Our new code
        # Any (JSON) -> AST (QueryNode)
+        self.aliases = {}


This is a good point which I also noticed. If we use the instance member to store aliases, this instance is not safe for multi-threading. [1] One way to solve this issue is through a helper parameter being passed through call the following function calls through the function arguments, similar to what we use as the memo in the rewriter engine. [2] Or we can make sure the QueryParser is declared as non-thread-safe, and every place we need to use the parse function, we create a new instance of the parser, i.e., parser = QueryParser(), and if we use this approach, it makes more sense to init the self.aliases = {} in the __init__ function instead of this parse function. @HazelYuAhiru , please make a decision with coordination with @colinthebomb1 's PR, if he uses the same approach (some internal state shared by different functions as an instance-level state), we can use [2]. If he already used [1], you may change to use [1]. If you decide to use [2], please find a proper annotation tag in Python to say it is non-thread-safe.

core/query_parser.py

tests/test_query_parser.py

core/query_parser.py

tests/test_query_parser.py

first batch fix Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

HazelYuAhiru added 2 commits November 20, 2025 17:34

parser complete

43bf081

clean up

74ba863

HazelYuAhiru requested a review from colinthebomb1 November 21, 2025 20:05

colinthebomb1 previously approved these changes Dec 1, 2025

View reviewed changes

colinthebomb1 reviewed Dec 1, 2025

View reviewed changes

tests/test_query_parser.py Outdated Show resolved Hide resolved

update select from set to list

fbbbbfc

HazelYuAhiru dismissed colinthebomb1’s stale review via fbbbbfc December 3, 2025 22:17

HazelYuAhiru marked this pull request as ready for review December 3, 2025 22:17

HazelYuAhiru requested a review from baiqiushi December 3, 2025 22:17

baiqiushi requested a review from Copilot December 4, 2025 00:02

Copilot started reviewing on behalf of baiqiushi December 4, 2025 00:03 View session

Copilot finished reviewing on behalf of baiqiushi December 4, 2025 00:06

Copilot AI reviewed Dec 4, 2025

View reviewed changes

baiqiushi requested changes Dec 7, 2025

View reviewed changes

HazelYuAhiru and others added 5 commits December 16, 2025 16:21

Apply suggestions from code review

0bb3113

first batch fix Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Apply suggestions from code review

d2ce8de

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

batch update 3

f95a659

update inconsistency

7614b6c

thread safe alias tracking

a9c4214

	OrderByNode, OrderByItemNode, LimitNode, OffsetNode, SubqueryNode, VarNode, VarSetNode, JoinNode
	OrderByNode, OrderByItemNode, LimitNode, OffsetNode, JoinNode

[Refactor] Basic Query Parser #90

Are you sure you want to change the base?

[Refactor] Basic Query Parser #90

Uh oh!

Conversation

HazelYuAhiru commented Nov 21, 2025

Overview:

Code Changes:

Uh oh!

colinthebomb1 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Dec 4, 2025

Choose a reason for hiding this comment

Uh oh!

baiqiushi Dec 7, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI Dec 4, 2025

Choose a reason for hiding this comment

Uh oh!

baiqiushi Dec 7, 2025

Choose a reason for hiding this comment

Uh oh!

HazelYuAhiru Dec 22, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI Dec 4, 2025

Choose a reason for hiding this comment

Uh oh!

baiqiushi Dec 7, 2025

Choose a reason for hiding this comment

Uh oh!

HazelYuAhiru Dec 16, 2025

Choose a reason for hiding this comment

Uh oh!

baiqiushi left a comment

Choose a reason for hiding this comment

Uh oh!

baiqiushi Dec 7, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

baiqiushi Dec 7, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

colinthebomb1 left a comment •

edited

Loading