refactor(#215): Phase 3A — Migrate matchToken() to ModelType

[ajitpratap0]

Part of #215 token type unification.

Migrates the last remaining string-based Type comparisons to int-based ModelType:

• matchToken() — the most-called parser comparison function
• isStringLiteral() — removed string fallback (tokens normalized at entry)
• isNumericLiteral() — removed string fallback
• isNonReservedKeyword() — uses ModelType with literal fallback for generic keywords
• recovery.go — removed string fallback in isStatementStart()
• Error message formatting across parser files (expressions.go, select.go, parser.go)

When ModelType is a specific type (not TokenTypeKeyword), matchToken uses O(1) integer comparison. For generic keywords that map to TokenTypeKeyword, it falls back to case-insensitive string comparison to avoid ambiguous matches.

Phase 3A of 3.

[claude]

Claude finished @ajitpratap0's task —— View job

---

PR Review: Token Type Migration (Phase 3A)

Reviewing the migration from string-based to int-based token comparisons in the parser.

Review Tasks:

• Read repository guidelines (CLAUDE.md)
• Analyze parser.go changes (matchToken function)
• Review expressions.go modifications
• Examine recovery.go updates
• Check select.go changes
• Assess performance implications
• Identify potential bugs or issues
• Evaluate security considerations
• Check test coverage requirements
• Provide comprehensive feedback

Review Summary

This PR successfully migrates the critical matchToken() function from string-based to integer-based token type comparisons, completing Phase 3A of the token type unification effort (#215). The changes are well-implemented and align with the performance goals.

✅ Strengths

1. Performance Optimization: The new matchToken() implementation uses O(1) integer comparison for specific token types vs O(n) string comparison, achieving the stated goal of faster token matching in the parser's hottest code path.

2. Backwards Compatibility: Smart fallback strategy maintains compatibility:

   • First tries fast integer comparison via stringTypeToModelType mapping
   • Falls back to case-insensitive string comparison for unmapped tokens or when ModelType == TokenTypeKeyword
   • This ensures existing test cases and edge cases continue working during the transition

3. Clean Code: Removal of string fallbacks in isStringLiteral(), isNumericLiteral(), and isStatementStartingKeyword() eliminates technical debt and simplifies logic.

4. Error Message Improvements: Consistent use of ModelType.String() instead of string(Type) in error messages provides better error reporting.

⚠️ Areas of Concern

1. Token Normalization Dependency: The matchToken() logic heavily depends on normalizeTokens() setting correct ModelType values. If tokens come in without normalization or with incorrect mappings in stringTypeToModelType, the fallback to string comparison may not work as expected.

2. Edge Case Risk: The condition mt != models.TokenTypeKeyword excludes generic keywords from fast path. While this prevents ambiguous matches, it means some valid specific keywords might still use slow string comparison if they're mapped to TokenTypeKeyword.

3. Testing Coverage: No new tests visible for the modified matchToken() logic. Given this is the most-called parser function, comprehensive test coverage is crucial.

🔍 Specific Code Issues

pkg/sql/parser/parser.go:664-678

if mt, ok := stringTypeToModelType[expected]; ok && mt != models.TokenTypeKeyword {
    // Fast path logic...
    if p.currentToken.ModelType == models.TokenTypeKeyword &&
        strings.EqualFold(string(p.currentToken.Type), string(expected)) {

This mixed approach could be confusing. Consider adding comments explaining when each path is used.

pkg/sql/parser/expressions.go:557
The modelTypeUnset check was removed but the constant still exists. This could lead to inconsistency if other parts of the codebase still rely on this pattern.

🚀 Performance Impact

• Positive: O(1) integer comparison vs O(n) string comparison for specific token types
• Measured: Should be benchmarked to quantify the performance gain
• Hotpath: matchToken() is correctly identified as the most-called parser function

🛡️ Security Considerations

No security concerns identified. The changes are purely performance optimizations without introducing new attack vectors.

✅ Test Coverage Requirements

Based on CLAUDE.md requirements:

• Race detection: Must run task test:race to ensure thread safety
• Parser coverage: Currently at 76.1% - this change should include specific tests for the new matchToken() logic paths
• Benchmarks: Should include benchmarks comparing old vs new matchToken() performance

📋 Recommendations

1. Add comprehensive tests for matchToken() covering both integer and string fallback paths
2. Run performance benchmarks to measure the actual performance improvement
3. Consider documentation explaining the dual-path strategy in matchToken()
4. Verify edge cases where ModelType might be unset or incorrectly mapped

Conclusion

This is a solid performance optimization that maintains backwards compatibility while achieving the stated goal of faster token matching. The implementation is thoughtful and well-structured, though it would benefit from additional test coverage and performance measurements.

Recommendation: ✅ Approve with suggestions - The changes are functionally correct and achieve the performance goals, but would benefit from additional testing and benchmarking.