-
Notifications
You must be signed in to change notification settings - Fork 3
[Refactor] Add basic formatter #89
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,265 @@ | ||||||||||
| import mo_sql_parsing as mosql | ||||||||||
| from core.ast.node import QueryNode | ||||||||||
| from core.ast.node import ( | ||||||||||
| QueryNode, SelectNode, FromNode, WhereNode, TableNode, ColumnNode, | ||||||||||
| LiteralNode, OperatorNode, FunctionNode, GroupByNode, HavingNode, | ||||||||||
| OrderByNode, LimitNode, OffsetNode, SubqueryNode, VarNode, VarSetNode, | ||||||||||
|
||||||||||
| OrderByNode, LimitNode, OffsetNode, SubqueryNode, VarNode, VarSetNode, | |
| OrderByNode, LimitNode, OffsetNode, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The same comment as in Yihong's PR. We will support Subquery in future PRs when we introduce a complex test case with a subquery.
baiqiushi marked this conversation as resolved.
Show resolved
Hide resolved
baiqiushi marked this conversation as resolved.
Show resolved
Hide resolved
Copilot
AI
Dec 4, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Potential edge case: The function formatting logic assumes functions have at least one argument. SQL functions with no arguments (e.g., NOW(), CURRENT_TIMESTAMP()) would result in {func_name: []}. Consider handling the zero-argument case explicitly, e.g., return {func_name: args} if args else {func_name: None} or verify mosql's expected format for zero-argument functions.
| args = [format_expression(arg) for arg in node.children] | |
| args = [format_expression(arg) for arg in node.children] | |
| if not args: | |
| return {func_name: None} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. We can fix it once we have a test case for it.
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1,89 @@ | ||||||
| import mo_sql_parsing as mosql | ||||||
| from core.query_formatter import QueryFormatter | ||||||
| from core.ast.node import ( | ||||||
| OrderByItemNode, QueryNode, SelectNode, FromNode, WhereNode, TableNode, ColumnNode, | ||||||
| LiteralNode, OperatorNode, FunctionNode, GroupByNode, HavingNode, | ||||||
| OrderByNode, LimitNode, OffsetNode, SubqueryNode, VarNode, VarSetNode, JoinNode | ||||||
|
||||||
| OrderByNode, LimitNode, OffsetNode, SubqueryNode, VarNode, VarSetNode, JoinNode | |
| OrderByNode, LimitNode, OffsetNode, JoinNode |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The same as the above. We will use them once we have a test case with a subquery.
Copilot
AI
Dec 4, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Import of 'NodeType' is not used.
| from core.ast.enums import NodeType, JoinType, SortOrder | |
| from core.ast.enums import JoinType, SortOrder |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch. Let's remove it.
Copilot
AI
Dec 4, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unused import: get_query is imported but never used in this test file. Consider removing it.
| from data.queries import get_query |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch. Let's remove it.
baiqiushi marked this conversation as resolved.
Show resolved
Hide resolved
Copilot
AI
Dec 4, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] Limited test coverage: Consider adding test cases for edge cases such as:
- SELECT with no FROM clause
- WHERE with OR conditions
- Different join types (LEFT, RIGHT, FULL)
- Nested subqueries
- Functions with multiple arguments
- NULL comparisons (IS NULL, IS NOT NULL)
While the current test provides good basic coverage, these cases would ensure the formatter handles various SQL patterns correctly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. We will add them in the following PRs.
Copilot
AI
Dec 4, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] Test could be more comprehensive: The test only validates the final SQL output but doesn't verify the intermediate JSON format produced by ast_to_json(). Consider adding an assertion to check the JSON structure matches mosql's expected format, which would help catch issues in the AST-to-JSON conversion step separately from the JSON-to-SQL step.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Duplicate import:
QueryNodeis imported on line 2 and again on line 4. Remove the duplicate import on line 2.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch, let's remove it.