fix(parser): support INSERT ... SELECT syntax

[ajitpratap0]

Fixes #288

Adds support for INSERT INTO table [(columns)] SELECT ... syntax, which is standard SQL.

Changes:

• Parser now checks for SELECT token after column list (in addition to VALUES)
• Changed InsertStatement.Query field type from *SelectStatement to Statement interface to support UNION/EXCEPT/INTERSECT
• Updated formatter and SQL generator to handle the interface type
• Added comprehensive tests for INSERT...SELECT variants
• Existing INSERT...VALUES tests continue to pass

[claude]

Claude finished @ajitpratap0's task —— View job

---

Code Review Complete ✅

I've completed a comprehensive review of PR #303 that adds INSERT...SELECT syntax support to GoSQLX. Here's my detailed analysis:

Summary

This is a well-implemented feature that properly adds INSERT...SELECT support with good architectural decisions. The changes are minimal, focused, and maintain backwards compatibility.

---

✅ Strengths

1. Parser Implementation (pkg/sql/parser/dml.go)

• Clean logic: Lines 61-68 show elegant parsing that checks for SELECT token after column list
• Proper integration: Uses parseSelectWithSetOperations() to support UNION/EXCEPT/INTERSECT in INSERT statements
• Backwards compatibility: Existing INSERT...VALUES logic remains unchanged (lines 69-116)

2. AST Architecture (pkg/sql/ast/ast.go:1099)

• Smart interface conversion: Changed InsertStatement.Query from *SelectStatement to Statement interface
• Future-proof design: Supports any Statement type (SelectStatement, SetOperation, etc.)
• Type safety: Maintains compile-time safety while adding flexibility

3. Formatter & SQL Generator Updates

• Format.go (lines 271-280): Properly handles both SelectStatement and other Statement types using interface detection
• SQL.go (lines 562-577): Uses stmtSQL() helper for proper SQL generation
• CLI formatter (sql_formatter.go:269-280): Includes SetOperation support and fallback handling

4. Test Coverage

• Comprehensive test cases in insert_select_test.go:
  • Basic INSERT...SELECT with/without columns
  • Complex queries with WHERE clauses
  • SET operations (UNION) support
  • Backwards compatibility verification
• Good test structure: Uses table-driven tests with clear assertions

---

🔍 Code Quality Analysis

Parser Logic Flow

// Line 61-68: Clean conditional logic
if p.isType(models.TokenTypeSelect) {
    p.advance() // Consume SELECT
    stmt, err := p.parseSelectWithSetOperations()
    if err != nil {
        return nil, err
    }
    query = stmt
} else if p.isType(models.TokenTypeValues) {
    // ... existing VALUES logic
}

Interface Usage Pattern

// Lines 271-280: Smart type detection in formatter
if i.Query != nil {
    sb.WriteString(f.clauseSep())
    if fq, ok := i.Query.(interface{ Format(FormatOptions) string }); ok {
        sb.WriteString(fq.Format(opts))
    } else {
        sb.WriteString(stmtSQL(i.Query))
    }
}

---

⚠️ Minor Observations

1. Error Handling

• Current implementation is solid, but could benefit from more specific error messages
• Consider adding context like "failed to parse SELECT in INSERT statement"

2. Edge Cases

• All major edge cases appear covered (UNION, complex WHERE, etc.)
• Test for UNION case appropriately handles different AST root types

3. Performance

• No performance concerns: Changes are minimal and don't add significant overhead
• Interface conversion is efficient in Go
• Existing object pooling patterns remain intact

---

🛡️ Security Assessment

• No security vulnerabilities identified
• SQL generation uses existing safe patterns
• No new injection vectors introduced
• Maintains existing parameterization patterns

---

🧪 Test Quality

The test suite is well-designed:

• Covers core functionality and edge cases
• Tests backwards compatibility
• Verifies SQL roundtrip doesn't panic
• Includes realistic query patterns
• Proper error handling verification

---

📝 Recommendations

Consider for Future (not blocking):

1. Enhanced error messages with more context
2. Additional test cases for deeply nested SET operations
3. Performance benchmarks (though no concerns expected)

---

✅ Final Verdict

APPROVED - This is a high-quality implementation that:

• ✅ Follows project patterns and conventions
• ✅ Maintains backwards compatibility
• ✅ Has comprehensive test coverage
• ✅ Uses good architectural decisions
• ✅ Introduces no security or performance concerns
• ✅ Has clean, readable code

The changes successfully implement INSERT...SELECT syntax while maintaining the existing codebase quality standards. Ready for merge! 🚀

---

[ajitpratap0]

Consolidated into batch PR #304