Skip to content

feat(dialect): add MariaDB + Snowflake to playground, fix WASM dialectMap, unify config validation#432

Merged
ajitpratap0 merged 37 commits intomainfrom
feat/mariadb-dialect
Mar 25, 2026
Merged

feat(dialect): add MariaDB + Snowflake to playground, fix WASM dialectMap, unify config validation#432
ajitpratap0 merged 37 commits intomainfrom
feat/mariadb-dialect

Conversation

@ajitpratap0
Copy link
Copy Markdown
Owner

Summary

Closes #426 — MariaDB dialect: complete playground + config + docs integration
Closes #428 — Snowflake dialect: add to playground dropdown and fix config validation

The MariaDB parser implementation (30 commits, SEQUENCE DDL, temporal tables, CONNECT BY, PERIOD FOR) was complete on this branch. This PR adds the final integration layer: playground UI, WASM backend wiring, config validation consistency, and documentation updates.


What Changed

Playground UI (website/src/components/playground/Playground.tsx)

  • Added { value: "mariadb", label: "MariaDB" } and { value: "snowflake", label: "Snowflake" } to the DIALECTS dropdown
  • Playground now exposes all 8 named dialects + Generic (9 total)

WASM backend (wasm/main.go)

  • Added "mariadb": sqlkeywords.DialectMariaDB to dialectMap — without this, selecting MariaDB silently fell back to generic parsing
  • Added "clickhouse": sqlkeywords.DialectClickHouse — pre-existing silent fallback bug discovered and fixed; ClickHouse was in the UI dropdown but not wired in the backend

Config validation — both layers unified

  • pkg/config/config.go: added "snowflake" to validDialects map and error message (it was accepted by --dialect flag but rejected in YAML config)
  • cmd/gosqlx/internal/config/schema.go and config.go: added "mariadb" and "snowflake" to ValidDialects slice and inline validation — a fully separate config stack that was missed
  • cmd/gosqlx/cmd/validate.go: updated --dialect flag help text to include snowflake
  • Tests in both config packages updated to cover the new dialects

Documentation + LSP

  • Updated doc comments in pkg/config/config.go, pkg/config/doc.go, pkg/config/lsp.go (runtime LSP string shown to IDEs), cmd/gosqlx/internal/config/doc.go, cmd/gosqlx/internal/config/config.go
  • CLAUDE.md: add Snowflake + MariaDB to dialect list (was missing both)
  • README.md: 7 → 8 dialects in stats bar, add MariaDB to all dialect lists and feature table
  • docs/SQL_COMPATIBILITY.md: 6 → 8 Supported Dialects (ClickHouse and MariaDB were missing)
  • website/src/lib/constants.ts: add MariaDB to website feature description
  • Publishing drafts (hn-draft.md, gosqlx-marketing-launch.md, comparison-article-draft.md): updated to 8 dialects

Bugs Found and Fixed During Review

Bug File Impact
MariaDB selected → generic parsing wasm/main.go Silent misbehavior for all MariaDB queries in playground
ClickHouse selected → generic parsing wasm/main.go Pre-existing; ClickHouse in UI dropdown but not wired
snowflake rejected in YAML config pkg/config/config.go validation.dialect: snowflake in .gosqlx.yaml returned error
mariadb/snowflake rejected by internal config cmd/gosqlx/internal/config/ CLI config.LoadDefault() rejected both via its own separate dialect list

Test Plan

  • go test ./pkg/config/... ./pkg/sql/keywords/... ./cmd/gosqlx/internal/config/... -count=1 — PASS
  • go test -race ./pkg/sql/keywords/... ./pkg/sql/parser/... -count=1 — PASS, no data races
  • Playground dropdown shows: Generic, PostgreSQL, MySQL, MariaDB, SQLite, SQL Server, Oracle, Snowflake, ClickHouse
  • Selecting MariaDB and parsing CREATE SEQUENCE s START WITH 1 returns a MariaDB-specific AST (not generic)
  • gosqlx validate --dialect snowflake "SELECT $1" — exits 0
  • .gosqlx.yaml with validation.dialect: snowflake — accepted by config load
  • gosqlx validate --list-dialects — includes mariadb and snowflake

