refactor: decompose parser package into focused modules

[ajitpratap0]

Problem

Parser package has ~8,873 LOC with files up to 1,505 lines. Hard to navigate and trending toward god object.

Fix

Pure code reorganization (no logic changes):

• select.go (1,505) → 4 focused files
• expressions.go (1,452) → 4 focused files
• ddl.go (878) → 4 focused files
• dml.go (840) → 4 focused files

All 8,551 existing tests pass unchanged. Total LOC unchanged.

[claude]

Claude finished @ajitpratap0's task —— View job

---

Code Review Complete ✅

I've analyzed the parser package decomposition refactor. Here's my comprehensive review:

Todo List

• Read repository guidelines and context
• Analyze the original large files structure
• Review the decomposed module organization
• Check for import/dependency issues
• Verify Go best practices compliance
• Assess potential bugs or regressions
• Evaluate performance implications
• Check security considerations
• Provide comprehensive feedback

Review Summary

Overall Assessment: ✅ Excellent refactoring - This is a well-executed code reorganization that significantly improves maintainability without introducing functional changes.

Code Quality & Best Practices ✅

Strengths:

1. Clear Module Separation: Each file has a focused responsibility with excellent naming conventions:

   • select.go → select_clauses.go, select_set_ops.go, select_subquery.go
   • expressions.go → expressions_literal.go, expressions_operators.go, expressions_complex.go
   • ddl.go → ddl_columns.go, ddl_index.go, ddl_view.go
   • dml.go → dml_insert.go, dml_update.go, dml_delete.go, dml_merge.go

2. Consistent Documentation: Each decomposed file includes proper headers explaining its purpose and related modules

3. Preserved Package Structure: All files remain in the same package, avoiding import cycle risks

4. Method Organization: Related parsing methods are logically grouped (e.g., all JOIN parsing in select_clauses.go)

Potential Bugs or Issues ✅

No Issues Found:

• Code appears to be pure reorganization with no logic changes
• All function signatures and implementations appear intact
• Error handling patterns remain consistent
• Import statements are properly maintained

Performance Considerations ✅

Positive Impact:

1. Build Performance: Smaller files will compile faster and improve IDE responsiveness
2. Developer Performance: Much easier to navigate and locate specific parsing logic
3. Runtime Performance: No impact - same compiled code, just better organized

No Negative Impact:

• Object pooling patterns remain unchanged
• No additional allocations introduced
• Memory layout unaffected by file reorganization

Security Considerations ✅

No Security Issues:

• No changes to input validation or error handling
• DoS protection mechanisms remain intact
• Context cancellation and recursion limits preserved
• No exposure of internal state or credentials

Test Coverage Assessment ✅

Strengths:

• PR description claims all 8,551 existing tests pass
• Test files remain in package directory
• No test modifications needed (good sign of true refactoring)

Recommendation: Run the full test suite including race detection to verify:

task test:race  # As specified in CLAUDE.md

Code Organization Excellence 📊

File Size Reduction:

• select.go: 1,505 lines → 274 lines + 3 focused modules
• expressions.go: 1,452 lines → 383 lines + 3 focused modules
• ddl.go: 878 lines → 508 lines + 3 focused modules
• dml.go: 840 lines → 23 lines + 4 focused modules

Maintainability Gains:

• Eliminates "god files" trending toward monoliths
• Makes code reviews more targeted and effective
• Reduces cognitive load for developers
• Improves IDE performance and navigation

Recommendations ✅

1. Verify Tests: Ensure task test:race passes to confirm no regressions
2. Update Documentation: Consider updating architecture docs if they reference file sizes
3. CI Pipeline: Verify all existing CI checks pass
4. Performance Regression: Run task bench to ensure no performance impact

Conclusion

This is an exemplary refactoring that follows the Single Responsibility Principle and significantly improves code maintainability. The decomposition is logical, well-documented, and preserves all functionality while making the codebase more approachable for future development.

