Repository navigation
feat: complete the BigQuery driver for 0.2.0 - #3
Conversation
Dispatch requests concurrently with a single writer and cancel the running job on the host cancel notification. Start idempotent, labelled jobs with a server-side timeout, retry transient failures, fill pages across page tokens, reuse finished jobs for later pages, browse native tables through tabledata.list, map host sessions to BigQuery sessions, return script results, and describe common errors with hints. Add impersonation, an API endpoint override and emulator-only unauthenticated access, and fix the client cache key.
explain_query returns the raw bigquery-job-json payload: dry-run bytes with a labelled on-demand cost estimate, or the executed stage plan for EXPLAIN ANALYZE. The TypeScript parser is declared in explain_parsers and requires Tabularis 0.23.0.
Expose the API endpoint, the service account to impersonate and the emulator-only no-authentication method. Clear the stored credential when switching to a method that does not use one, and default the query timeout to 110 seconds.
Quote every identifier part, escape control characters, and render NUMERIC, BIGNUMERIC, GEOGRAPHY, INTERVAL, RANGE, JSON, BYTES, ARRAY and STRUCT values correctly. Parse rangeElementType and parameterized type fields from table schemas.
Run jobs in the billing project (billing_project extra or database), list projects, datasets, tables and routines for any project, and add run_statement with named query parameters, default dataset and location. Fix the double slash in REST URLs.
List projects as databases, fetch table resources once with bounded concurrency and a short cache, fill descriptions and parameterized types, read DDL from INFORMATION_SCHEMA with a metadata fallback, refresh materialized views, and read and manage routines through the routines REST API with host-compatible parameters.
Reject NOT NULL on added columns, SET NOT NULL and unsupported type changes with clear errors, set column descriptions through OPTIONS, add and drop NOT ENFORCED primary keys, and accept optional partitioning, clustering and description for new tables.
Bind values as query parameters, report real affected-row counts, refuse edits on snapshots and external tables, match keyless rows on comparable columns inside a transaction that rolls back when more than one row matches, and explain streaming-buffer errors.
Deserialize RPC results into mirrors of the host models and add live metadata and CRUD checks gated on BIGQUERY_EMULATOR_HOST.
Enable routine management, complete type mappings, stop requiring NUMERIC precision, add registry metadata, and document multi-project browsing, permissions and BigQuery-specific behaviour.
Adds optional billing_project and additional_projects text fields under Advanced options. The backend that reads them lands separately.
Query, dry-run, session abort and table browsing pick their project through job_project() and data_project(), so a separate billing project can be wired in one place. The client cache key also covers billing_project and additional_projects.
# Conflicts: # .tabularium # README.md # src/client.rs # src/lib.rs # tests/conformance.rs # tests/emulator.rs
…he data project Wire the two project helpers of the query runtime to the multi-project connection settings: jobs run and are billed in `billing_project` (or the GCP Project ID), while `defaultDataset.projectId`, table browsing and `ping` use the selected project. The data project is part of the client cache key. Mocked tests cover the job URL and default dataset, and that row edits reach the job runtime with request IDs, labels, job timeouts, the dataset location and typed named parameters.
Statements run from a query tab that are not plain queries may create, alter or drop objects, so the short-lived metadata cache is cleared once they have run, also after a failure since a script may have applied part of its changes.
Every DDL builder now qualifies objects with the data project from the connection parameters the host sends (falling back to the billing project in all-databases mode, and to dataset-qualified names on older hosts). Add the optional get_drop_table_sql RPC: it looks up the object type and returns DROP TABLE, DROP EXTERNAL TABLE, DROP SNAPSHOT TABLE, DROP VIEW or DROP MATERIALIZED VIEW on the fully qualified name. Clear a column description with SET OPTIONS(description = NULL) when the host omits the new comment. Conformance tests replay the exact host request payloads.
Set min_runtime_version to 0.29.0, the first host with plugin connection fields and secrets, the DDL connection context and parser bundles, and fold the old Visual EXPLAIN requirement into it. Fix the GCP Project ID placeholder, which suggested the billing project.
pnpm 10+ refuses frozen installs with unapproved build scripts and pnpm 11+ no longer reads the pnpm field of package.json. Each package directory gets its own pnpm-workspace.yaml with the build allowlist; the packages entry keeps pnpm 9, pinned in CI, working.
…nto release/0.2.0
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| .map(str::trim) | ||
| .filter(|value| !value.is_empty()) | ||
| { | ||
| output.push_str(" DEFAULT "); |
There was a problem hiding this comment.
CRITICAL: column_sql emits DEFAULT <expr> before NOT NULL, producing invalid GoogleSQL for required columns with defaults
For a column that is both required and defaulted, the emitted clause order is `col` INT64 DEFAULT 0 NOT NULL (the DEFAULT block at lines 381-389 runs before the NOT NULL block at 390-398), but BigQuery's column_schema grammar is column_name type [NOT NULL] [DEFAULT expr] [OPTIONS(...)] — NOT NULL must come first. create_table_sql is the only caller with allow_required = true, so a CREATE TABLE from the column editor with such a column fails at execution with a syntax error. The pre-PR code emitted NOT NULL first, so this is a regression; the new tests never combine the two options (the NOT NULL column has no default, the defaulted column is nullable). Move the DEFAULT block after the NOT NULL block.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
There was a problem hiding this comment.
Fixed in bce4e0f. Column definitions now emit NOT NULL before DEFAULT and OPTIONS, both in the column editor DDL (handlers/ddl.rs) and in the metadata-based table DDL fallback (schema_ddl.rs). Added tests for a required column with a default and a description.
| let page_size = request.page_size; | ||
| let offset = u64::from(request.page.saturating_sub(1)) * u64::from(page_size); | ||
| let session_key = session.as_ref().and_then(|state| state.bigquery_id.clone()); | ||
| let cache_key = self.result_key(request, session_key.as_deref()); |
There was a problem hiding this comment.
WARNING: Result-cache key is computed before the BigQuery session id exists, so paging past page 1 of the first statement in a session silently re-executes the query
cache_key is derived from state.bigquery_id, which is still None for the first statement of a session; the id is only captured later in finish, and the result is stored under the no-session key. When the host requests page 2, bigquery_id is now Some(..), the lookup key differs, lookup_result misses, is_read_only_query passes, and the whole statement runs again as a fresh job. That second execution is then cached under the session key, so page 1 and pages 3+ can come from two different executions that disagree on a mutating table. Consider deferring the store until the session id is known (or keying the cache by the host session id).
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
There was a problem hiding this comment.
Fixed in b38aace. The cache key is now computed after the session id of a newly created BigQuery session is captured, so page 2 of the first statement in a session finds the cached job. Added a mock test asserting only one jobs.query call for pages 1 and 2.
| if has_errors { | ||
| let job_resource = self.get_job(job).await?; | ||
| if let Some(error) = job_resource.pointer("/status/errorResult") { | ||
| return Err(ApiError::from_error_proto(error).describe()); |
There was a problem hiding this comment.
WARNING: A failed createSession job returns before the session id is captured, leaking the BigQuery session server-side
When the job has errors, finish returns here before reaching the session-capture block at lines 296-317. A job submitted with createSession: true may still have created a session, and its id is readable at /statistics/sessionInfo/sessionId in the job resource that was just fetched (the success path falls back to exactly that location). Since the id never lands in state.bigquery_id, session::abort can never run BQ.ABORT_SESSION for it: every failed statement in a session tab orphans a BigQuery session (which only expires after ~24h idle) and the next statement creates yet another fresh one.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
There was a problem hiding this comment.
Fixed in b38aace. On the job error path the session id is captured from statistics.sessionInfo.sessionId (or the query response) before returning the error, so release_session still runs BQ.ABORT_SESSION. Covered by a mock test.
| async fn list_child_jobs(&self, job: &JobRef) -> Result<Vec<Value>, String> { | ||
| let mut children = Vec::new(); | ||
| let mut page_token: Option<String> = None; | ||
| loop { |
There was a problem hiding this comment.
WARNING: list_child_jobs loops forever with unbounded memory growth if the server keeps returning a nextPageToken
The loop only exits when nextPageToken is absent; a misbehaving endpoint (an emulator or proxy repeating a token) makes children grow without bound and the request hang. tabledata.rs in this same PR explicitly distrusts server paging for exactly this reason, so the missing repeated-token guard here is inconsistent with the codebase's own assumptions about endpoints it talks to.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
There was a problem hiding this comment.
Fixed in b38aace. list_child_jobs now fails on a repeated nextPageToken and stops after 100 pages, with a mock test that returns the same token forever.
| let mut tasks = JoinSet::new(); | ||
| let mut lines = reader.lines(); | ||
| while let Ok(Some(line)) = lines.next_line().await { | ||
| while tasks.try_join_next().is_some() {} |
There was a problem hiding this comment.
WARNING: Finished task results are dropped uninspected, so a panicking request handler is silently swallowed and its JSON-RPC request is never answered
while tasks.try_join_next().is_some() {} (and the drain at line 68) discards the Result<_, JoinError> from each completed task. If a handler panics, no response is ever sent for that request id — the host waits until its own call timeout — and the panic is not even logged, though eprintln! is used elsewhere in this file. Inspecting the join result and logging errors would preserve observability.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
There was a problem hiding this comment.
Fixed in 89fa84f. The server tracks the request id per task, inspects each JoinError, logs it to stderr and replies with a JSON-RPC internal error (-32603). A unit test with a panicking handler checks that both the failing and the following request are answered.
| } | ||
|
|
||
| pub async fn abort(state: &mut SessionState) -> Result<(), String> { | ||
| let (Some(client), Some(id)) = (state.client.clone(), state.bigquery_id.take()) else { |
There was a problem hiding this comment.
SUGGESTION: abort clears bigquery_id even when client is None, dropping the session id without aborting the server-side session
Both tuple elements are evaluated before the pattern match, so state.bigquery_id.take() runs regardless; if client is None the else branch returns Ok(()) with the id already erased and abort_session never called. Currently unreachable because the runtime sets client before bigquery_id can be set, but the fields are public and the invariant is implicit — taking the id only after matching would remove the footgun.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
There was a problem hiding this comment.
Fixed in 53339ff. abort now checks for a client first and takes bigquery_id only after that, so the id is never dropped without aborting the session. Added a unit test.
|
|
||
| /// Split a script into statements on top-level semicolons, skipping quoted | ||
| /// strings, quoted identifiers and comments. | ||
| pub fn split(sql: &str) -> Vec<String> { |
There was a problem hiding this comment.
SUGGESTION: split does not track scripting BEGIN...END block nesting, so transaction-control statements inside blocks are invisible to transaction_state_after / ends_transaction
IF x THEN COMMIT; END IF; splits into IF x THEN COMMIT and END IF, and transaction_control matches neither piece, leaving state.in_transaction stale after BigQuery actually ended the transaction. Execution routing is unaffected (may_be_script still detects the script); impact is limited to the host's transaction badge and session decisions for scripts that bury COMMIT/ROLLBACK inside blocks.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
There was a problem hiding this comment.
Fixed in c2b2958. Transaction tracking now scans the script with block depth: statements start after ;, BEGIN, THEN, ELSE, DO, LOOP and REPEAT, but not inside parentheses or CASE expressions. COMMIT/ROLLBACK inside IF ... END IF, loops and exception handlers now update in_transaction. Unit tests cover these cases plus block BEGIN, CASE expressions and quoted names.
| statements.len() == 1 | ||
| && matches!( | ||
| words(&statements[0]).first().map(String::as_str), | ||
| Some("SELECT" | "WITH" | "(") |
There was a problem hiding this comment.
SUGGESTION: The "(" alternative never matches a query written as (SELECT ...) because words() splits on whitespace
For (SELECT 1) the first word is "(SELECT", not "(", so a leading-parenthesis SELECT is treated as non-read-only: every run triggers a full catalog invalidation, and expired cached results cannot be transparently re-run for later pages. Minor UX/cache-efficiency issue only; no wrong data.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
There was a problem hiding this comment.
Fixed in c2b2958. is_read_only_query strips leading parentheses before checking for SELECT or WITH, so (SELECT ...) UNION ALL (SELECT ...) counts as read-only. Tests added.
| *provider.cached.lock().await = None; | ||
| Box::pin(provider.source.invalidate()).await; | ||
| } | ||
| Self::Google(_) | Self::AccessToken(_) | Self::None => {} |
There was a problem hiding this comment.
SUGGESTION: invalidate() is a no-op for AuthProvider::Google, so the "refresh token once after 401" retry reuses the same cached token for ADC and service-account connections
gcp_auth's TokenProvider caches tokens internally and offers no force-refresh here; after a 401 (e.g. a revoked but unexpired token) the retry gets the identical token from gcp_auth's cache and fails again. The error still surfaces correctly to the user, but the doc comment on transport::call ("a 401 refreshes the token once") overstates the behavior for the two most common auth methods.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
There was a problem hiding this comment.
Fixed in 2a898b4. Google (ADC and service account) providers are now wrapped so invalidate() rebuilds the gcp_auth provider, and the retry after a 401 gets a fresh token. The transport::call doc comment now also says a fixed access token cannot be refreshed. Unit test with a fake token provider.
| sleep 2 | ||
| done | ||
| exit 1 | ||
| - run: cargo test --test emulator |
There was a problem hiding this comment.
SUGGESTION: The emulator CI job runs only cargo test --test emulator, so tests/emulator_metadata.rs never executes in CI
Both emulator test binaries are gated on BIGQUERY_EMULATOR_HOST and share this setup; --test emulator selects only one target, so the metadata/DDL/CRUD round-trip tests in emulator_metadata.rs (including the get_drop_table_sql live check added by this PR) are never run.
| - run: cargo test --test emulator | |
| - run: cargo test --test emulator --test emulator_metadata |
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
There was a problem hiding this comment.
Applied in 4e7c4a6: the emulator job now runs cargo test --test emulator --test emulator_metadata. Both suites pass locally against the emulator.
Code Review SummaryStatus: 1 Issue Found | Recommendation: Address before merge Overview
Fix these issues in Kilo Cloud Issue Details (click to expand)WARNING
Files Reviewed (22 files)Incremental review of changes since the previous review (
Previous Review Summary (commit bdae214)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit bdae214)Status: 13 Issues Found | Recommendation: Address before merge Overview
Fix these issues in Kilo Cloud Issue Details (click to expand)CRITICAL
WARNING
SUGGESTION
Files Reviewed (72 files)
Note: the PR head moved from Reviewed by kimi-k3 · Input: 596.7K · Output: 30K · Cached: 903.2K |
# Conflicts: # .github/workflows/ci.yml # .github/workflows/release.yml # .gitignore # .tabularium # CHANGELOG.md # README.md # src/auth.rs # tests/manifest_contract.rs # ui/src/connection-fields.tsx # ui/src/styles.ts
GoogleSQL column_schema is name type [NOT NULL] [DEFAULT expr] [OPTIONS(...)], so a required column with a default produced invalid CREATE TABLE SQL.
…okens - Key the result cache after capturing a newly created BigQuery session, so page 2 of the first statement in a session reuses its job instead of running the statement again. - Capture the session id of a failed createSession job, so releasing the editor tab still aborts the server-side session. - Fail on a repeated pageToken while filling a result page and on a repeated nextPageToken (or more than 100 pages) while listing script child jobs, instead of duplicating rows or looping forever.
Finished tasks were dropped uninspected, so a panicking handler was never logged and its request never answered. Track the request id per task, log the JoinError to stderr and reply with a JSON-RPC internal error.
get_all_columns_batch and get_all_foreign_keys_batch fetched every table of the dataset and failed the whole batch on one error. Filter to the requested tables before fetching and skip (and log) tables that cannot be read.
- Scan scripts with block depth so COMMIT / ROLLBACK / BEGIN TRANSACTION nested in IF, BEGIN...END, loops or exception handlers update the session transaction state, while CASE expressions and parentheses do not start statements. - Treat a query that starts with parentheses, such as (SELECT ...), as read-only.
invalidate() was a no-op for ADC and service account connections because gcp_auth caches tokens internally, so the retry after a 401 sent the same token again. Rebuild the provider instead, so the retry fetches a new token.
Null or non-object query_plan elements (and malformed steps) crashed the parser with a TypeError. Validate stages, inputStages, steps and substeps and throw the parser's usual descriptive errors.
| let mut pending: HashMap<Id, Value> = HashMap::new(); | ||
| let mut lines = reader.lines(); | ||
| while let Ok(Some(line)) = lines.next_line().await { | ||
| while let Some(joined) = tasks.try_join_next_with_id() { |
There was a problem hiding this comment.
WARNING: A panicked handler is only answered once the next input line arrives — an idle host still waits for a reply that is ready but not sent
The task set is drained only inside the read loop (here, before each new line) and once after EOF. When a handler panics, the -32603 response is emitted by settle solely at those points, so if the panicking request is the only one in flight, the host sits waiting for its response until its own call timeout fires — the same host-visible outcome this change set out to eliminate ("its request still gets a JSON-RPC internal error, so the host never waits for a reply that cannot come"). The unit test passes only because it closes stdin (drop(host_write)), which forces the drain. Consider driving the main loop with tokio::select! over lines.next_line() and tasks.join_next_with_id() so a panicked task is settled as soon as it finishes.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
Summary
Completes the BigQuery driver for a 0.2.0 release: a robust query runtime, the everyday workflows the built-in PostgreSQL driver offers, and correct GoogleSQL for editing and DDL. Requires Tabularis 0.29.0 (
min_runtime_version).Query runtime
cancelnotification cancels the BigQuery job (jobs.cancel), and notifications never get a reply.requestId(retries cannot run a statement twice),jobTimeoutMs, labels, and run in the billing project. Default timeout is 110 s, below the host call timeout.rateLimitExceeded/backendErrorwith jitter; one token refresh after 401; failure judged bystatus.errorResult.pageTokenand reuses the finished job instead of re-running the SQL for every page (exports included). Timestamps are exact (int64).SELECT * FROM table [LIMIT/OFFSET]on native tables is served by the freetabledata.list.session_id(in_transaction,release_session), soBEGIN TRANSACTION,DECLAREand temp tables survive between runs. Scripts return the last result and the summed DML row count.Workflows
get_databasesfromprojects.list, plusadditional_projectssuch asbigquery-public-dataand a separatebilling_project), datasets as schemas, fully qualified generated SQL.routine_management).get_table_ddl, view and materialized view definitions fromINFORMATION_SCHEMA(OPTIONS, partitioning and clustering preserved), materialized view refresh.SET NOT NULL, noADD COLUMN ... NOT NULL, widening-only type changes,NOT ENFORCEDkeys), column descriptions (column_comments), andget_drop_table_sqlwith the right keyword for snapshots, external tables, views and materialized views (drop_table_sql).Connection and auth
impersonate_service_accountoption;token_urirestricted to Google endpoints.httpandnoneauth only on loopback), billing project, additional projects.Manifest
sql_syntaxdeclares GoogleSQL (backtick identifiers, triple-quoted and raw strings,#comments, procedural blocks, backslash literals,TRUE/FALSEbooleans, BigQuery formatter, keywords and functions), withsql_dialect: mysqlas the fallback for older hosts.min_runtime_version: 0.29.0,routine_management,column_comments,drop_table_sql,explain_parsers, completetype_mappings, registry metadata. Validates against the live registry.Host changes: TabularisDB/tabularis#629, TabularisDB/tabularis#1199, TabularisDB/tabularis#1200.
Tests
BIGQUERY_EMULATOR_HOST, run in CI as a service container).cargo fmt --check,cargo clippy -D warnings, UI and explain bundle typecheck, build and tests pass.@@row_countin the keyless edit guard andINFORMATION_SCHEMA.TABLES.ddl(the emulator does not support them). To be checked before tagging.