🤖 Generated with Claude Code

Ajit Pratap Singh and others added 30 commits March 22, 2026 19:01
… React error #310

useState(copied) and useCallback(handleCopy) were declared after the
if (loading) and if (error) early returns, violating the Rules of Hooks.

When the WASM binary was a 404, the component always hit the error branch
so both renders exited with the same hook count. After committing the WASM
binary (#423), the component now successfully transitions loading→ready,
causing React to see 11 hooks instead of 9 on the second render → #310.

Fix: move both hooks above all conditional returns.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…equenceStatement nodes

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…nce tests

- Guard s.Name nil dereference in CreateSequenceStatement.ToSQL,
  DropSequenceStatement.ToSQL, and AlterSequenceStatement.ToSQL
- Split SequenceOptions.Restart (*LiteralValue) into two fields:
  Restart bool (bare RESTART) and RestartWith *LiteralValue (RESTART WITH n)
- Update writeSequenceOptions to emit bare RESTART or RESTART WITH n accordingly
- Add ast_sequence_test.go with full ToSQL table-driven tests and pool round-trip test

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…fields

- Add SystemTimeClauseType enum with AS OF, BETWEEN, FROM/TO, ALL variants
- Add ForSystemTimeClause struct for MariaDB FOR SYSTEM_TIME temporal queries
- Add PeriodDefinition struct for PERIOD FOR clauses in CREATE TABLE
- Extend TableReference with ForSystemTime field (MariaDB 10.3.4+)
- Extend CreateTableStatement with WithSystemVersioning and PeriodDefinitions fields
- Add ForSystemTimeClause.ToSQL() and tableRefSQL integration in sql.go

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ctStatement field order

Add SQL() methods to ForSystemTimeClause, ConnectByClause, and PeriodDefinition
(all implement expressionNode()) so they satisfy the Expression interface fully
without silently degrading via the exprSQL() fallback. Move StartWith and
ConnectBy fields in SelectStatement to directly follow Having, matching logical
SQL clause ordering.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…B dialect

Add parseCreateSequenceStatement, parseDropSequenceStatement, and
parseAlterSequenceStatement to mariadb.go with full option parsing
(START WITH, INCREMENT BY, MINVALUE, MAXVALUE, CYCLE, CACHE, RESTART WITH).
Wire dispatch into parseStatement() for DROP/ALTER and into parseCreateStatement()
for CREATE. Gate all paths behind isMariaDB() so MySQL and other dialects
are unaffected. Add six passing parser tests in mariadb_test.go.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
… SYSTEM VERSIONING, PERIOD FOR)

Add parseForSystemTimeClause and parseTemporalPointExpression to mariadb.go
supporting AS OF, BETWEEN, FROM/TO, and ALL variants. Hook into
parseFromTableReference in select_subquery.go (after alias, before SQL Server
hints) with a peek-ahead guard so FOR is only consumed when followed by
SYSTEM_TIME. Add WITH SYSTEM VERSIONING parsing to parseCreateTable (after
closing paren, before PARTITION BY) and PERIOD FOR column parsing to the
column loop in ddl.go. Add four passing tests for temporal queries and
system versioning in mariadb_test.go.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…iaDB dialect

Add parseConnectByCondition to mariadb.go which handles the PRIOR prefix
operator by wrapping the referenced column in a FunctionCall node and
building a BinaryExpression for the full PRIOR col = parent_col pattern.
Wire START WITH and CONNECT BY [NOCYCLE] parsing into parseSelectStatement
in select.go after the HAVING clause. Guard CONNECT and START from being
consumed as table aliases in parseFromTableReference via a peek-ahead check
in select_subquery.go. Add three passing tests covering basic, NOCYCLE, and
no-START-WITH variants in mariadb_test.go.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ion test

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…rser