Recommendation: ✅ Approve - This refactoring aligns perfectly with clean code principles and will make the parser package much more maintainable going forward.

---

[ajitpratap0]

Architect Review — PR #343: refactor: decompose parser package into focused modules

Summary

Pure code reorganization — no logic changes claimed. Splits 4 large files (select.go, expressions.go, ddl.go, dml.go) into 16 focused files. The motivation is sound: a 1,505-line select.go is legitimately hard to navigate. The risk profile is elevated for a "no logic changes" PR because of how much code moves.

---

✅ Strong Points

1. File split is principled

• ddl.go → ddl.go (CREATE TABLE core) + ddl_columns.go + ddl_index.go + ddl_view.go
• dml.go → dml.go (dispatcher) + dml_insert.go + dml_delete.go + dml_update.go + dml_merge.go
• select.go → select.go + select_clauses.go + select_set_ops.go + select_subquery.go
• expressions.go → expressions.go + expressions_complex.go + expressions_literal.go + expressions_operators.go

The file names are self-explanatory and follow a consistent <domain>_<subdomain>.go pattern.

2. Claim: 8,551 tests pass unchanged
If this passes full CI including parser integration tests, that's strong evidence of correctness. CI green is necessary but not sufficient for a refactor of this size — see concerns below.

---

🔴 Conflict Risk: Highest of All 7 PRs

This PR touches essentially every parser file. It will conflict with:

• #339/#345 (both modify cte.go, ddl.go, dml.go, select.go, etc. for token type changes)
• #340 (modifies cte.go, ddl.go for error position fixes)

If #345 and #340 are merged first, this PR will need to be rebased against main and all token-API changes re-applied in the 16 new files. This is significant merge work.

Merge order matters enormously here. This PR should be last in the sequence.

---

Design Concerns

1. Function ordering changed in dml.go
In the original dml.go, the order was:

parseInsertStatement → parseUpdateStatement → parseDeleteStatement → parseMergeStatement

In the new split, the diff shows parseDeleteStatement is moved to dml_delete.go and parseInsertStatement moves to dml_insert.go. The dispatcher in dml.go should still be verified to call all of them correctly — missing a case in the dispatcher would be a silent regression.

2. parseMergeWhenClause and parseMergeAction move to dml_merge.go
These are internal helpers. Verify they're not accidentally duplicated (a risk when splitting files manually) — Go compilation would catch duplicate function names, but the CI status being green suggests this is fine.

3. Column definition functions promoted from private to file-level
ddl_columns.go introduces parseColumnDef, parseColumnConstraint, parseReferentialActions, parseReferentialAction, parseTableConstraint, parseConstraintColumnList as new top-level (but unexported) functions. Since they were previously inline within ddl.go, verify they maintain the exact same behavior — especially parseReferentialActions which modifies state through return values.

4. No behavioral tests in this PR
Since the PR claims "no logic changes", it doesn't add new tests. However, it would strengthen confidence to add a brief parser smoke test that calls each new file's primary functions to ensure the package compiles and routes correctly.

---

Edge Case: Package-Level init() or var dependencies

If any of the split files had package-level var declarations or init() functions that depend on execution order, splitting across files could change initialization order. In Go, this is determined by file name alphabetical order within a package. Verify there are no such dependencies (e.g., in expressions_operators.go or expressions_literal.go).

---

Naming Consistency

• dml_merge.go follows the <domain>_<subdomain>.go convention ✓
• expressions_operators.go is slightly ambiguous — "operators" could mean parsing binary operators OR defining operator precedence. Consider expressions_binary.go for clarity, but this is minor.

---

Test Coverage

• Existing 8,551 tests cover behavior ✓
• Missing: explicit test that all 16 new files compile independently (satisfied by CI build)
• Recommended: add TestParserDecompositionSmoke that exercises one function from each new file

---

Verdict

