refactor(vnext): share bounded SQL lexer - #187
Conversation
Coverage Report
File Coverage
|
||||||||||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment.
Pull request overview
Refactors the vNext relation query-site recognizer to reuse a shared streaming, bounded SQL lexer implementation, preserving dialect-specific lexical behavior and resource ceilings while reducing duplicated lexer code.
Changes:
- Extracted the bounded, streaming SQL lexer into
src/vnext/bounded-sql-lexer.tswith explicit resource-limit signaling. - Updated
src/vnext/query-site.tsto consumeBoundedSqlLexerand translate lexer resource limits through a local exhaustive map. - Added direct unit tests that validate token boundaries, embedded-region barriers, pushback semantics, and resource-limit behavior.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/vnext/query-site.ts | Replaces the inlined query-site lexer with BoundedSqlLexer and maps lexer resource signals into query-site resources. |
| src/vnext/bounded-sql-lexer.ts | New shared, bounded streaming lexer extracted from the prior query-site implementation. |
| src/vnext/tests/bounded-sql-lexer.test.ts | Adds focused boundary/resource tests for the shared bounded lexer across key dialect behaviors and masking. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
All reported issues were addressed across 3 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
|
Resolved Cubic’s P3 at exact head |
Summary
This is a zero-semantics prerequisite for the separate bounded CTE layout/visibility recognizer. The recognizers will initially use separate streaming traversals over the same lexical implementation; any traversal fusion remains benchmark-driven.
Performance
An initial broader helper extraction caused a reproducible Vite SSR namespace-call regression on the hot path. The boundary was narrowed before commit.
Stable means at the exact head are back at the PR #186 baseline:
USINGcolumns: about 0.21 msThe lexer remains streaming and does not allocate a token tape.
Verification
ed079df0f68aadaf8957a7ca5e6c497e91b74860Part of #169.
Summary by cubic
Refactored vNext to share a streaming bounded SQL lexer and the embedded-region lookup, and switched
query-siteto use them. Behavior is unchanged across PostgreSQL, DuckDB, BigQuery, and Dremio; this unblocks the bounded CTE recognizer in #169.src/vnext/bounded-sql-lexer.ts.findSqlEmbeddedRegionAtOrAfterinsrc/vnext/source.tsand reused it in the lexer andquery-site(with tests).query-site; mapped lexer resource signals to localquery-siteresources.src/vnext/query-site.tsand wired it to the shared modules.Written for commit 76190f9. Summary will update on new commits.