AST layer:
- Add `Prior` UnaryOperator constant for CONNECT BY PRIOR expressions
- Add `NoCache bool` to SequenceOptions (NOCACHE was previously a silent no-op)
- Add `Pos models.Location` to all 6 new MariaDB AST nodes (CreateSequence,
  DropSequence, AlterSequence, ForSystemTimeClause, ConnectByClause, PeriodDefinition)
- Add `NewDropSequenceStatement()` / `ReleaseDropSequenceStatement()` pool funcs
  to match Create/Alter sequence pooling consistency
- Emit NOCACHE in writeSequenceOptions when NoCache=true

Keywords:
- Remove duplicate MINVALUE, MAXVALUE, INCREMENT, RESTART, NOCACHE entries
  from MARIADB_SPECIFIC (already covered by base/Oracle keyword lists)

Parser:
- Fix PRIOR operator: switch from FunctionCall to UnaryExpression{Operator: Prior}
- Fix PRIOR on RHS: CONNECT BY col = PRIOR col now parsed correctly
- Fix parseJoinedTableRef: add isMariaDBClauseKeyword guard to prevent CONNECT/START
  from being consumed as table aliases (same guard already in parseFromTableReference)
- Fix parseJoinedTableRef: add FOR SYSTEM_TIME temporal clause support on JOIN refs
- Fix DROP SEQUENCE: support IF NOT EXISTS in addition to IF EXISTS
- Fix DROP SEQUENCE: use NewDropSequenceStatement() for pool consistency
- Fix parseSequenceOptions: set opts.NoCache=true for NOCACHE keyword
- Add comment in parseTemporalPointExpression explaining quote-stripping behaviour

Tests:
- Add TestMariaDB_ConnectBy_PriorOnRight
- Add TestMariaDB_DropSequence_IfNotExists
- Add TestMariaDB_Sequence_NoCache
- Expand testdata SQL files with NO MINVALUE/MAXVALUE forms, PRIOR-on-right cases,
  IF NOT EXISTS on DROP, and multi-table temporal JOIN query

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…T BY, dedup

Parser dispatch (parser.go, ddl.go, select.go):
- Populate Pos on CreateSequenceStatement (at SEQUENCE token in ddl.go)
- Populate Pos on DropSequenceStatement (at DROP token in parser.go)
- Populate Pos on AlterSequenceStatement (at ALTER token in parser.go)
- Populate Pos on ConnectByClause (at CONNECT token in select.go)
- Populate Pos on PeriodDefinition (at PERIOD token in ddl.go)

mariadb.go:
- Fix NO CACHE (two-token) to also set opts.NoCache=true, matching NOCACHE
- Fix parseConnectByCondition to handle complex AND/OR chains:
  CONNECT BY PRIOR id = parent_id AND active = 1 now fully parsed
- Extract isMariaDBClauseStart() method (was duplicated closure in two functions)
- Populate Pos on ForSystemTimeClause (at SYSTEM_TIME token)
- Add comment clarifying IF NOT EXISTS is a non-standard permissive extension

select_subquery.go:
- Remove both isMariaDBClauseKeyword closures, replace with p.isMariaDBClauseStart()

ast.go:
- Update DropSequenceStatement doc to show [IF EXISTS | IF NOT EXISTS]

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…eriodDefinition.SQL()

- Move START WITH / CONNECT BY emission to after HAVING, before ORDER BY
- Implement PeriodDefinition.SQL() — was silently returning empty string
- Add round-trip tests for both fixes

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
… PERIOD FOR SYSTEM_TIME test

- Add '// zero all fields' comment to ReleaseCreateSequenceStatement and ReleaseAlterSequenceStatement
- Add design-choice comments on expressionNode() for ForSystemTimeClause, ConnectByClause, PeriodDefinition
- Add TestMariaDB_CreateTable_PeriodForSystemTime to cover PERIOD FOR SYSTEM_TIME parsing
- Fix parsePeriodDefinition to use parseColumnName so SYSTEM_TIME (reserved keyword) is accepted as period name
- Add GENERATED ALWAYS AS ROW START/END column constraint parsing in parseColumnConstraint (MariaDB system-versioned columns)

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ns pure CONNECT BY queries