Approve in principle. Must be merged last (after #345, #340).

After rebasing, re-verify:

1. All function definitions are present in exactly one file (no duplicates, no omissions)
2. The dml.go dispatcher correctly calls functions now defined in sub-files
3. No package-level initialization order issues

[ajitpratap0]

✅ Dispatcher Routing Verification — PR #343

Verified the dml.go dispatcher correctly routes to all split sub-files after parser decomposition. No conflicts or mismatches found.

Routing Map (parser.go parseStatement() → sub-file)

Token Type	Dispatcher Call	Sub-file	Function Found
TokenTypeSelect	parseSelectWithSetOperations()	select_set_ops.go	✅
TokenTypeInsert	parseInsertStatement()	dml_insert.go	✅
TokenTypeUpdate	parseUpdateStatement()	dml_update.go	✅
TokenTypeDelete	parseDeleteStatement()	dml_delete.go	✅
TokenTypeMerge	parseMergeStatement()	dml_merge.go	✅
TokenTypeWith	parseWithStatement()	cte.go	✅
TokenTypeCreate	parseCreateStatement()	ddl.go	✅
TokenTypeAlter	parseAlterTableStmt()	parser.go	✅
TokenTypeDrop	parseDropStatement()	ddl.go	✅
TokenTypeTruncate	parseTruncateStatement()	ddl.go	✅
TokenTypeRefresh	parseRefreshStatement()	ddl_view.go	✅
TokenTypeShow	parseShowStatement()	mysql.go	✅
TokenTypeDescribe/Explain	parseDescribeStatement()	mysql.go	✅
TokenTypeReplace	parseReplaceStatement()	mysql.go	✅

Test Results

ok  github.com/ajitpratap0/GoSQLX/pkg/sql/parser  27.045s

All packages pass: go test ./... — 0 failures across all packages.

Notes

• dml.go is a thin stub (package declaration + doc comment only) — all DML logic lives in the 4 sub-files as intended
• No function signature mismatches found between dispatcher calls and sub-file implementations
• dml_insert.go exports 5 parser methods (INSERT, ON CONFLICT, RETURNING, VALUES, OUTPUT clauses), dml_merge.go exports 3 (MERGE + MATCHED clause variants)

No fixes required. Dispatcher is correctly wired. Safe to merge.

[ajitpratap0]

Architect Review — PR #343: Decompose Parser Package into Focused Modules

Summary

This is a significant structural refactoring that splits the monolithic parser package into 13 focused modules. No behavioral changes are intended — this is pure organizational work. The decomposition follows clear domain lines:

New file	Responsibility
ddl_columns.go	Column definitions and table constraints
ddl_index.go	CREATE INDEX
ddl_view.go	CREATE VIEW, CREATE MATERIALIZED VIEW, REFRESH
dml_delete.go	DELETE statement
dml_insert.go	INSERT statement
dml_merge.go	MERGE statement
dml_update.go	UPDATE statement
expressions_complex.go	CASE, CAST, window functions, subqueries
expressions_literal.go	Literals, arrays, row constructors
expressions_operators.go	Operator precedence chain
select_clauses.go	WHERE, GROUP BY, HAVING, ORDER BY, LIMIT
select_set_ops.go	UNION, INTERSECT, EXCEPT
select_subquery.go	Subquery and derived table parsing

---

DML Dispatcher Verification (Previous Review Concern)

The previous review asked to verify that the DML dispatcher correctly routes to the new per-statement modules. Confirmed: the DML routing remains in dml.go via the existing parse dispatch logic. The extracted files (dml_delete.go, dml_insert.go, dml_merge.go, dml_update.go) contain the implementation functions, not new entry points. The dispatcher pattern is correct.

---

Architecture Analysis

Positive aspects:

1. File sizes are now manageable. The old ddl.go / dml.go / expressions.go god files were over 1000 lines each. Each new module is focused and navigable.

2. Module boundaries are coherent. DDL subtypes (columns, indexes, views) are correctly separated. DML operations are cleanly split. Expression parsing is organized by complexity layer.

3. No circular dependencies. All new files are in the same parser package, so the decomposition introduces no import cycle risk. This is the right call for a parser where functions freely call each other.

4. Comment headers on new files ("Related modules:" block in ddl.go header) correctly document the split and help future developers navigate.

5. parseTableHints() extracted into select_subquery.go is correctly reused from both parseTableReference() and parseJoinedTableRef(). The deduplication is good.

---

Concerns

1. parseJoinedTableRef() and parseTableReference() code duplication

Both functions handle:

• LATERAL keyword
• Derived table (subquery in parens)
• Simple qualified name
• Optional alias (with or without AS)
• SQL Server WITH (hints)

The logic is nearly identical. This refactor was an opportunity to unify them via a shared parseTableReferenceBase() helper that both can call. As-is, any future fix to alias parsing or table hint handling must be made in two places. This is a pre-existing issue, not introduced by this PR, but the decomposition makes it more visible.

2. goerrors import removed from ddl.go

The PR removes goerrors "github.com/ajitpratap0/GoSQLX/pkg/errors" from ddl.go because the functions that used it (parseCreateView, parseCreateMaterializedView) moved to ddl_view.go. Verify that ddl_view.go correctly imports goerrors — if it does not, view query error wrapping silently degrades to a bare error. (From the diff context, ddl_view.go is a new file and should have this import, but confirm.)

3. Test coverage for the decomposition

This is a structural change, and the existing test suite provides coverage. However, it would be valuable to confirm that the test suite passes cleanly on this branch without relying on the fixes in other PRs (especially #340, which modifies ddl.go and dml.go lines that this PR also touches). The merge order matters.

4. expressions_operators.go clarity

Moving the operator precedence chain into a dedicated file is the right call architecturally. Verify that the file contains the complete precedence ladder (from lowest OR/AND through comparison, BETWEEN/IN, additive, multiplicative, unary, and primary) in one readable sequence so future maintainers can see the full grammar without jumping files.

---

What Is Good

• The decomposition reduces cognitive load significantly — a developer debugging MERGE issues goes directly to dml_merge.go
• The dispatcher design is verified correct
• No behavioral changes means regression risk is low if the test suite is comprehensive
• The module headers clearly document the split for new contributors

---

Decision

Status: APPROVED (conditional on test suite passing)

This is clean structural work with no behavioral changes. The DML dispatcher is verified correct. Minor concerns (duplication in table reference parsing, verify goerrors import in ddl_view.go) are pre-existing issues or low risk. This PR should merge after verifying all tests pass on this branch independently.

Merge order note: This PR modifies ddl.go and dml.go which overlap with PRs #340 (uat-fixes) and #341 (fix-ddl-pools). This PR should merge BEFORE #340 and #341 to minimize conflict risk, or conflicts should be resolved carefully at merge time.

[ajitpratap0]

Architect Review — PR #343: Decompose Parser Package into Focused Modules

Summary

This is a significant structural refactoring that splits the monolithic parser package into 13 focused modules. No behavioral changes are intended — this is pure organizational work. The decomposition follows clear domain lines:

New file	Responsibility
ddl_columns.go	Column definitions and table constraints
ddl_index.go	CREATE INDEX
ddl_view.go	CREATE VIEW, CREATE MATERIALIZED VIEW, REFRESH
dml_delete.go	DELETE statement
dml_insert.go	INSERT statement
dml_merge.go	MERGE statement
dml_update.go	UPDATE statement
expressions_complex.go	CASE, CAST, window functions, subqueries
expressions_literal.go	Literals, arrays, row constructors
expressions_operators.go	Operator precedence chain
select_clauses.go	WHERE, GROUP BY, HAVING, ORDER BY, LIMIT
select_set_ops.go	UNION, INTERSECT, EXCEPT
select_subquery.go	Subquery and derived table parsing

---

DML Dispatcher Verification (Previous Review Concern)

The previous review asked to verify that the DML dispatcher correctly routes to the new per-statement modules. Confirmed: the DML routing remains in dml.go via the existing parse dispatch logic. The extracted files (dml_delete.go, dml_insert.go, dml_merge.go, dml_update.go) contain the implementation functions, not new entry points. The dispatcher pattern is correct.

---

Architecture Analysis

Positive aspects:

1. File sizes are now manageable. The old ddl.go / dml.go / expressions.go god files were over 1000 lines each. Each new module is focused and navigable.

2. Module boundaries are coherent. DDL subtypes (columns, indexes, views) are correctly separated. DML operations are cleanly split. Expression parsing is organized by complexity layer.

3. No circular dependencies. All new files are in the same parser package, so the decomposition introduces no import cycle risk. This is the right call for a parser where functions freely call each other.

4. Comment headers on new files ("Related modules:" block in ddl.go header) correctly document the split and help future developers navigate.

5. parseTableHints() extracted into select_subquery.go is correctly reused from both parseTableReference() and parseJoinedTableRef(). The deduplication is good.

---

Concerns

1. parseJoinedTableRef() and parseTableReference() code duplication

Both functions handle:

• LATERAL keyword
• Derived table (subquery in parens)
• Simple qualified name
• Optional alias (with or without AS)
• SQL Server WITH (hints)

The logic is nearly identical. This refactor was an opportunity to unify them via a shared parseTableReferenceBase() helper that both can call. As-is, any future fix to alias parsing or table hint handling must be made in two places. This is a pre-existing issue, not introduced by this PR, but the decomposition makes it more visible.

2. goerrors import removed from ddl.go

The PR removes goerrors "github.com/ajitpratap0/GoSQLX/pkg/errors" from ddl.go because the functions that used it (parseCreateView, parseCreateMaterializedView) moved to ddl_view.go. Verify that ddl_view.go correctly imports goerrors — if it does not, view query error wrapping silently degrades to a bare error. (From the diff context, ddl_view.go is a new file and should have this import, but confirm.)

3. Test coverage for the decomposition

This is a structural change, and the existing test suite provides coverage. However, it would be valuable to confirm that the test suite passes cleanly on this branch without relying on the fixes in other PRs (especially #340, which modifies ddl.go and dml.go lines that this PR also touches). The merge order matters.

4. expressions_operators.go clarity

Moving the operator precedence chain into a dedicated file is the right call architecturally. Verify that the file contains the complete precedence ladder (from lowest OR/AND through comparison, BETWEEN/IN, additive, multiplicative, unary, and primary) in one readable sequence so future maintainers can see the full grammar without jumping files.

---

What Is Good

• The decomposition reduces cognitive load significantly — a developer debugging MERGE issues goes directly to dml_merge.go
• The dispatcher design is verified correct
• No behavioral changes means regression risk is low if the test suite is comprehensive
• The module headers clearly document the split for new contributors

---

Decision

Status: APPROVED (conditional on test suite passing)

This is clean structural work with no behavioral changes. The DML dispatcher is verified correct. Minor concerns (duplication in table reference parsing, verify goerrors import in ddl_view.go) are pre-existing issues or low risk. This PR should merge after verifying all tests pass on this branch independently.

Merge order note: This PR modifies ddl.go and dml.go which overlap with PRs #340 (uat-fixes) and #341 (fix-ddl-pools). This PR should merge BEFORE #340 and #341 to minimize conflict risk, or conflicts should be resolved carefully at merge time.

[claude]

Claude finished @ajitpratap0's task —— View job

---

Code Review: Parser Package Decomposition ✅

I've completed a comprehensive review of this parser decomposition refactor.

Todo List

• Analyze the decomposed file structure and organization
• Check imports and dependencies in new files
• Review for potential bugs or regressions
• Assess performance implications
• Evaluate security considerations
• Check test coverage adequacy
• Provide comprehensive feedback

---

Review Summary

Overall Assessment: ✅ Excellent refactoring - This is a well-executed code reorganization that significantly improves maintainability without introducing functional changes.

Code Quality & Best Practices ✅

Strengths:

1. Clear Module Separation: Each file has a focused responsibility with excellent naming conventions:

   • select.go → select_clauses.go, select_set_ops.go, select_subquery.go
   • expressions.go → expressions_literal.go, expressions_operators.go, expressions_complex.go
   • ddl.go → ddl_columns.go, ddl_index.go, ddl_view.go
   • dml.go → dml_insert.go, dml_update.go, dml_delete.go, dml_merge.go

2. Consistent Documentation: Each decomposed file includes proper headers explaining its purpose and related modules

3. Preserved Package Structure: All files remain in the same package, avoiding import cycle risks

4. Method Organization: Related parsing methods are logically grouped (e.g., all JOIN parsing in select_clauses.go)

Potential Bugs or Issues ✅

No Issues Found:

• Code appears to be pure reorganization with no logic changes
• All function signatures and implementations appear intact
• Error handling patterns remain consistent
• Import statements are properly maintained
• Dispatcher routing verified: parseStatement() correctly calls functions now defined in sub-files

Performance Considerations ✅

Positive Impact:

1. Build Performance: Smaller files will compile faster and improve IDE responsiveness
2. Developer Performance: Much easier to navigate and locate specific parsing logic
3. Runtime Performance: No impact - same compiled code, just better organized

No Negative Impact:

• Object pooling patterns remain unchanged
• No additional allocations introduced
• Memory layout unaffected by file reorganization

Security Considerations ✅

No Security Issues:

• No changes to input validation or error handling
• DoS protection mechanisms remain intact
• Context cancellation and recursion limits preserved
• No exposure of internal state or credentials

Test Coverage Assessment ✅

Strengths:

• PR description claims all 8,551 existing tests pass
• Test files remain in package directory
• No test modifications needed (good sign of true refactoring)

Verified Dispatcher Routing:

• All DML functions (parseInsertStatement, parseUpdateStatement, parseDeleteStatement, parseMergeStatement) correctly route from parser.go:parseStatement()
• DDL functions (parseCreateStatement, parseDropStatement, parseTruncateStatement) properly dispatched
• Expression parsing maintains the precedence hierarchy across decomposed files

Code Organization Excellence 📊

File Size Reduction:

• select.go: 1,505 lines → 276 lines + 3 focused modules
• expressions.go: 1,452 lines → 383 lines + 3 focused modules
• ddl.go: 878 lines → 508 lines + 3 focused modules
• dml.go: 840 lines → 23 lines + 4 focused modules

Maintainability Gains:

• Eliminates "god files" trending toward monoliths
• Makes code reviews more targeted and effective
• Reduces cognitive load for developers
• Improves IDE performance and navigation

Import Analysis ✅

Verified Clean Dependencies:

• ddl_view.go correctly imports goerrors package for error wrapping
• All decomposed files have appropriate imports (models, ast, keywords)
• No circular dependencies introduced
• Standard library imports (fmt, strings) properly maintained

Recommendations ✅

1. Tests Pass: Existing test suite provides comprehensive coverage for behavioral correctness
2. Documentation Alignment: The decomposition aligns with clean architecture principles
3. Merge Safety: Previous dispatcher verification confirms all routing works correctly
4. Future Maintenance: New structure makes parser much more approachable for contributors

Minor Observations

Code Duplication Opportunity: parseJoinedTableRef() and parseTableReference() in select_subquery.go handle similar logic (LATERAL, derived tables, aliases, hints). This is a pre-existing pattern, not introduced by this PR, but the decomposition makes it more visible for future refactoring.

Conclusion

This is an exemplary refactoring that follows the Single Responsibility Principle and significantly improves code maintainability. The decomposition is logical, well-documented, and preserves all functionality while making the codebase much more approachable for future development.

Recommendation: ✅ Approve - This refactoring aligns perfectly with clean code principles and will make the parser package much more maintainable going forward.

---