Oracle's CONNECT BY weight is 3; MariaDB's was also 3, causing a tie broken
by non-deterministic map iteration. Reducing MariaDB to weight 2 ensures
Oracle wins when CONNECT BY is the only hint. MariaDB is still correctly
detected when its unique high-weight hints (NEXTVAL, FOR SYSTEM_TIME, etc.)
are present.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…n, structured errors

- Replace Cycle bool + NoCycle bool with CycleOption enum to prevent
  invalid state where both could be true simultaneously
- Add validation in parseSequenceOptions for contradictory CACHE/NOCACHE
- Replace fmt.Errorf with p.expectedError() for consistent error style

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
17 benchmarks across 4 groups:
- BenchmarkMariaDB_Sequence: CREATE/ALTER/DROP SEQUENCE (5 cases)
- BenchmarkMariaDB_ForSystemTime: temporal table queries (5 cases)
- BenchmarkMariaDB_ConnectBy: hierarchical queries with PRIOR (4 cases)
- BenchmarkMariaDB_Mixed: combined features — CTE+temporal, CTE+CONNECT BY,
  CREATE TABLE WITH SYSTEM VERSIONING (3 cases)

Baseline (M2): 398–3001 ns/op, 984–6763 B/op
Closes #428 — Snowflake was missing despite full keyword support.
Closes #426 (partial) — MariaDB playground integration complete.
Playground now exposes all 8 named dialects + Generic (9 total).
…fect

Without this, selecting MariaDB in the dropdown silently fell back to
generic dialect parsing. The UI entry was a no-op at the parser layer.
Ajit Pratap Singh and others added 5 commits March 25, 2026 11:56
snowflake was accepted by --dialect flag but rejected when set in
YAML config. Both layers are now consistent.

Part of #428.
…and doc comments

Add "snowflake" and "mariadb" to the ValidDialects slice and inline
validDialects list in cmd/gosqlx/internal/config, update corresponding
tests, and sync doc comments in pkg/config to reflect all supported
dialects including the two new ones.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- CLAUDE.md: add Snowflake + MariaDB (was missing both)
- README.md: 7→8 dialects in stats bar, add MariaDB to feature lists
- SQL_COMPATIBILITY.md: 6→8 Supported Dialects (add ClickHouse + MariaDB)
- website/src/lib/constants.ts: add MariaDB to feature description

Resolves #426 (docs integration)
Resolves #428 (count sync)
@vercel
Copy link
Copy Markdown

vercel bot commented Mar 25, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
gosqlx-website Ready Ready Preview, Comment Mar 25, 2026 8:40am

@github-actions
Copy link
Copy Markdown

github-actions bot commented Mar 25, 2026

Claude finished @ajitpratap0's task —— View job


Code Review: MariaDB + Snowflake Dialect Integration

Review Progress

  • Analyze playground UI changes (Playground.tsx)
  • Review WASM backend fixes (main.go)
  • Examine config validation changes
  • Check documentation updates
  • Evaluate test coverage
  • Assess code quality and best practices
  • Check for security concerns
  • Provide final recommendations

🔍 Review Summary

This PR successfully integrates MariaDB and Snowflake dialect support into the playground and fixes critical backend wiring bugs. The implementation demonstrates strong attention to detail and comprehensive testing coverage.

Code Quality Assessment

Excellent - The changes follow established patterns, maintain consistency, and demonstrate good software engineering practices.

Strengths:

  • Consistent patterns: All dialect additions follow the same structure across UI, backend, and config layers
  • Comprehensive coverage: Changes span playground UI, WASM backend, CLI config, and documentation
  • Good separation of concerns: Each layer handles its specific responsibilities
  • Proper testing: New dialects are covered in existing test suites

Implementation Quality:

  • Playground.tsx:25-30: Clean array addition following existing patterns
  • wasm/main.go:39-44: Proper mapping from string keys to dialect constants
  • Config validation: Unified approach across both config packages with proper error handling

🐛 Bugs Found & Fixed

The PR identifies and fixes 4 critical bugs:

  1. MariaDB silent fallback (wasm/main.go:39) - MariaDB selection fell back to generic parsing
  2. ClickHouse silent fallback (wasm/main.go:40) - Pre-existing bug, ClickHouse in UI but not backend
  3. Snowflake YAML rejection (pkg/config/config.go:219) - Snowflake rejected in .gosqlx.yaml config
  4. Internal config rejection (cmd/gosqlx/internal/config/schema.go:27) - Separate CLI config validation missed new dialects

These were genuine functional issues that would have caused silent misbehavior in production.

🚀 Performance Considerations

No concerns - Changes are additive and don't affect runtime performance:

  • Static array extensions (O(1) lookups)
  • No new computational overhead
  • WASM bundle size increase is negligible (2 additional map entries)

🔒 Security Assessment

Secure - No security vulnerabilities introduced:

  • ✅ No user input validation changes
  • ✅ No new attack vectors
  • ✅ Follows existing secure patterns
  • ✅ Uses established dialect enumeration (no arbitrary string execution)

Test Coverage Analysis

Comprehensive - Excellent test coverage for the changes:

Existing Tests Cover New Dialects:

  • pkg/config/config_test.go:165: Tests mariadb and snowflake in validation loop
  • cmd/gosqlx/internal/config/schema_test.go:29-30: Explicit test cases for both dialects
  • Config validation tests ensure proper error handling

Test Quality:

  • Both validation layers tested: Main config and internal CLI config
  • Edge cases covered: Invalid dialects, case sensitivity, empty strings
  • Integration tested: WASM dialectMap includes new entries

📋 Documentation Quality

Thorough - Documentation updates are comprehensive and accurate:

Updated Locations:

  • Help text in validate.go:226: Includes snowflake in CLI flag help
  • Multiple README and doc files: Dialect count 7→8, feature matrices updated
  • LSP runtime descriptions: IDE integration reflects new dialects
  • Marketing materials: Consistent 8-dialect messaging

Documentation Standards:

  • ✅ Consistent terminology across all files
  • ✅ Accurate dialect counts in all locations
  • ✅ No missed references to old counts

🔧 Architecture & Design

Well-designed - The implementation follows clean architecture principles:

Clean Layering:

  • UI Layer: Playground.tsx - Pure presentation logic
  • API Layer: wasm/main.go - Translation between JS and Go
  • Business Layer: Dialect constants and validation
  • Config Layer: Dual config validation (unified patterns)

No Design Issues:

  • Maintains existing interfaces
  • No breaking changes
  • Follows established patterns
  • Proper error handling throughout

🎯 Final Verdict: APPROVE

This is a high-quality PR that successfully integrates MariaDB and Snowflake support while fixing critical bugs. The implementation is thorough, well-tested, and follows established patterns.

Key Strengths:

  • ✅ Fixes 4 genuine bugs that caused silent failures
  • ✅ Comprehensive test coverage for all changes
  • ✅ Excellent documentation consistency
  • ✅ Clean, maintainable code following existing patterns
  • ✅ No security or performance concerns

Recommendation:

Ready to merge - This PR enhances functionality, fixes bugs, and maintains code quality standards. No changes needed.


@github-actions github-actions bot added documentation Improvements or additions to documentation cli wasm WebAssembly playground labels Mar 25, 2026
@github-actions
Copy link
Copy Markdown

github-actions bot commented Mar 25, 2026

Claude finished @ajitpratap0's task —— View job


Code Review: MariaDB + Snowflake Dialect Integration

Review Summary

This PR successfully addresses the MariaDB and Snowflake dialect integration for both the playground UI and configuration validation. The implementation is clean, well-structured, and fixes several critical bugs that were silently causing incorrect behavior.


Strengths

1. Critical Bug Fixes (wasm/main.go:39-44)

  • Fixed silent fallback bugs: MariaDB and ClickHouse selections in playground were silently falling back to generic parsing
  • Complete dialectMap: All 8 dialects now properly mapped in WASM backend
  • Impact: Users selecting MariaDB/ClickHouse now get dialect-specific parsing instead of generic

2. Unified Config Validation

  • Two-layer fix: Both pkg/config/config.go:215-226 and cmd/gosqlx/internal/config/schema.go:23-32 updated
  • Consistency: Snowflake was accepted by CLI flags but rejected in YAML config - now unified
  • Test coverage: Both config test suites updated with new dialect validation

3. Playground UI Enhancement (website/src/components/playground/Playground.tsx:25,29)

  • Clean integration: MariaDB and Snowflake added to DIALECTS array
  • User experience: All 8 supported dialects now accessible via dropdown
  • Consistency: UI now matches backend capabilities

4. Documentation Updates

  • Comprehensive: Updated CLAUDE.md, README.md, SQL_COMPATIBILITY.md
  • Accurate counts: 7 → 8 dialects consistently updated across all docs
  • LSP integration: Runtime description strings updated for IDE users

⚠️ Minor Issues & Suggestions

1. Inconsistent Default Dialects

// pkg/config/config.go:168 - defaults to "postgresql"
Dialect: "postgresql",

// cmd/gosqlx/internal/config/config.go:173 - defaults to ""
Dialect: "",

Recommendation: Consider standardizing default behavior. Empty string is more permissive but "postgresql" is more explicit.

2. Test Coverage Validation Needed

  • Tests exist for both config packages (pkg/config/config_test.go, cmd/gosqlx/internal/config/schema_test.go)
  • Both include new dialect validation tests
  • Verification needed: Run go test ./pkg/config/... ./cmd/gosqlx/internal/config/... -v to confirm coverage

3. CLI Help Text Update (cmd/gosqlx/cmd/validate.go:51-52)

  • Updated --dialect flag help to include snowflake
  • Missing: MariaDB not explicitly mentioned in help text (though it's supported)

🔒 Security Assessment

  • No security concerns identified
  • Configuration validation properly sanitizes inputs
  • File path expansion uses standard library functions safely
  • No new attack vectors introduced

🚀 Performance Considerations

  • Positive impact: Fixes silent fallbacks that could cause unexpected parsing behavior
  • Zero overhead: Additions are lookup operations in existing maps/slices
  • Memory efficient: No additional allocations in hot paths

📋 Test Plan Verification

Based on the PR description, the following should be tested:

# Test config validation
go test ./pkg/config/... ./cmd/gosqlx/internal/config/... -count=1

# Test race conditions
go test -race ./pkg/sql/keywords/... ./pkg/sql/parser/... -count=1

# Verify CLI dialect support
./gosqlx validate --dialect snowflake "SELECT $1"
./gosqlx validate --dialect mariadb "CREATE SEQUENCE s START WITH 1"

# Test YAML config acceptance
echo "validation:\n  dialect: snowflake" > .test-config.yaml
./gosqlx validate --config .test-config.yaml "SELECT 1"

🎯 Final Recommendations

Immediate Actions

  1. Approve and merge - this is a solid implementation that fixes real bugs
  2. 🧪 Verify tests pass - run the test commands mentioned above
  3. 📝 Update CLI help text to mention MariaDB explicitly

Follow-up Considerations

  1. Standardize default dialects across the two config systems
  2. Add integration tests that verify playground → WASM → parser flow works correctly
  3. Consider documentation about the difference between the two config systems

Overall Assessment: ✅ APPROVED

This PR successfully addresses the stated goals and fixes several important bugs. The code quality is high, the implementation is clean, and the changes are well-tested. The unified configuration validation was particularly important as it fixed inconsistent behavior between CLI and YAML configuration.

Impact: Users can now reliably use MariaDB and Snowflake dialects in both the playground and CLI tools, with proper validation and feedback.


@github-actions
Copy link
Copy Markdown

github-actions bot commented Mar 25, 2026

Claude finished @ajitpratap0's task —— View job


Code Review: MariaDB + Snowflake Dialect Integration

🔍 Review Summary

This PR successfully integrates MariaDB and Snowflake dialect support into the playground and fixes critical backend wiring bugs. The implementation demonstrates strong attention to detail and comprehensive testing coverage.

Code Quality Assessment

Excellent - The changes follow established patterns, maintain consistency, and demonstrate good software engineering practices.

Strengths:

  • Consistent patterns: All dialect additions follow the same structure across UI, backend, and config layers
  • Comprehensive coverage: Changes span playground UI, WASM backend, CLI config, and documentation
  • Good separation of concerns: Each layer handles its specific responsibilities
  • Proper testing: New dialects are covered in existing test suites

Implementation Quality:

  • Playground.tsx:25-30: Clean array addition following existing patterns
  • wasm/main.go:39-44: Proper mapping from string keys to dialect constants
  • Config validation: Unified approach across both config packages with proper error handling

🐛 Bugs Found & Fixed

The PR identifies and fixes 4 critical bugs:

  1. MariaDB silent fallback (wasm/main.go:39) - MariaDB selection fell back to generic parsing
  2. ClickHouse silent fallback (wasm/main.go:40) - Pre-existing bug, ClickHouse in UI but not backend
  3. Snowflake YAML rejection (pkg/config/config.go:219) - Snowflake rejected in .gosqlx.yaml config
  4. Internal config rejection (cmd/gosqlx/internal/config/schema.go:27) - Separate CLI config validation missed new dialects

These were genuine functional issues that would have caused silent misbehavior in production.

🚀 Performance Considerations

No concerns - Changes are additive and don't affect runtime performance:

  • Static array extensions (O(1) lookups)
  • No new computational overhead
  • WASM bundle size increase is negligible (2 additional map entries)

🔒 Security Assessment

Secure - No security vulnerabilities introduced:

  • ✅ No user input validation changes
  • ✅ No new attack vectors
  • ✅ Follows existing secure patterns
  • ✅ Uses established dialect enumeration (no arbitrary string execution)

Test Coverage Analysis

Comprehensive - Excellent test coverage for the changes:

Existing Tests Cover New Dialects:

  • pkg/config/config_test.go:165: Tests mariadb and snowflake in validation loop
  • cmd/gosqlx/internal/config/schema_test.go:29-30: Explicit test cases for both dialects
  • Config validation tests ensure proper error handling

Test Quality:

  • Both validation layers tested: Main config and internal CLI config
  • Edge cases covered: Invalid dialects, case sensitivity, empty strings
  • Integration tested: WASM dialectMap includes new entries

📋 Documentation Quality

Thorough - Documentation updates are comprehensive and accurate:

Updated Locations:

  • Help text in validate.go:226: Includes snowflake in CLI flag help
  • Multiple README and doc files: Dialect count 7→8, feature matrices updated
  • LSP runtime descriptions: IDE integration reflects new dialects
  • Marketing materials: Consistent 8-dialect messaging

Documentation Standards:

  • ✅ Consistent terminology across all files
  • ✅ Accurate dialect counts in all locations
  • ✅ No missed references to old counts

🔧 Architecture & Design

Well-designed - The implementation follows clean architecture principles:

Clean Layering:

  • UI Layer: Playground.tsx - Pure presentation logic
  • API Layer: wasm/main.go - Translation between JS and Go
  • Business Layer: Dialect constants and validation
  • Config Layer: Dual config validation (unified patterns)

No Design Issues:

  • Maintains existing interfaces
  • No breaking changes
  • Follows established patterns
  • Proper error handling throughout

🎯 Final Verdict: APPROVE

This is a high-quality PR that successfully integrates MariaDB and Snowflake support while fixing critical bugs. The implementation is thorough, well-tested, and follows established patterns.

Key Strengths:

  • ✅ Fixes 4 genuine bugs that caused silent failures
  • ✅ Comprehensive test coverage for all changes
  • ✅ Excellent documentation consistency
  • ✅ Clean, maintainable code following existing patterns
  • ✅ No security or performance concerns

Recommendation:

Ready to merge - This PR enhances functionality, fixes bugs, and maintains code quality standards. No changes needed.


@ajitpratap0 ajitpratap0 merged commit e7b4895 into main Mar 25, 2026
21 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cli documentation Improvements or additions to documentation wasm WebAssembly playground

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[bug] Snowflake dialect missing from WASM playground dropdown [enhancement] Add MariaDB dialect support

1 participant