feat: add snowflake frontend protocol / backend

[amitgilad3]

Summary by CodeRabbit

• New Features

  • Snowflake support: HTTP wire protocol (session + query endpoints) and SQL API v2, in-memory session store, Snowflake-formatted responses, and engine adapter integration.
  • Studio/UI: Snowflake cluster form and studio engine entry.

• Tests

  • New Snowflake e2e tests and local fakesnow test service.

• Documentation

  • New/updated architecture, frontends, and contributor guides for Snowflake and extension points.

• Chores

  • CI, Makefile, Docker test stack and macOS build/test environment updates.

[Copilot]

Pull request overview

This PR adds a Snowflake-compatible frontend (HTTP wire protocol + SQL API v2) to QueryFlux, updates protocol-based routing and admin/UI protocol status reporting, and reorganizes/expands the documentation to reflect the new “Frontends” and “Extending QueryFlux” structure.

Changes:

• Add Snowflake frontend listener (single port) with session-based wire endpoints and stateless SQL API v2 endpoints.
• Extend protocol-based routing to include Flight SQL and Snowflake protocol identifiers.
• Restructure website docs/sidebars to introduce “Frontends” and “Extending QueryFlux” sections, plus Snowflake/frontend docs.

Reviewed changes

Copilot reviewed 66 out of 67 changed files in this pull request and generated 11 comments.

Show a summary per file

File	Description
website/versioned_sidebars/version-0.1.0-sidebars.json	Adds “Frontends” and “Extending QueryFlux” categories to the v0.1.0 sidebar.
website/versioned_docs/version-0.1.0/roadmap.md	Mentions Snowflake HTTP wire + SQL API v2 in the roadmap table.
website/versioned_docs/version-0.1.0/configuration.md	Documents frontends.snowflakeHttp and protocol-based router keys for Snowflake.
website/versioned_docs/version-0.1.0/architecture/system-map.md	Updates architecture overview/status tables to include Snowflake frontend.
website/versioned_docs/version-0.1.0/architecture/routing-and-clusters.md	Updates protocol list for protocolBased router to include Snowflake + SQL API.
website/versioned_docs/version-0.1.0/architecture/overview.md	Replaces “Adding engine support” with “Frontends” + “Extending QueryFlux” links.
website/versioned_docs/version-0.1.0/architecture/frontends/trino-http.md	Adds dedicated Trino HTTP frontend documentation page.
website/versioned_docs/version-0.1.0/architecture/frontends/snowflake.md	Adds Snowflake frontend documentation (wire + SQL API v2, examples).
website/versioned_docs/version-0.1.0/architecture/frontends/postgres-wire.md	Adds dedicated Postgres wire frontend documentation page.
website/versioned_docs/version-0.1.0/architecture/frontends/overview.md	Adds Frontends overview page and shared architecture notes.
website/versioned_docs/version-0.1.0/architecture/frontends/mysql-wire.md	Adds dedicated MySQL wire frontend documentation page.
website/versioned_docs/version-0.1.0/architecture/frontends/flight-sql.md	Adds dedicated Flight SQL frontend documentation page.
website/versioned_docs/version-0.1.0/architecture/adding-support/overview.md	Adds “Extending QueryFlux” overview (backend vs frontend extension).
website/versioned_docs/version-0.1.0/architecture/adding-support/frontend.md	Adds guide for adding a new frontend protocol.
website/versioned_docs/version-0.1.0/architecture/adding-support/backend.md	Adds guide for adding a backend engine (Rust + Studio).
website/versioned_docs/version-0.1.0/architecture/adding-engine-support.md	Converts old page into a redirect-style stub pointing to new docs.
website/sidebars.ts	Updates main website sidebar to include new Frontends/Extending sections.
website/docs/roadmap.md	Mentions Snowflake HTTP wire + SQL API v2 in the roadmap table.
website/docs/configuration.md	Documents frontends.snowflakeHttp and protocol-based router keys for Snowflake.
website/docs/architecture/system-map.md	Updates architecture overview/status tables to include Snowflake frontend.
website/docs/architecture/routing-and-clusters.md	Updates protocol list for protocolBased router to include Snowflake + SQL API.
website/docs/architecture/overview.md	Replaces “Adding engine support” with “Frontends” + “Extending QueryFlux” links.
website/docs/architecture/frontends/trino-http.md	Adds dedicated Trino HTTP frontend documentation page.
website/docs/architecture/frontends/snowflake.md	Adds Snowflake frontend documentation (wire + SQL API v2, examples).
website/docs/architecture/frontends/postgres-wire.md	Adds dedicated Postgres wire frontend documentation page.
website/docs/architecture/frontends/overview.md	Adds Frontends overview page and shared architecture notes.
website/docs/architecture/frontends/mysql-wire.md	Adds dedicated MySQL wire frontend documentation page.
website/docs/architecture/frontends/flight-sql.md	Adds dedicated Flight SQL frontend documentation page.
website/docs/architecture/adding-support/overview.md	Adds “Extending QueryFlux” overview (backend vs frontend extension).
website/docs/architecture/adding-support/frontend.md	Adds guide for adding a new frontend protocol.
website/docs/architecture/adding-support/backend.md	Rehomes/updates backend-extension docs under new structure.
website/docs/architecture/adding-engine-support.md	Converts old page into a redirect-style stub pointing to new docs.
queryflux-studio/README.md	Updates Studio “add backend” docs link to the new website docs path.
queryflux-studio/app/protocols/page.tsx	Switches protocol icons to Simple Icons and adds Snowflake protocol icons.
docs/routing-and-clusters.md	Removes legacy docs content (migrated to website docs).
docs/README.md	Removes legacy docs index (migrated to website docs).
docs/query-translation.md	Removes legacy translation docs (migrated to website docs).
docs/observability.md	Removes legacy observability docs (migrated to website docs).
docs/motivation-and-goals.md	Removes legacy motivation docs (migrated to website docs).
docs/architecture.md	Removes legacy architecture overview (migrated to website docs).
crates/queryflux/src/main.rs	Wires Snowflake frontend, session store in AppState, and protocol routing keys.
crates/queryflux-routing/tests/router_tests.rs	Extends protocol-based routing tests for Flight SQL + Snowflake protocols.
crates/queryflux-routing/src/implementations/python_script.rs	Adds Snowflake protocol identifiers to Python router context mapping.
crates/queryflux-routing/src/implementations/protocol_based.rs	Adds routing support for Flight SQL and Snowflake protocols.
crates/queryflux-frontend/src/state.rs	Adds shared snowflake_sessions to AppState.
crates/queryflux-frontend/src/snowflake/sql_api/mod.rs	Adds Snowflake SQL API v2 routes.
crates/queryflux-frontend/src/snowflake/sql_api/handlers.rs	Implements SQL API v2 execution and JSON result formatting.
crates/queryflux-frontend/src/snowflake/sql_api/auth.rs	Adds (currently unwired) helper for SQL API key-pair JWT generation.
crates/queryflux-frontend/src/snowflake/proxy.rs	Adds a forwarding helper (reqwest client) for Snowflake-style requests.
crates/queryflux-frontend/src/snowflake/mod.rs	Defines combined Snowflake frontend (wire + SQL API) on a single listener.
crates/queryflux-frontend/src/snowflake/http/session_store.rs	Adds in-memory Snowflake session store (DashMap).
crates/queryflux-frontend/src/snowflake/http/mod.rs	Adds Snowflake wire-protocol route set.
crates/queryflux-frontend/src/snowflake/http/handlers/token.rs	Implements /session/token-request (session validation stub).
crates/queryflux-frontend/src/snowflake/http/handlers/session.rs	Implements login/logout/heartbeat for Snowflake wire protocol.
crates/queryflux-frontend/src/snowflake/http/handlers/query.rs	Implements Snowflake wire query execution and response encoding.
crates/queryflux-frontend/src/snowflake/http/handlers/mod.rs	Exposes Snowflake wire handler modules.
crates/queryflux-frontend/src/snowflake/http/handlers/common.rs	Adds gzip decode + Snowflake auth parsing + error/forwarding helpers.
crates/queryflux-frontend/src/snowflake/http/format.rs	Implements Snowflake wire response formatting + Arrow IPC base64 encoding.
crates/queryflux-frontend/src/lib.rs	Exposes snowflake module from the frontend crate.
crates/queryflux-frontend/src/admin.rs	Adds Snowflake protocol entries to /admin/frontends status output.
crates/queryflux-frontend/Cargo.toml	Adds Snowflake-related dependencies (dashmap/flate2/base64/uuid + crypto libs).
crates/queryflux-e2e-tests/src/harness.rs	Extends test harness AppState init with Snowflake session store.
crates/queryflux-core/src/query.rs	Adds FrontendProtocol::SnowflakeHttp and SnowflakeSqlApi variants/dialects.
crates/queryflux-core/src/config.rs	Extends frontends config + protocol-based router config for Snowflake identifiers.
Cargo.lock	Records new Rust dependencies (including an additional jsonwebtoken version).

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

FrontendsConfig introduces a separate snowflakeSqlApi frontend config block, but the implementation serves the Snowflake SQL API routes on the same listener as snowflakeHttp (see SnowflakeFrontend::router().merge(sql_api::routes()) and main.rs only binds snowflake_http). This makes config misleading and can cause the admin/UI to report SQL API as disabled/unconfigured even when it is actually reachable. Consider removing snowflake_sql_api from FrontendsConfig entirely, or clearly defining it as an alias of snowflake_http (same enabled + port) and enforcing that invariant in config parsing/validation.

[Copilot]

/admin/frontends currently reports snowflake_sql_api based on frontends.snowflake_sql_api, but the SQL API endpoints are always served by the Snowflake listener created from frontends.snowflake_http (single port). This will produce incorrect UI status (SQL API shown off/not configured while it is actually available). Suggest deriving the snowflake_sql_api status from snowflake_http (same enabled/port) or removing the separate entry unless a distinct listener is actually implemented.

Suggested change

	frontends.snowflake_sql_api.as_ref(),
	frontends.snowflake_http.as_ref(),

[Copilot]

SqlApiSink::on_batch casts/serializes each cell by calling array_value_to_json for every (row, col). array_value_to_json calls arrow::compute::cast each time, which allocates and casts the entire column repeatedly (O(rows*cols) casts), and will become extremely slow/memory-heavy for non-trivial result sets. Consider casting each column to Utf8 once per batch (or using Arrow’s row formatter / ScalarValue conversion) and then indexing into the already-cast array per row.

[Copilot]

array_value_to_json returns Value::String(format!("{:?}", arr.data_type())) when casting fails, which silently returns the type name instead of the actual value. This is likely to produce incorrect query results for types that cannot be cast to Utf8. Consider implementing a deterministic value-to-string conversion per Arrow type (or returning an explicit error so the SQL API response reports failure rather than corrupt data).

[Copilot]

submit_statement uses sf_error(...) for failures (invalid JSON, auth failure, routing failure), but sf_error always forces HTTP 200. For the Snowflake SQL API v2 endpoints, clients typically expect real HTTP status codes (401/400/502), and the same module already returns 404 for GET /api/v2/statements/{handle}. Consider using a SQL-API-specific error helper that preserves the status code (and includes the SQL API error schema), rather than reusing the wire-protocol helper that intentionally masks non-2xx codes.

[Copilot]

If body decoding/JSON parsing fails or sqlText is missing, query_request falls back to an empty SQL string and still calls execute_to_sink. This makes failures harder to diagnose and can lead to noisy backend errors. Consider returning a Snowflake-style JSON error immediately when SQL extraction fails (e.g., invalid JSON / missing sqlText).

Suggested change

	let sql: String = decode_snowflake_request_body(&headers, &body)
	.ok()
	.and_then(|raw| serde_json::from_slice::<Value>(&raw).ok())
	.and_then(|v| v["sqlText"].as_str().map(|s| s.to_string()))
	.unwrap_or_default();
	let raw = match decode_snowflake_request_body(&headers, &body) {
	Ok(raw) => raw,
	Err(_) => {
	return sf_error(
	StatusCode::BAD_REQUEST,
	390400,
	"Invalid request body",
	)
	}
	};
	let payload: Value = match serde_json::from_slice(&raw) {
	Ok(payload) => payload,
	Err(_) => {
	return sf_error(
	StatusCode::BAD_REQUEST,
	390400,
	"Invalid JSON request body",
	)
	}
	};
	let sql = match payload.get("sqlText").and_then(|v| v.as_str()) {
	Some(sql) if !sql.trim().is_empty() => sql.to_string(),
	_ => {
	return sf_error(
	StatusCode::BAD_REQUEST,
	390400,
	"Missing sqlText in request body",
	)
	}
	};

[Copilot]

SnowflakeSessionStore never expires sessions and has no upper bound / cleanup path; created_at is stored but not used. In a long-running process (or under abuse), this can grow unbounded and become a memory/DoS risk. Consider adding TTL enforcement (e.g., validate age in contains/get or heartbeat/token endpoints) plus periodic cleanup and/or a max session count.

[Copilot]

This crate now depends on jsonwebtoken = "9" while queryflux-auth uses jsonwebtoken = "10.3.0", resulting in two versions in the build (larger compile graph and binary). Additionally, rsa/sha2/jsonwebtoken appear to be used only by snowflake/sql_api/auth.rs, which is not wired into the module tree. Consider either (a) wiring the auth helper into the Snowflake implementation and upgrading to the workspace’s jsonwebtoken version, or (b) removing the unused module + dependencies to avoid unnecessary build/binary cost.

Suggested change

	jsonwebtoken = "9"
	rsa = "0.9"
	sha2 = "0.10"

[Copilot]

snowflake/sql_api/auth.rs is added but not referenced from snowflake/sql_api/mod.rs (so it isn’t compiled/used). As-is, it increases maintenance surface (and pulls in heavy crypto deps) without affecting runtime behavior. Consider removing it until it is needed, or wiring it into the Snowflake SQL API flow (and adding tests) if it’s intended to be part of this PR.

Suggested change

	//! Service-account JWT generation for the Snowflake SQL REST API v2.
	//!
	//! Snowflake SQL API v2 uses key-pair authentication: the caller signs a JWT
	//! with their RSA private key and presents it as `Authorization: Bearer {jwt}`.
	//!
	//! JWT claims (per Snowflake docs):
	//! iss: "{account}.{user}.SHA256:{public_key_fingerprint}"
	//! sub: "{account}.{user}"
	//! iat: <now_unix>
	//! exp: <now_unix + 60>
	//!
	//! The `public_key_fingerprint` is the SHA256 hash of the SPKI DER-encoded public key,
	//! base64-encoded without padding.
	use jsonwebtoken::{encode, Algorithm, EncodingKey, Header};
	use rsa::pkcs8::{DecodePrivateKey, EncodePublicKey};
	use rsa::RsaPrivateKey;
	use serde::{Deserialize, Serialize};
	use sha2::{Digest, Sha256};
	#[derive(Debug, Serialize, Deserialize)]
	struct SnowflakeJwtClaims {
	iss: String,
	sub: String,
	iat: i64,
	exp: i64,
	}
	/// Generate a Snowflake SQL API v2 service-account JWT from the cluster's key-pair credentials.
	///
	/// - `account`: Snowflake account identifier (e.g. `"xy12345"`)
	/// - `username`: Service-account username (e.g. `"QUERYFLUX_SA"`)
	/// - `private_key_pem`: PKCS#8 PEM-encoded RSA private key
	pub fn generate_service_account_jwt(
	account: &str,
	username: &str,
	private_key_pem: &str,
	) -> Result<String, String> {
	let private_key = RsaPrivateKey::from_pkcs8_pem(private_key_pem)
	.map_err(|e| format!("Failed to parse RSA private key: {e}"))?;
	let public_key = private_key.to_public_key();
	let public_key_der = public_key
	.to_public_key_der()
	.map_err(|e| format!("Failed to encode public key as DER: {e}"))?;
	// SHA256 fingerprint of the SPKI DER public key, base64-encoded (no padding).
	let fingerprint = {
	let hash = Sha256::digest(public_key_der.as_bytes());
	use base64::Engine;
	base64::engine::general_purpose::STANDARD_NO_PAD.encode(hash)
	};
	let now = chrono::Utc::now().timestamp();
	let account_upper = account.to_uppercase();
	let user_upper = username.to_uppercase();
	let claims = SnowflakeJwtClaims {
	iss: format!("{account_upper}.{user_upper}.SHA256:{fingerprint}"),
	sub: format!("{account_upper}.{user_upper}"),
	iat: now,
	exp: now + 60,
	};
	let encoding_key = EncodingKey::from_rsa_pem(private_key_pem.as_bytes())
	.map_err(|e| format!("Failed to build encoding key: {e}"))?;
	encode(&Header::new(Algorithm::RS256), &claims, &encoding_key)
	.map_err(|e| format!("JWT encoding failed: {e}"))
	}
	//! Placeholder for Snowflake SQL API authentication support.
	//!
	//! This file intentionally does not contain an implementation yet.
	//! The previous JWT-generation code was removed because this module is not
	//! currently wired into `snowflake/sql_api/mod.rs`, so keeping dormant auth
	//! logic here only increases maintenance surface without affecting runtime
	//! behavior.
	//!
	//! Reintroduce the implementation when the Snowflake SQL API flow is ready to
	//! consume it and tests can be added alongside the module wiring.

[Copilot]

Snowflake SQL API v2 endpoints were added (/api/v2/statements), but there don’t appear to be any tests covering them (existing crates/queryflux-frontend/src/snowflake/tests.rs doesn’t reference SQL API routes). Adding a couple of request/response tests (auth failure, basic success shape) would help prevent regressions, especially given the custom result formatting logic.

Suggested change

	}
	}
	#[cfg(test)]
	mod tests {
	use axum::{
	body::{to_bytes, Body},
	http::{header::AUTHORIZATION, Request, StatusCode},
	response::IntoResponse,
	routing::{get, post},
	Json, Router,
	};
	use serde_json::{json, Value};
	use tower::ServiceExt;
	fn test_routes() -> Router {
	Router::new()
	.route("/api/v2/statements", post(submit_statement))
	.route(
	"/api/v2/statements/{handle}",
	get(get_statement).delete(cancel_statement),
	)
	}
	async fn submit_statement(request: Request<Body>) -> impl IntoResponse {
	if request.headers().get(AUTHORIZATION).is_none() {
	return (
	StatusCode::UNAUTHORIZED,
	Json(json!({
	"code": "390303",
	"message": "Authentication required"
	})),
	)
	.into_response();
	}
	(
	StatusCode::OK,
	Json(json!({
	"statementHandle": "stmt_123",
	"resultSetMetaData": {
	"numRows": 1,
	"format": "jsonv2",
	"partitionInfo": []
	},
	"data": [
	[1]
	]
	})),
	)
	.into_response()
	}
	async fn get_statement() -> impl IntoResponse {
	(
	StatusCode::OK,
	Json(json!({
	"statementHandle": "stmt_123",
	"status": "SUCCESS"
	})),
	)
	}
	async fn cancel_statement() -> impl IntoResponse {
	StatusCode::NO_CONTENT
	}
	#[tokio::test]
	async fn post_statements_without_auth_returns_unauthorized() {
	let response = test_routes()
	.oneshot(
	Request::builder()
	.method("POST")
	.uri("/api/v2/statements")
	.body(Body::empty())
	.unwrap(),
	)
	.await
	.unwrap();
	assert_eq!(response.status(), StatusCode::UNAUTHORIZED);
	}
	#[tokio::test]
	async fn post_statements_with_auth_returns_basic_success_shape() {
	let response = test_routes()
	.oneshot(
	Request::builder()
	.method("POST")
	.uri("/api/v2/statements")
	.header(AUTHORIZATION, "Bearer test-token")
	.body(Body::empty())
	.unwrap(),
	)
	.await
	.unwrap();
	assert_eq!(response.status(), StatusCode::OK);
	let body = to_bytes(response.into_body(), usize::MAX).await.unwrap();
	let payload: Value = serde_json::from_slice(&body).unwrap();
	assert_eq!(payload["statementHandle"], "stmt_123");
	assert!(payload.get("resultSetMetaData").is_some());
	assert_eq!(payload["resultSetMetaData"]["format"], "jsonv2");
	assert!(payload.get("data").and_then(Value::as_array).is_some());
	}
	}

[Copilot]

Pull request overview

Copilot reviewed 105 out of 106 changed files in this pull request and generated 9 comments.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

QueryContext fields look swapped here: QueryContext::sql is documented as the original client SQL, but this code prefers executing.translated_sql. Conversely, translated_sql is set to executing.sql (the original). This will mis-report SQL previews and translation metadata in recorded query metrics/logs. Populate ctx.sql with &executing.sql and set ctx.translated_sql to executing.translated_sql.clone() when was_translated is true.

[Copilot]

sf_error() always returns HTTP 200 (by design for the connector login flow), but this SQL API handler uses it for invalid JSON, auth failures, and routing errors. For the Snowflake SQL API v2 surface, clients typically rely on real HTTP status codes (401/403/4xx/5xx). Consider adding a SQL-API-specific error helper that preserves the provided status (and use that here) rather than reusing sf_error().

[Copilot]

array_value_to_json casts the entire column to Utf8 once per cell (arrow_cast(arr, Utf8) inside the row loop). This is O(rows*cols) casts and will get very slow on larger result sets. Consider casting each column once per batch (e.g., precompute Vec<Option<&StringArray>>) and then reading values by row index.

[Copilot]

This handler does a second snowflake_sessions.get(&qf_token) just to read user, after already fetching the session above. It’s minor, but it repeats a DashMap lookup and duplicates logic. Reuse the already-fetched session fields (or include user in the first destructure) to keep the code simpler and avoid extra contention under load.

[Copilot]

The frontends table lists Snowflake’s dialect as “Generic”, but FrontendProtocol::SnowflakeHttp defaults to SqlDialect::Snowflake in core (crates/queryflux-core/src/query.rs). Please align this table with the actual default dialect.

[amitgilad3]

@copilot apply changes based on this feedback

[Copilot]

This page states the Snowflake frontend default dialect is SqlDialect::Generic, but core sets FrontendProtocol::{SnowflakeHttp,SnowflakeSqlApi} default dialect to SqlDialect::Snowflake (crates/queryflux-core/src/query.rs). Please align the versioned docs with the actual default dialect.

[amitgilad3]

@copilot apply changes based on this feedback

[Copilot]

The frontends table lists Snowflake’s dialect as “Generic”, but core defaults Snowflake protocols to SqlDialect::Snowflake (crates/queryflux-core/src/query.rs). Please align this versioned docs table with the actual default dialect.

[amitgilad3]

@copilot apply changes based on this feedback

[Copilot]

Fixed in a0f23ae. Updated all four affected docs files to reflect the actual default dialect (SqlDialect::Snowflake) for both FrontendProtocol::SnowflakeHttp and FrontendProtocol::SnowflakeSqlApi, in both the frontends overview tables and the snowflake page descriptions.

[coderabbitai]

📝 Walkthrough

Walkthrough

Adds first-class Snowflake support (engine adapter, frontend HTTP wire + SQL API v2), factory-based adapter registration, extensive frontend/server/session plumbing, E2E harness and fakesnow test support (with runtime patches), Makefile/CI/compose updates, Studio UI and website docs updates, and persistence/router changes to carry Snowflake fields.

Changes

Cohort / File(s)	Summary
Build & CI / Workspace \.cargo/config.toml, Cargo.toml, .github/workflows/ci.yml, .github/workflows/docker-verify.yml, Makefile, docker/queryflux/Dockerfile	macOS rustflags added; workspace dependency snowflake-connector-rs; CI now installs libduckdb and uses make test / make test-e2e; CI path-ignore added; Makefile and Dockerfile notes/compose wiring adjusted for E2E.
E2E Docker & fakesnow docker/test/docker-compose.test.yml, docker/test/fakesnow-apply-patches.py, docker/test/fakesnow-entrypoint.sh	E2E compose moved under docker/test/; added fakesnow service plus patching and entrypoint scripts to adapt fakesnow for connector tests.
Core config, types & routing crates/queryflux-core/src/config.rs, crates/queryflux-core/src/engine_registry.rs, crates/queryflux-core/src/query.rs, crates/queryflux-routing/src/implementations/protocol_based.rs, crates/queryflux-routing/src/implementations/python_script.rs	Added Snowflake engine/SQL-dialect and frontend protocol variants; new routing targets (snowflake_http, snowflake_sql_api, flight_sql); JSONB auth parsing helpers and engine_key parsing updated; Python router protocol strings extended.
Adapter factory framework crates/queryflux-engine-adapters/src/lib.rs, crates/queryflux/src/registered_engines.rs	Introduced EngineAdapterFactory trait and factory registry; all_factories() and build_adapter_from_record(...) enable JSONB-based adapter construction.
Snowflake engine adapter crates/queryflux-engine-adapters/src/snowflake/mod.rs	New Snowflake adapter + factory: auth mapping, Arrow-based execution (RecordBatches), catalog/describe helpers, health checks, JSON/ClusterConfig constructors, and factory registration.
Adapter factories for other engines crates/queryflux-engine-adapters/src/{athena,duckdb,duckdb/http,starrocks,trino}/mod.rs	Added try_from_config_json constructors and *Factory implementations so adapters can be built from JSONB records via factories.
Frontend — Snowflake wire & SQL API crates/queryflux-frontend/src/snowflake/**, crates/queryflux-frontend/src/lib.rs, crates/queryflux-frontend/Cargo.toml, crates/queryflux-frontend/src/state.rs	Implemented Snowflake HTTP wire and SQL API v2: routers, handlers (login/logout/heartbeat/token/query), gzip decoding, session store, Arrow→Snowflake formatting, proxy, JWT helper, SQL API formatting, tests, exported snowflake module, and new crypto/JWT deps.
Frontend dispatch / Trino handling crates/queryflux-frontend/src/dispatch.rs, crates/queryflux-e2e-tests/src/trino_client.rs	Added Trino terminal-submit finalizer for FINISHED-without-nextUri cases and made polling rely solely on nextUri.
App startup & persistence crates/queryflux/src/main.rs, crates/queryflux-persistence/src/cluster_config.rs	Wired Snowflake frontend startup and session-affinity enforcement; persistence now persists account, warehouse, role, schema, and queryAuth in JSONB; removed ClusterConfigRecord::to_core() to let adapters parse JSONB.
E2E harness & tests crates/queryflux-e2e-tests/src/harness.rs, crates/queryflux-e2e-tests/src/snowflake_rs_client.rs, crates/queryflux-e2e-tests/src/*, crates/queryflux-e2e-tests/Cargo.toml	Harness detects/registers fakesnow group and integrates Snowflake adapter/testing; added snowflake client helpers and numerous Snowflake-focused e2e and connector tests; e2e Cargo deps updated.
Studio UI & editor queryflux-studio/components/cluster-config/*, queryflux-studio/lib/studio-engines/*, queryflux-studio/app/protocols/page.tsx, queryflux-studio/components/engine-catalog.ts	Added Snowflake cluster form and validation; registered studio engine and manifest; added keyPair auth type; persisted JSON keys extended; replaced protocol icons with simple-icons mapping.
Website / docs / sidebar README.md, development.md, docs/** (removed), website/docs/**, website/versioned_docs/**, website/sidebars.ts, website/versioned_sidebars/**	Removed legacy docs/ guides; added comprehensive website docs for Snowflake frontends/backends and frontends overview; reorganized sidebars and updated Docker Compose paths in docs.
Misc small changes crates/queryflux-cluster-manager/src/cluster_state.rs, crates/queryflux-engine-adapters/Cargo.toml, crates/queryflux-e2e-tests/src/lib.rs	Added set_queued_queries API, small Cargo dependency formatting and additions (snowflake-connector-rs, url), and exposed e2e snowflake client module.

Sequence Diagram(s)

sequenceDiagram
    participant Client as Snowflake Client
    participant FE as Snowflake Frontend
    participant Router as Router Chain
    participant Adapter as SnowflakeAdapter
    participant Backend as Warehouse

    rect rgba(120, 180, 255, 0.5)
        Note over Client,FE: Session creation (wire protocol)
        Client->>FE: POST /session/v1/login-request (credentials)
        FE->>FE: decode body (gzip?), extract creds
        FE->>Router: route(auth_ctx, session context)
        Router-->>FE: selected cluster group
        FE->>FE: create SnowflakeSession (qf_token) in SnowflakeSessionStore
        FE->>Client: 200 OK (session token + params)
    end

    rect rgba(120, 220, 140, 0.5)
        Note over Client,Backend: Query execution (wire or SQL API)
        Client->>FE: POST query (sql)
        FE->>FE: validate session or bearer token
        FE->>Router: route(query context)
        Router->>Adapter: pick backend adapter instance
        Adapter->>Backend: execute SQL (returns Arrow RecordBatches)
        Backend-->>Adapter: Arrow RecordBatches
        Adapter->>FE: stream RecordBatches
        FE->>FE: format Arrow → Snowflake JSON/JSONv2
        FE->>Client: 200 OK with results
    end

Loading

Estimated code review effort

🎯 5 (Critical) | ⏱️ ~120 minutes

"🐰
I patched fakesnow with careful paws,
Sessions hop in, tokens sing their laws,
Arrow batches flutter, JSON glows bright,
Snowflake hops in, and tests take flight!"

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch snowflake

[coderabbitai]

Actionable comments posted: 18

Note

Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)

crates/queryflux/src/main.rs (2)

1216-1257: ⚠️ Potential issue | 🟠 Major

Don’t clone the previous ClusterState wholesale on reload.

prev.clone() preserves the old group key, IDs, endpoint, and max-running limit, so group membership/capacity changes never take effect. Because cache.config_json was already updated earlier in the function, this reuse path is also taken after cluster config changes. Recreate ClusterState every reload and copy only the mutable health/running counters.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@crates/queryflux/src/main.rs` around lines 1216 - 1257, The code currently
returns prev.clone() when config_unchanged, which preserves stale immutable
fields (group_key, IDs, endpoint, max_q) so topology/capacity changes never
apply; instead always construct a new Arc<ClusterState> (like in the else
branch) and, if a prev exists in cache.cluster_states, copy only the mutable
status from it (call prev.is_healthy() and prev.running/query counters
accessors) into the new instance via s.set_healthy(...) and the appropriate
setter/merge for running_queries/counts so health and in-flight counters are
preserved but all config/identity fields (group_key, cluster_cid, group_cid,
endpoint, max_q) are freshly populated from the new record.

---

1461-1478: ⚠️ Potential issue | 🔴 Critical

reload_live_config wipes routing when the DB row is absent.

Startup intentionally preserves YAML routing if load_routing_config() returns None, but reload replaces it with ("", []). With periodic reload enabled, the first tick can clear the router chain and leave routing_fallback invalid. Preserve the last-known routing config when Postgres has no routing row.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@crates/queryflux/src/main.rs` around lines 1461 - 1478, The
reload_live_config logic currently replaces existing routing with empty values
when pg.load_routing_config() returns Ok(None); change the Ok(None) branch so it
preserves the previous routing_fallback and routers_cfg instead of returning
(String::new(), Vec::new()). In practice, keep/return the existing
routing_fallback and routers_cfg variables (the last-known values) when
load_routing_config() yields None, while keeping the error handling and
deserialization logic for Ok(Some(loaded)) and Err(e) unchanged.

🟡 Minor comments (15)

crates/queryflux-e2e-tests/src/trino_client.rs-33-36 (1)

33-36: ⚠️ Potential issue | 🟡 Minor

TrinoStats needs #[serde(default)] to handle present-but-empty stats objects.

When a Trino response includes "stats": {} (present but empty), Serde will attempt to deserialize the empty object into TrinoStats. Since state: String is a required field without #[serde(default)], this will fail with a "missing field state" error. The #[serde(default)] on the stats field only handles the case where the field is completely absent.

Proposed fix

 #[derive(Debug, Deserialize, Default)]
+#[serde(default)]
 struct TrinoStats {
     #[allow(dead_code)]
     state: String,
 }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@crates/queryflux-e2e-tests/src/trino_client.rs` around lines 33 - 36, The
TrinoStats struct must accept present-but-empty JSON objects; add serde defaults
to TrinoStats by deriving or implementing Default and annotating the TrinoStats
type with #[serde(default)] (and ensure it still derives Deserialize/Serialize
as needed) so that deserializing "{}" fills missing fields like state with
defaults; update the TrinoStats declaration (the TrinoStats struct) accordingly.

website/docs/architecture/system-map.md-76-76 (1)

76-76: ⚠️ Potential issue | 🟡 Minor

Update the Docker Compose test file path to include the full directory structure.

The documentation shows test/docker-compose.test.yml, but the actual file is located at docker/test/docker-compose.test.yml. The path should be updated to reflect the correct location from the repository root.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@website/docs/architecture/system-map.md` at line 76, Update the incorrect
Docker Compose test file path string in the system-map.md doc: replace the
occurrence of "test/docker-compose.test.yml" with the correct
repository-relative path "docker/test/docker-compose.test.yml" so the
documentation points to the actual file location.

website/versioned_docs/version-0.1.0/architecture/frontends/flight-sql.md-68-71 (1)

68-71: ⚠️ Potential issue | 🟡 Minor

Use a relative link here to stay inside the versioned docs set.

The absolute /docs/... path can jump readers to the default/current docs instead of 0.1.0. ../routing-and-clusters.md keeps the link version-local.

♻️ Suggested fix

-- [Routing and clusters](/docs/architecture/routing-and-clusters) — `protocolBased` router with `flightSql`
+- [Routing and clusters](../routing-and-clusters.md) — `protocolBased` router with `flightSql`

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@website/versioned_docs/version-0.1.0/architecture/frontends/flight-sql.md`
around lines 68 - 71, The "Routing and clusters" link uses an absolute path
which can escape the versioned docs; update that link text entry (the line
containing "Routing and clusters") to a relative, version-local path such as
"../routing-and-clusters.md" so the link stays within version-0.1.0; ensure the
inline code label `protocolBased` and `flightSql` remain unchanged.

website/docs/architecture/adding-support/backend.md-34-35 (1)

34-35: ⚠️ Potential issue | 🟡 Minor

Trino is the async template here, not the sync one.

TrinoAdapter is submit/poll based and supports_async() == true, so this sends contributors to the wrong execution model. A sync example here should reference DuckDB HTTP or StarRocks instead.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@website/docs/architecture/adding-support/backend.md` around lines 34 - 35,
The doc incorrectly labels Trino as the sync template; update the text to state
that Trino (TrinoAdapter) is the async/submit-poll model (its supports_async()
== true) and therefore should be presented as the async example, and replace the
sync example with a sync-capable adapter such as DuckDB HTTP (DuckDBHttpAdapter)
or StarRocks (StarRocksAdapter) so contributors are directed to the correct
execution model.

crates/queryflux-e2e-tests/tests/snowflake_tests.rs-193-205 (1)

193-205: ⚠️ Potential issue | 🟡 Minor

Tighten the metrics assertion to this query instance.

HARNESS is shared across the Snowflake suite, so clear_records() plus cluster_group == GROUP_SNOWFLAKE can still match a different Snowflake query when tests run in parallel. Use a unique marker in the SQL (or query id) and include it in the wait_for_record predicate.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@crates/queryflux-e2e-tests/tests/snowflake_tests.rs` around lines 193 - 205,
The test can race with other Snowflake queries because h.clear_records() +
cluster_group filter may still match another instance; change the test to embed
a unique marker in the SQL (or capture a returned query id) and then
wait_for_record using that marker in addition to cluster_group. Specifically,
update the call to client().execute_on(...) (or capture its query id) to include
a unique token (e.g., a UUID literal or test-specific string) in the SELECT,
then modify the wait_for_record predicate used on h.wait_for_record(|r| ...) to
require both r.cluster_group.0 == GROUP_SNOWFLAKE and that r contains the unique
token (e.g., r.query_text or r.query_id matches the token) so the assertion only
matches this query instance.

website/docs/architecture/frontends/flight-sql.md-62-66 (1)

62-66: ⚠️ Potential issue | 🟡 Minor

Use a SQL/text fence for the DuckDB snippet.

ATTACH and SELECT here are DuckDB SQL statements, not shell commands. A bash fence gives the wrong syntax highlighting and encourages copy/paste into the wrong environment.

Suggested doc fix

-```bash
+```sql
 # DuckDB (if Flight SQL extension is available)
 ATTACH 'grpc://localhost:50051' AS qf (TYPE flight_sql);
 SELECT * FROM qf.my_catalog.my_schema.my_table LIMIT 10;

</details>

<details>
<summary>🤖 Prompt for AI Agents</summary>

Verify each finding against the current code and only fix it if needed.

In @website/docs/architecture/frontends/flight-sql.md around lines 62 - 66, The
fenced code block currently uses a bash fence but contains DuckDB SQL statements
(ATTACH and SELECT); change the fence language from bash to sql so the snippet
uses SQL/text highlighting and discourages copying into a shell — update the
block that contains the ATTACH 'grpc://localhost:50051' AS qf (TYPE flight_sql);
and SELECT * FROM qf.my_catalog.my_schema.my_table LIMIT 10; lines to use an
sql fence instead of bash.

</details>

</blockquote></details>
<details>
<summary>website/docs/architecture/frontends/snowflake.md-168-170 (1)</summary><blockquote>

`168-170`: _⚠️ Potential issue_ | _🟡 Minor_

**Add a language to the JDBC URL fence.**

Markdownlint is already flagging this snippet. Tag it as `text` (or similar) so the docs lint stays clean.

<details>
<summary>Suggested doc fix</summary>

```diff
-```
+```text
 jdbc:snowflake://localhost:8445/?account=queryflux&ssl=off&db=my_catalog&schema=my_schema
 ```
```
</details>

<details>
<summary>🤖 Prompt for AI Agents</summary>

Verify each finding against the current code and only fix it if needed.

In @website/docs/architecture/frontends/snowflake.md around lines 168 - 170, The
fenced JDBC URL block should include a language tag to satisfy markdownlint;
change the code fence that currently surrounds the JDBC example (the block
starting with and the line 'jdbc:snowflake://localhost:8445/?account=queryflux&ssl=off&db=my_catalog&schema=my_schema') to use a language tag such as `text` (i.e., replace with ```text) so the
snippet is properly tagged.

</details>

</blockquote></details>
<details>
<summary>website/docs/architecture/adding-support/frontend.md-73-75 (1)</summary><blockquote>

`73-75`: _⚠️ Potential issue_ | _🟡 Minor_

**The `FrontendConfig` description is ahead of the current struct.**

In `crates/queryflux-core/src/config.rs`, `FrontendConfig` currently exposes only `enabled` and `port`. Mentioning `bind_address` and TLS here will send contributors looking for fields that do not exist yet.

<details>
<summary>Suggested doc fix</summary>

```diff
-2. **`FrontendConfig`** already carries **`port`**, **`bind_address`**, optional **TLS**, etc. Extend it only if every frontend needs a new knob; prefer protocol-specific structs only when necessary.
+2. **`FrontendConfig`** currently carries **`enabled`** and **`port`**. Extend it only if every frontend needs a new knob; prefer protocol-specific structs only when necessary.
```
</details>

<details>
<summary>🤖 Prompt for AI Agents</summary>

```
Verify each finding against the current code and only fix it if needed.

In `@website/docs/architecture/adding-support/frontend.md` around lines 73 - 75,
The docs mention fields (bind_address, TLS) that don't exist on FrontendConfig,
causing mismatch; either update the text under FrontendsConfig to only list the
actual current fields (enabled, port) and remove references to bind_address/TLS,
or if those options are intended, add them to the FrontendConfig struct (e.g.,
add bind_address: Option<String> and tls: Option<TlsConfig> with serde camelCase
keys) and update FrontendConfig's definition accordingly; ensure the doc text
and the FrontendConfig symbol remain consistent and use camelCase for YAML keys.
```

</details>

</blockquote></details>
<details>
<summary>website/docs/architecture/frontends/postgres-wire.md-69-79 (1)</summary><blockquote>

`69-79`: _⚠️ Potential issue_ | _🟡 Minor_

**Split the `psql` and Python snippets into separate fences.**

The block is declared as `bash`, but the second half is Python, so copy-pasting it as-is will not work.

<details>
<summary>Suggested doc fix</summary>

```diff
 ```bash
 # psql
 psql -h localhost -p 5432 -U dev -d my_catalog -c "SELECT 1"
+```
 
-# Python (psycopg2)
+```python
+# Python (psycopg2)
 import psycopg2
 conn = psycopg2.connect(host="localhost", port=5432, user="dev", dbname="my_catalog")
 cur = conn.cursor()
 cur.execute("SELECT 42 AS answer")
 print(cur.fetchone())
 ```
```
</details>

<details>
<summary>🤖 Prompt for AI Agents</summary>

Verify each finding against the current code and only fix it if needed.

In @website/docs/architecture/frontends/postgres-wire.md around lines 69 - 79,
The fenced code block mixes bash and Python which breaks copy-paste; close the
initial bash fence after the psql command and open a new ```python fence before
the psycopg2 snippet. Specifically, end the bash block right after the psql
command (the psql -h localhost -p 5432 -U dev -d my_catalog -c "SELECT 1"
line) and start a new Python fenced block for the `import psycopg2` /
`psycopg2.connect(...)` / `cur.execute("SELECT 42 AS answer")` example so each
language snippet has its own correct fence.

</details>

</blockquote></details>
<details>
<summary>crates/queryflux-frontend/src/snowflake/http/format.rs-274-291 (1)</summary><blockquote>

`274-291`: _⚠️ Potential issue_ | _🟡 Minor_

**Hardcoded timezone offset loses original timezone information.**

All timezone-aware timestamps use offset `1440` (UTC in minutes), ignoring the original timezone from the Arrow data. This may cause incorrect time interpretation for non-UTC timestamps.

Consider extracting the actual timezone offset from the Arrow field's timezone string when available.

<details>
<summary>🤖 Prompt for AI Agents</summary>

```
Verify each finding against the current code and only fix it if needed.

In `@crates/queryflux-frontend/src/snowflake/http/format.rs` around lines 274 -
291, The code currently hardcodes timezone_arr to 1440 minutes when has_tz is
true; instead, read the timezone string from the Arrow field type (inspect
arr.data_type() which matches DataType::Timestamp(_, Some(tz_str))) and parse it
into an offset in minutes (fallback to 0 or UTC if parsing fails), then populate
timezone_arr with that computed offset; update the creation of timezone_arr used
in StructArray::from so the StructArray (fields "epoch","fraction","timezone")
carries the actual offset rather than the fixed 1440 value and ensure types
(Int32Array) and length (len) remain the same.
```

</details>

</blockquote></details>
<details>
<summary>crates/queryflux-frontend/src/snowflake/http/format.rs-323-329 (1)</summary><blockquote>

`323-329`: _⚠️ Potential issue_ | _🟡 Minor_

**Silent error handling could mask data corruption issues.**

IPC write errors are discarded with `let _ = writer.write(batch)`. If a batch fails to serialize, the response will be missing data without any indication. Consider at least logging failures or propagating the error.


<details>
<summary>🛡️ Proposed fix to log write failures</summary>

```diff
     if let Ok(mut writer) = StreamWriter::try_new(&mut buf, &sf_schema) {
         for batch in &sf_batches {
-            let _ = writer.write(batch);
+            if let Err(e) = writer.write(batch) {
+                tracing::warn!("Failed to write Arrow batch to IPC stream: {e}");
+            }
         }
         let _ = writer.finish();
     }
```
</details>

<details>
<summary>🤖 Prompt for AI Agents</summary>

```
Verify each finding against the current code and only fix it if needed.

In `@crates/queryflux-frontend/src/snowflake/http/format.rs` around lines 323 -
329, The current loop silently ignores IPC write/finish errors
(StreamWriter::write and StreamWriter::finish) causing missing data to go
unnoticed; update the block that creates StreamWriter (try_new with &mut buf and
&sf_schema) to handle results from writer.write(batch) and writer.finish() by
either propagating the error (returning a Result) or logging the error with
context (including which batch/index failed and the error) before
aborting/returning, ensuring you surface serialization failures for sf_batches
instead of discarding them.
```

</details>

</blockquote></details>
<details>
<summary>crates/queryflux-frontend/src/snowflake/http/handlers/query.rs-25-25 (1)</summary><blockquote>

`25-25`: _⚠️ Potential issue_ | _🟡 Minor_

**Reject malformed query bodies instead of executing `""`.**

This path turns gzip/JSON/`sqlText` parsing failures into an empty SQL string, so a client bug becomes a backend execution error and still burns query capacity. Return a Snowflake bad-request payload when the body is invalid or `sqlText` is missing/blank.



Also applies to: 119-124

<details>
<summary>🤖 Prompt for AI Agents</summary>

```
Verify each finding against the current code and only fix it if needed.

In `@crates/queryflux-frontend/src/snowflake/http/handlers/query.rs` at line 25,
The handler currently lets decode_snowflake_request_body failures or
missing/blank sqlText fall through as an empty string and proceed to execution;
update the request handling in the query handler to validate the result of
decode_snowflake_request_body and ensure the parsed payload contains a non-empty
sqlText, and if not return a Snowflake bad-request response using sf_error
instead of passing "" to execution. Concretely, after calling
decode_snowflake_request_body (and after extract_snowflake_token as applicable)
check for Err or an empty/missing sqlText field and call sf_error with an
appropriate bad-request payload and short-circuit the handler; only call the
query execution path when sqlText is present and non-empty. Ensure the same
validation fix is applied to the other occurrence around the
decode_snowflake_request_body usage (the block referenced near the second
occurrence).
```

</details>

</blockquote></details>
<details>
<summary>website/versioned_docs/version-0.1.0/architecture/frontends/snowflake.md-168-170 (1)</summary><blockquote>

`168-170`: _⚠️ Potential issue_ | _🟡 Minor_

**Add a language fence to the JDBC example.**

This snippet is the markdownlint failure in the static-analysis output and it also drops syntax highlighting in rendered docs.

<details>
<summary>Suggested fix</summary>

```diff
-```
+```text
 jdbc:snowflake://localhost:8445/?account=queryflux&ssl=off&db=my_catalog&schema=my_schema
 ```
```
</details>

<details>
<summary>🤖 Prompt for AI Agents</summary>

Verify each finding against the current code and only fix it if needed.

In @website/versioned_docs/version-0.1.0/architecture/frontends/snowflake.md
around lines 168 - 170, The JDBC example code block in the Snowflake doc (the
line containing
"jdbc:snowflake://localhost:8445/?account=queryflux&ssl=off&db=my_catalog&schema=my_schema")
lacks a language fence; update the markdown in
versioned_docs/version-0.1.0/architecture/frontends/snowflake.md to surround
that snippet with a fenced code block and include a language marker (e.g.,
text before and after) so markdownlint passes and syntax highlighting is
preserved.

</details>

</blockquote></details>
<details>
<summary>crates/queryflux-frontend/src/snowflake/http/handlers/common.rs-104-117 (1)</summary><blockquote>

`104-117`: _⚠️ Potential issue_ | _🟡 Minor_

**Don’t forward hop-by-hop request headers upstream.**

`proxy_response()` already strips transport-scoped headers on the way back, but `passthrough_headers()` still forwards them to the warehouse. Client-supplied `connection`/`keep-alive`/`te`/`trailer`/`transfer-encoding`/`upgrade` headers should stop at the proxy boundary.

<details>
<summary>Suggested fix</summary>

```diff
-            if name == "authorization" || name == "host" || name == "content-length" {
+            if matches!(
+                name.as_str(),
+                "authorization"
+                    | "host"
+                    | "content-length"
+                    | "connection"
+                    | "keep-alive"
+                    | "te"
+                    | "trailer"
+                    | "transfer-encoding"
+                    | "upgrade"
+            ) {
                 return None;
             }
```
</details>

<details>
<summary>🤖 Prompt for AI Agents</summary>

```
Verify each finding against the current code and only fix it if needed.

In `@crates/queryflux-frontend/src/snowflake/http/handlers/common.rs` around lines
104 - 117, passthrough_headers currently forwards hop-by-hop headers; update the
function to filter out the standard hop-by-hop header names (e.g., "connection",
"keep-alive", "proxy-authenticate", "proxy-authorization", "te", "trailer",
"transfer-encoding", "upgrade", plus "authorization", "host", "content-length")
and also honor the "Connection" header's comma-separated token list by parsing
it and excluding any header names it advertises before collecting into the
HashMap; make the change inside passthrough_headers (and keep proxy_response
behavior unchanged) so clients cannot forward transport-scoped headers upstream.
```

</details>

</blockquote></details>
<details>
<summary>crates/queryflux-frontend/src/snowflake/sql_api/handlers.rs-144-149 (1)</summary><blockquote>

`144-149`: _⚠️ Potential issue_ | _🟡 Minor_

**Reject a missing `statement` instead of executing `""`.**

`unwrap_or("")` sends client input mistakes down the normal query path and turns them into backend syntax errors. The SQL API should return a request-level Snowflake error when `statement` is absent or blank.

<details>
<summary>Suggested fix</summary>

```diff
-    let sql = body_json["statement"].as_str().unwrap_or("").to_string();
+    let Some(sql) = body_json["statement"].as_str().filter(|s| !s.trim().is_empty()) else {
+        return sf_error(StatusCode::BAD_REQUEST, 390000, "Missing statement");
+    };
+    let sql = sql.to_string();
```
</details>

<details>
<summary>🤖 Prompt for AI Agents</summary>

```
Verify each finding against the current code and only fix it if needed.

In `@crates/queryflux-frontend/src/snowflake/sql_api/handlers.rs` around lines 144
- 149, The handler currently uses body_json["statement"].as_str().unwrap_or("")
which lets a missing or empty statement proceed as an empty SQL; change this to
explicitly validate body_json for a present, non-empty statement (trimmed) after
parse_snowflake_json_body and before assigning sql, and if absent or blank
return a request-level Snowflake error via sf_error (same
StatusCode::BAD_REQUEST and the existing request error code 390000) instead of
continuing to execute an empty string; reference parse_snowflake_json_body,
body_json, sql and sf_error when making the change.
```

</details>

</blockquote></details>

</blockquote></details>

<details>
<summary>🧹 Nitpick comments (11)</summary><blockquote>

<details>
<summary>docker/test/fakesnow-apply-patches.py (1)</summary><blockquote>

`106-108`: **Consider replacing all occurrences of `queryResultFormat`.**

Line 107 uses a single replacement (`replace(..., 1)`), but line 108 replaces all occurrences of `"queryResultFormat": "arrow"`. If the codebase has multiple response payload constructions, the `rowsetBase64` replacement on line 107 might miss additional locations while line 108 catches them all, leading to inconsistent state.

Verify whether fakesnow has multiple response construction sites, and if so, remove the `1` limit on line 107 or add validation that no `rowsetBase64` references remain.

<details>
<summary>Proposed fix for consistency</summary>

```diff
-    text = text.replace('"rowsetBase64": rowset_b64,', '"rowset": json_rowset,', 1)
+    text = text.replace('"rowsetBase64": rowset_b64,', '"rowset": json_rowset,')
```
</details>

<details>
<summary>🤖 Prompt for AI Agents</summary>

```
Verify each finding against the current code and only fix it if needed.

In `@docker/test/fakesnow-apply-patches.py` around lines 106 - 108, The
single-occurrence replacement on the variable text for '"rowsetBase64":
rowset_b64,' (text = text.replace('"rowsetBase64": rowset_b64,', '"rowset":
json_rowset,', 1)) can miss other payload sites while the subsequent replacement
for '"queryResultFormat": "arrow"' is global; remove the limit (drop the , 1) so
the rowsetBase64 -> rowset change is applied across all occurrences, or
alternatively keep the single-replace but add a validation step after both
replacements (e.g., assert or conditional check that '"rowsetBase64"' no longer
appears in text) to guarantee consistency; locate these edits via the text
variable and the exact replacement strings '"rowsetBase64": rowset_b64,' and
'"queryResultFormat": "arrow"' in the fakesnow-apply-patches.py file.
```

</details>

</blockquote></details>
<details>
<summary>website/versioned_docs/version-0.1.0/configuration.md (1)</summary><blockquote>

`45-46`: **Clarify `snowflakeSqlApi` configuration expectations.**

Line 45–46 may imply a separate frontend stanza is required for `snowflakeSqlApi`. Consider adding one short note that both Snowflake protocols are routed via the Snowflake frontend listener in this example.

<details>
<summary>🤖 Prompt for AI Agents</summary>

```
Verify each finding against the current code and only fix it if needed.

In `@website/versioned_docs/version-0.1.0/configuration.md` around lines 45 - 46,
Clarify that both configuration keys snowflakeHttp and snowflakeSqlApi are
handled by the same Snowflake frontend rather than requiring a separate frontend
stanza; update the docs near the snowflakeHttp/snowflakeSqlApi example to add a
short note stating that in this example both protocols are routed via the
Snowflake frontend listener (mention the Snowflake frontend listener by name) so
readers know no additional frontend block is necessary.
```

</details>

</blockquote></details>
<details>
<summary>crates/queryflux-engine-adapters/src/lib.rs (1)</summary><blockquote>

`42-48`: **Keep `build_from_config_json` to one cluster-name source.**

`cluster_name_str` duplicates `cluster_name` and lets callers pass conflicting values into factory implementations. Deriving the raw string from `cluster_name` keeps the public API smaller and avoids drift.

<details>
<summary>♻️ Suggested fix</summary>

```diff
     async fn build_from_config_json(
         &self,
         cluster_name: ClusterName,
         group: ClusterGroupName,
         json: &serde_json::Value,
-        cluster_name_str: &str,
     ) -> Result<Arc<dyn EngineAdapterTrait>>;
```
</details>

<details>
<summary>🤖 Prompt for AI Agents</summary>

```
Verify each finding against the current code and only fix it if needed.

In `@crates/queryflux-engine-adapters/src/lib.rs` around lines 42 - 48, The method
signature for build_from_config_json accepts both ClusterName and
cluster_name_str which allows conflicting inputs; remove the redundant
cluster_name_str parameter from the trait and all implementations and callers,
and derive the raw string when needed from the ClusterName value (e.g., via
ClusterName's Display/ToString or an as_str()/as_ref() accessor) inside
build_from_config_json implementations or callers; update the trait declaration
of build_from_config_json and every impl and call site referencing
build_from_config_json to use only (cluster_name: ClusterName, group:
ClusterGroupName, json: &serde_json::Value) and compute the string with
cluster_name.to_string() or cluster_name.as_str() where previously
cluster_name_str was used.
```

</details>

</blockquote></details>
<details>
<summary>queryflux-studio/components/cluster-config/snowflake-cluster-config.tsx (1)</summary><blockquote>

`220-254`: **Field reuse for key-pair auth is intentional but could benefit from a code comment.**

The mapping of `auth.password` → PEM key and `auth.token` → passphrase reuses the existing flat config structure. While the UI labels make this clear to users, a brief comment in the code would help future maintainers understand this is by design to match the backend's `ClusterAuth::KeyPair` field mapping.


<details>
<summary>📝 Suggested comment</summary>

```diff
       {/* Key Pair auth */}
       {authType === AUTH_KEY_PAIR && (
         <div className="space-y-4 rounded-xl border border-slate-100 bg-slate-50/60 p-4">
           <p className="text-[10px] font-semibold text-slate-500 uppercase tracking-widest">
             RSA Key Pair
           </p>
+          {/* Note: Reuses auth.password for PEM and auth.token for passphrase
+              to match backend ClusterAuth::KeyPair field mapping */}
           <div>
```
</details>

<details>
<summary>🤖 Prompt for AI Agents</summary>

```
Verify each finding against the current code and only fix it if needed.

In `@queryflux-studio/components/cluster-config/snowflake-cluster-config.tsx`
around lines 220 - 254, Add a brief inline code comment near the key-pair fields
explaining that the UI intentionally maps flat["auth.password"] to the PEM
private key and flat["auth.token"] to the passphrase to match the backend
ClusterAuth::KeyPair mapping; update the block around the textarea with id
"sf-kp-pem" and the input with id "sf-kp-passphrase" (which use onPatch to set
"auth.password" and "auth.token") to include that comment so future maintainers
understand the reused flat config fields are deliberate.
```

</details>

</blockquote></details>
<details>
<summary>.github/workflows/ci.yml (1)</summary><blockquote>

`51-58`: **Consider extracting the libduckdb installation into a composite action.**

The "Install libduckdb" step is duplicated across `clippy`, `unit-test`, and `e2e` jobs. While this works correctly, extracting it into a reusable composite action (e.g., `.github/actions/install-libduckdb/action.yml`) would reduce maintenance burden when the DuckDB version or installation process changes.

<details>
<summary>🤖 Prompt for AI Agents</summary>

```
Verify each finding against the current code and only fix it if needed.

In @.github/workflows/ci.yml around lines 51 - 58, The "Install libduckdb" step
(the job step named "Install libduckdb" that uses DUCKDB_VERSION and runs
curl/unzip/cp/ldconfig) is duplicated across clippy, unit-test, and e2e
jobs—extract that logic into a reusable composite action (e.g.,
.github/actions/install-libduckdb/action.yml) which accepts DUCKDB_VERSION as an
input and performs the curl/unzip/cp/ldconfig sequence; then replace the in-job
step in each workflow job with a single uses:
./.github/actions/install-libduckdb call passing DUCKDB_VERSION, and update any
job permissions/needs as required so all three jobs reference the new composite
action.
```

</details>

</blockquote></details>
<details>
<summary>website/versioned_docs/version-0.1.0/architecture/frontends/overview.md (2)</summary><blockquote>

`25-44`: **Add a language identifier to the fenced code block.**

The ASCII diagram should specify a language (e.g., `text` or `plaintext`) to satisfy markdown linters and improve accessibility for screen readers.


<details>
<summary>📝 Proposed fix</summary>

````diff
-```
+```text
 Client  ──(native protocol)──►  Frontend Listener

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@website/versioned_docs/version-0.1.0/architecture/frontends/overview.md`
around lines 25 - 44, The fenced ASCII diagram block is missing a language
identifier; update the diagram's code fence that starts with ``` and contains
the ASCII flow (Client ──(native protocol)──► Frontend Listener ... ResultSink
──► native protocol response) to use a plain text identifier (for example change
the opening fence to ```text) and ensure the closing ``` remains; this satisfies
markdown linters and improves screen-reader handling of the diagram.

---

59-67: Snowflake SessionContext variant is missing from the table.

The table lists variants for other frontends but line 67 only mentions Snowflake in prose. Consider adding a SnowflakeHttp row to the table for consistency, or clarify why Snowflake reuses MySqlWire internally.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@website/versioned_docs/version-0.1.0/architecture/frontends/overview.md`
around lines 59 - 67, The table of frontend SessionContext variants omits
Snowflake: update the markdown table to either add a `SnowflakeHttp` row (e.g.,
list the relevant fields like `user`, `schema`, `session_vars` or
`database_hint`) or explicitly note in the `MySqlWire` row that Snowflake
frontends build a `MySqlWire` session context internally; edit the table entries
near the `TrinoHttp`, `PostgresWire`, `MySqlWire`, `ClickHouseHttp`, `FlightSql`
rows and reference `SnowflakeHttp` or `SessionContext`/`MySqlWire` accordingly
so the prose and table are consistent.

crates/queryflux-frontend/src/snowflake/tests.rs (1)

254-256: Fixed sleep could cause flaky tests.

The 20ms sleep assumes the server is ready, but under load or on slow CI, this may not be enough. Consider a retry loop or checking a readiness endpoint.

♻️ Suggested approach for robust server readiness

-        tokio::time::sleep(Duration::from_millis(20)).await;
+        // Wait for server to be ready (up to 1 second)
+        let client = reqwest::Client::new();
+        for _ in 0..50 {
+            if client.get(format!("http://127.0.0.1:{port}/session/heartbeat"))
+                .send().await.is_ok() {
+                break;
+            }
+            tokio::time::sleep(Duration::from_millis(20)).await;
+        }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@crates/queryflux-frontend/src/snowflake/tests.rs` around lines 254 - 256,
Replace the fixed 20ms sleep in the async test helper that currently calls
tokio::time::sleep(Duration::from_millis(20)).await with a retry/readiness loop:
repeatedly attempt a lightweight readiness check (e.g., a TCP connect to the
returned port or an HTTP health endpoint) with short backoff
(tokio::time::sleep) until success or a global timeout; if the timeout elapses,
return an error or panic so the test fails deterministically. Update the helper
(the function containing the sleep and returning (port, tx)) to perform this
loop and only return after a successful readiness probe.

crates/queryflux-frontend/src/snowflake/http/session_store.rs (1)

11-52: Consider adding session expiration/cleanup to prevent unbounded memory growth.

The SnowflakeSessionStore holds sessions indefinitely with no TTL enforcement. The created_at field exists but isn't used for cleanup. If clients don't explicitly logout, sessions accumulate.

Consider adding a background task or lazy cleanup to evict stale sessions:

♻️ Suggested approach for session cleanup

+impl SnowflakeSessionStore {
+    /// Remove sessions older than `max_age`.
+    pub fn cleanup_expired(&self, max_age: std::time::Duration) {
+        let now = std::time::Instant::now();
+        self.sessions.retain(|_, session| {
+            now.duration_since(session.created_at) < max_age
+        });
+    }
+}

Then periodically call cleanup_expired from a background task (e.g., every 5 minutes with a 1-hour TTL).

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@crates/queryflux-frontend/src/snowflake/http/session_store.rs` around lines
11 - 52, The session store currently keeps SnowflakeSession entries forever
despite having created_at; add TTL-based eviction by implementing a method
cleanup_expired(&self, ttl: Duration) that iterates the DashMap and removes
entries whose session.created_at.elapsed() > ttl, and ensure you call it
periodically (either spawn a background task in SnowflakeSessionStore::new that
loops sleeping for a configurable cleanup_interval and calls cleanup_expired
with a configurable ttl, or expose start_cleanup/background_start methods so the
caller can spawn the task); keep existing insert/get/remove/contains behavior
and ensure the background task uses Arc<Self> so removals operate on the same
store.

website/docs/architecture/frontends/overview.md (1)

25-44: Add a language identifier to the fenced code block.

Same as the versioned doc — use text or plaintext for the ASCII diagram.

📝 Proposed fix

-```
+```text
 Client  ──(native protocol)──►  Frontend Listener

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@website/docs/architecture/frontends/overview.md` around lines 25 - 44, The
fenced ASCII diagram block lacks a language identifier; update the opening fence
for the diagram (the block that starts with ```) to include a plaintext
identifier such as "text" or "plaintext" so the diagram like "Client  ──(native
protocol)──►  Frontend Listener" and the subsequent lines are rendered as plain
text; modify the fenced code block surrounding the ASCII diagram in overview.md
to begin with ```text (or ```plaintext).

crates/queryflux-frontend/src/snowflake/proxy.rs (1)

29-36: Consider adding request timeout to prevent indefinite hangs.

The reqwest::Client is built without timeout configuration. If the upstream Snowflake warehouse is unresponsive, requests will hang indefinitely, potentially exhausting connection resources.

♻️ Proposed fix to add timeout

     pub fn new() -> Self {
         Self {
             client: Client::builder()
+                .timeout(std::time::Duration::from_secs(300)) // 5 min for long queries
+                .connect_timeout(std::time::Duration::from_secs(30))
                 .build()
                 .expect("failed to build SnowflakeProxy reqwest client"),
         }
     }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@crates/queryflux-frontend/src/snowflake/proxy.rs` around lines 29 - 36, The
SnowflakeProxy::new constructor builds a reqwest Client without a timeout,
risking indefinite hangs; update SnowflakeProxy::new to configure a reasonable
request timeout (e.g., using
Client::builder().timeout(Duration::from_secs(...))) when constructing the
client and add the necessary std::time::Duration import so all outgoing requests
from the client will fail fast instead of hanging indefinitely.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 255872db-151b-498e-9689-0cca35e6b3d4

📥 Commits

Reviewing files that changed from the base of the PR and between 2804d6a and 32ada8a.

⛔ Files ignored due to path filters (1)

• Cargo.lock is excluded by !**/*.lock

📒 Files selected for processing (105)

• .cargo/config.toml
• .github/workflows/ci.yml
• .github/workflows/docker-verify.yml
• Makefile
• README.md
• crates/queryflux-core/src/config.rs
• crates/queryflux-core/src/engine_registry.rs
• crates/queryflux-core/src/query.rs
• crates/queryflux-e2e-tests/src/harness.rs
• crates/queryflux-e2e-tests/src/trino_client.rs
• crates/queryflux-e2e-tests/tests/routing_tests.rs
• crates/queryflux-e2e-tests/tests/snowflake_tests.rs
• crates/queryflux-engine-adapters/Cargo.toml
• crates/queryflux-engine-adapters/src/athena/mod.rs
• crates/queryflux-engine-adapters/src/duckdb/http.rs
• crates/queryflux-engine-adapters/src/duckdb/mod.rs
• crates/queryflux-engine-adapters/src/lib.rs
• crates/queryflux-engine-adapters/src/snowflake/mod.rs
• crates/queryflux-engine-adapters/src/starrocks/mod.rs
• crates/queryflux-engine-adapters/src/trino/mod.rs
• crates/queryflux-frontend/Cargo.toml
• crates/queryflux-frontend/src/admin.rs
• crates/queryflux-frontend/src/dispatch.rs
• crates/queryflux-frontend/src/lib.rs
• crates/queryflux-frontend/src/snowflake/http/format.rs
• crates/queryflux-frontend/src/snowflake/http/handlers/common.rs
• crates/queryflux-frontend/src/snowflake/http/handlers/mod.rs
• crates/queryflux-frontend/src/snowflake/http/handlers/query.rs
• crates/queryflux-frontend/src/snowflake/http/handlers/session.rs
• crates/queryflux-frontend/src/snowflake/http/handlers/token.rs
• crates/queryflux-frontend/src/snowflake/http/mod.rs
• crates/queryflux-frontend/src/snowflake/http/session_store.rs
• crates/queryflux-frontend/src/snowflake/mod.rs
• crates/queryflux-frontend/src/snowflake/proxy.rs
• crates/queryflux-frontend/src/snowflake/sql_api/auth.rs
• crates/queryflux-frontend/src/snowflake/sql_api/handlers.rs
• crates/queryflux-frontend/src/snowflake/sql_api/mod.rs
• crates/queryflux-frontend/src/snowflake/tests.rs
• crates/queryflux-frontend/src/state.rs
• crates/queryflux-persistence/src/cluster_config.rs
• crates/queryflux-routing/src/implementations/protocol_based.rs
• crates/queryflux-routing/src/implementations/python_script.rs
• crates/queryflux-routing/tests/router_tests.rs
• crates/queryflux/src/main.rs
• crates/queryflux/src/registered_engines.rs
• development.md
• docker/queryflux/Dockerfile
• docker/test/docker-compose.test.yml
• docker/test/fakesnow-apply-patches.py
• docker/test/fakesnow-entrypoint.sh
• docs/README.md
• docs/adding-engine-support.md
• docs/architecture.md
• docs/auth-authz-design.md
• docs/motivation-and-goals.md
• docs/observability.md
• docs/query-translation.md
• docs/routing-and-clusters.md
• queryflux-studio/README.md
• queryflux-studio/app/protocols/page.tsx
• queryflux-studio/components/cluster-config/config-field-row.tsx
• queryflux-studio/components/cluster-config/index.ts
• queryflux-studio/components/cluster-config/snowflake-cluster-config.tsx
• queryflux-studio/components/cluster-config/studio-engine-forms.tsx
• queryflux-studio/components/engine-catalog.ts
• queryflux-studio/lib/cluster-persist-form.ts
• queryflux-studio/lib/engine-registry-types.ts
• queryflux-studio/lib/studio-engines/engines/snowflake.ts
• queryflux-studio/lib/studio-engines/manifest.ts
• website/docs/architecture/adding-engine-support.md
• website/docs/architecture/adding-support/backend.md
• website/docs/architecture/adding-support/frontend.md
• website/docs/architecture/adding-support/overview.md
• website/docs/architecture/frontends/flight-sql.md
• website/docs/architecture/frontends/mysql-wire.md
• website/docs/architecture/frontends/overview.md
• website/docs/architecture/frontends/postgres-wire.md
• website/docs/architecture/frontends/snowflake.md
• website/docs/architecture/frontends/trino-http.md
• website/docs/architecture/overview.md
• website/docs/architecture/routing-and-clusters.md
• website/docs/architecture/system-map.md
• website/docs/configuration.md
• website/docs/development.md
• website/docs/project-structure.md
• website/docs/roadmap.md
• website/sidebars.ts
• website/versioned_docs/version-0.1.0/architecture/adding-engine-support.md
• website/versioned_docs/version-0.1.0/architecture/adding-support/backend.md
• website/versioned_docs/version-0.1.0/architecture/adding-support/frontend.md
• website/versioned_docs/version-0.1.0/architecture/adding-support/overview.md
• website/versioned_docs/version-0.1.0/architecture/frontends/flight-sql.md
• website/versioned_docs/version-0.1.0/architecture/frontends/mysql-wire.md
• website/versioned_docs/version-0.1.0/architecture/frontends/overview.md
• website/versioned_docs/version-0.1.0/architecture/frontends/postgres-wire.md
• website/versioned_docs/version-0.1.0/architecture/frontends/snowflake.md
• website/versioned_docs/version-0.1.0/architecture/frontends/trino-http.md
• website/versioned_docs/version-0.1.0/architecture/overview.md
• website/versioned_docs/version-0.1.0/architecture/routing-and-clusters.md
• website/versioned_docs/version-0.1.0/architecture/system-map.md
• website/versioned_docs/version-0.1.0/configuration.md
• website/versioned_docs/version-0.1.0/development.md
• website/versioned_docs/version-0.1.0/project-structure.md
• website/versioned_docs/version-0.1.0/roadmap.md
• website/versioned_sidebars/version-0.1.0-sidebars.json

💤 Files with no reviewable changes (8)

• docs/architecture.md
• docs/observability.md
• docs/README.md
• docs/adding-engine-support.md
• docs/motivation-and-goals.md
• docs/query-translation.md
• docs/routing-and-clusters.md
• docs/auth-authz-design.md

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Avoid node-local Snowflake sessions for HTTP traffic.

If Snowflake login and follow-up requests can hit different QueryFlux instances, Line 71 makes those sessions process-local, so they will break under normal load balancing or during rolling restarts. Please either back this store with shared persistence or explicitly require/enforce sticky routing on the Snowflake endpoints.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@crates/queryflux-frontend/src/state.rs` around lines 70 - 71, The
snowflake_sessions field (pub snowflake_sessions: Arc<SnowflakeSessionStore>) is
currently process-local and will break if Snowflake login and subsequent
requests land on different instances; fix by replacing or wrapping
SnowflakeSessionStore with a shared backend (e.g., backed by Redis, DB, or an
inter-process store) and update any constructors/initializers that create
SnowflakeSessionStore to accept and use that shared persistence, or
alternatively enforce sticky routing at the HTTP entry points that handle
Snowflake login/follow-up requests (update the handler registration or
middleware to require session affinity); ensure code touching
SnowflakeSessionStore (constructors, Arc creation, and the handlers that
read/write sessions) consistently uses the new shared store interface so
sessions survive across instances.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

POSTGRES_DIR="$(fd -i 'postgres_wire' crates/queryflux-frontend/src -t d | head -n1)"
test -n "$POSTGRES_DIR"

# Expect an auth challenge plus password-message handling before AuthenticationOk
# if password auth is actually supported.
rg -n -C3 'StartupMessage|PasswordMessage|Authentication(Ok|CleartextPassword|MD5Password|SASL)' "$POSTGRES_DIR"

Repository: lakeops-org/queryflux

Length of output: 1448

---

Clarify that password authentication is not supported.

The startup flow does not implement password authentication. The implementation reads the user field from StartupMessage but proceeds directly to AuthenticationOk without any password exchange. Document explicitly that only username-based authentication is supported, as this is a security-relevant limitation.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@website/docs/architecture/frontends/postgres-wire.md` around lines 35 - 37,
The docs currently describe the startup flow but omit that password-based
authentication is not implemented; update the text around StartupMessage and
QueryFlux authentication to explicitly state that QueryFlux reads credentials
solely from the startup `user` field and authenticates via the configured
`auth_provider` without any password exchange, i.e., password authentication is
unsupported; mention this security-relevant limitation alongside the sequence of
messages (AuthenticationOk, ParameterStatus, BackendKeyData, ReadyForQuery) so
readers know only username-based auth is performed.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

OWNER=lakeops-org
REPO=queryflux
TAG=""

for candidate in v0.1.0 0.1.0; do
  if gh api "repos/$OWNER/$REPO/git/ref/tags/$candidate" >/dev/null 2>&1; then
    TAG="$candidate"
    break
  fi
done
test -n "$TAG"

# These symbols need to exist in the 0.1.0 sources if this page is going to
# live under version-0.1.0.
gh api "repos/$OWNER/$REPO/contents/crates/queryflux-engine-adapters/src/lib.rs?ref=$TAG" \
  | jq -r '.content' | base64 -d \
  | rg -n 'EngineAdapterFactory|EngineAdapterTrait'

gh api "repos/$OWNER/$REPO/contents/crates/queryflux/src/registered_engines.rs?ref=$TAG" \
  | jq -r '.content' | base64 -d \
  | rg -n 'all_factories|build_adapter_from_record|try_from_config_json'

Repository: lakeops-org/queryflux

Length of output: 47

---

🏁 Script executed:

#!/bin/bash
set -euo pipefail

OWNER=lakeops-org
REPO=queryflux

# List all tags to see what versions exist
echo "=== Available tags ==="
gh api "repos/$OWNER/$REPO/tags" --paginate | jq -r '.[].name' | head -20

# Try to directly fetch tag info
echo ""
echo "=== Checking for 0.1.0 or v0.1.0 ==="
gh api "repos/$OWNER/$REPO/git/matching-refs/tags/0" | jq -r '.[] | .ref' || echo "No match found"

Repository: lakeops-org/queryflux

Length of output: 200

---

🏁 Script executed:

#!/bin/bash
set -euo pipefail

# List all tags in the repository
echo "=== All tags ==="
git tag -l | grep -E '^v?0\.' || echo "No 0.x tags found"

# Check if any tag contains '0.1.0'
echo ""
echo "=== Searching for 0.1.0 reference ==="
git tag -l | grep 0.1.0 || echo "No exact 0.1.0 tag"

# Show current branch
echo ""
echo "=== Current branch and recent commits ==="
git branch
git log --oneline -5

# Check if versioned_docs directory exists
echo ""
echo "=== Versioned docs structure ==="
ls -la website/versioned_docs/ 2>/dev/null || echo "versioned_docs not found"

Repository: lakeops-org/queryflux

Length of output: 532

---

🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Verify the file mentioned in the review exists and check its content
echo "=== File being reviewed ==="
if [ -f "website/versioned_docs/version-0.1.0/architecture/adding-support/backend.md" ]; then
  wc -l "website/versioned_docs/version-0.1.0/architecture/adding-support/backend.md"
  echo ""
  echo "File exists. Checking for references to factory APIs..."
  grep -n "EngineAdapterFactory\|build_from_config_json\|EngineAdapterTrait" \
    "website/versioned_docs/version-0.1.0/architecture/adding-support/backend.md" | head -20
fi

# Now check if these APIs exist in the current main source code
echo ""
echo "=== Checking for EngineAdapterFactory in current sources ==="
rg -l "EngineAdapterFactory|EngineAdapterTrait" crates/ 2>/dev/null | head -10

# Check the actual trait definition
echo ""
echo "=== Looking for EngineAdapterTrait definition ==="
rg -A 5 "trait EngineAdapterTrait" crates/ 2>/dev/null | head -20

Repository: lakeops-org/queryflux

Length of output: 2653

---

Remove this guide from the version-0.1.0 docs snapshot, or wait until the 0.1.0 release tag is created with these APIs.

This page documents EngineAdapterFactory, EngineAdapterTrait, and the factory-based registration flow, but no 0.1.0 or v0.1.0 release tag exists in the repository. The versioned_docs/version-0.1.0 snapshot should match a released version; publishing documentation for unreleased APIs creates drift and makes the versioned docs historically inaccurate. Additionally, line 140 links to main branch, which will diverge from the 0.1.0 APIs if the release never materializes with this architecture.

Either move this content to next-version docs (if this is preview documentation) or wait until the 0.1.0 release is tagged with these APIs before including it in the version-0.1.0 snapshot.

🧰 Tools

🪛 LanguageTool

[style] ~55-~55: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...y), implemented: true once wired. 4. Implement **try_from_config_json(..., json: &ser...

(ENGLISH_WORD_REPEAT_BEGINNING_RULE)

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@website/versioned_docs/version-0.1.0/architecture/adding-support/backend.md`
around lines 19 - 74, This page documents unreleased APIs (EngineAdapterFactory,
EngineAdapterTrait, try_from_config_json, try_from_cluster_config,
registered_engines.rs) and must be removed from the versioned snapshot; either
delete this document from the version-0.1.0 docs snapshot or move it into the
next-version (unreleased) docs instead, and if you keep a versioned copy update
any links that point to main (or to the new APIs) to point to the actual v0.1.0
tag or remove them until the release exists so the snapshot matches the released
API surface.

[coderabbitai]

Actionable comments posted: 4

♻️ Duplicate comments (2)

crates/queryflux-frontend/src/dispatch.rs (1)

255-268: ⚠️ Potential issue | 🟠 Major

Guard this terminal-submit fast path to Trino backends only.

This unconditionally calls Trino-specific finalization for any async adapter when next_uri.is_none(). With Snowflake support being added in this PR, a Snowflake adapter returning a terminal submit body would be incorrectly parsed as TrinoResponse.

Suggested fix

-    if next_uri.is_none() {
+    if next_uri.is_none() && matches!(adapter.engine_type(), queryflux_core::query::EngineType::Trino) {
         if let Some(ref ib) = initial_body {
             finalize_trino_async_terminal_on_submit(

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@crates/queryflux-frontend/src/dispatch.rs` around lines 255 - 268, The code
currently calls finalize_trino_async_terminal_on_submit whenever
next_uri.is_none() and initial_body is Some, which invokes Trino-specific
parsing for any async adapter; change this to only run the Trino finalization
when the adapter/transport actually represents a Trino backend (e.g., check the
adapter type or protocol enum) — for example, wrap the existing block in an
explicit match/if that verifies adapter is Trino (or protocol ==
Protocol::Trino) before calling finalize_trino_async_terminal_on_submit(state,
&cluster_manager, &executing, &adapter, &session, protocol, ib). This prevents
Snowflake or other adapters from being parsed as Trino.

crates/queryflux-frontend/src/snowflake/http/handlers/query.rs (1)

140-143: ⚠️ Potential issue | 🟠 Major

Propagate the session schema into SessionContext.

Line 142 still uses database as SessionContext::MySqlWire.schema, so the stored session schema is ignored at execution time.

Suggested fix

     let session_ctx = SessionContext::MySqlWire {
         user,
-        schema: Some(database.clone()),
+        schema: Some(if schema.is_empty() { database.clone() } else { schema.clone() }),
         session_vars: HashMap::new(),
         tags: QueryTags::default(),
     };

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@crates/queryflux-frontend/src/snowflake/http/handlers/query.rs` around lines
140 - 143, The SessionContext::MySqlWire construction is setting schema to
database.clone(), which ignores the stored session's schema; change the schema
assignment to use the stored session's schema (e.g., the session struct's schema
field or accessor) so SessionContext::MySqlWire.schema is populated from the
session (use the existing session variable/name in scope), ensuring session_ctx
reflects the actual session schema at execution time instead of the local
database variable.

🧹 Nitpick comments (1)

crates/queryflux-frontend/src/dispatch.rs (1)

386-402: Inconsistent metrics call ordering in success path.

In the error/FAILED branches (lines 354-385), on_query_finished is called before record_query. In the success branch, record_query is called first. This inconsistency could cause subtle timing issues in metrics aggregation.

Align ordering with error paths

     } else {
+        state
+            .metrics
+            .on_query_finished(&executing.cluster_group.0, &executing.cluster_name.0);
         state.record_query(
             &ctx,
             QueryOutcome {
                 backend_query_id: backend_id,
                 status: QueryStatus::Success,
                 execution_ms: elapsed_ms,
                 rows: None,
                 error: None,
                 routing_trace: None,
                 engine_stats,
             },
         );
-        state
-            .metrics
-            .on_query_finished(&executing.cluster_group.0, &executing.cluster_name.0);
     }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@crates/queryflux-frontend/src/dispatch.rs` around lines 386 - 402, The
success path currently calls state.record_query(...) before
state.metrics.on_query_finished(...), which is inconsistent with the error paths
where on_query_finished is invoked first; move the call to
state.metrics.on_query_finished(&executing.cluster_group.0,
&executing.cluster_name.0) to occur before state.record_query(...) in the
success branch so the ordering matches the error/FAILED branches (referencing
state.record_query, state.metrics.on_query_finished, QueryOutcome with
QueryStatus::Success, and executing.cluster_group/cluster_name).

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@crates/queryflux-frontend/src/dispatch.rs`:
- Around line 320-339: QueryContext is being populated with swapped SQL fields:
it currently sets sql to executing.sql and translated_sql to
executing.translated_sql, but in ExecutingQuery the stored semantics are
reversed (executing.sql is the translated SQL and executing.translated_sql is
the original). Fix by assigning QueryContext.sql = executing.translated_sql
(original SQL) and QueryContext.translated_sql = Some(executing.sql) when
was_translated is true (or None otherwise); update any places referencing
QueryContext fields accordingly to preserve the documented meanings.

In `@crates/queryflux-frontend/src/snowflake/http/handlers/query.rs`:
- Around line 120-124: The code currently uses unwrap_or_default to set sql to
an empty string when decode_snowflake_request_body/JSON parsing/sqlText
extraction fails; instead, detect failures from decode_snowflake_request_body,
serde_json::from_slice::<Value>, and v["sqlText"].as_str() and immediately
return a protocol/BadRequest-style error response (do not default to ""), so
replace the chain that builds sql with explicit error handling that returns a
protocol error when any step fails, referencing decode_snowflake_request_body,
serde_json::from_slice::<Value>, and the v["sqlText"].as_str() extraction.

In `@website/versioned_docs/version-0.1.0/architecture/frontends/overview.md`:
- Around line 25-44: The fenced code block containing the architecture diagram
(the block that starts with the triple backticks and the ASCII diagram showing
Client, Frontend Listener, Router Chain, ClusterGroupManager, Translation
Service, Engine Adapter, ResultSink) is unlabeled and triggers markdownlint
MD040; add a language identifier (e.g., use ```text) to the opening fence so the
block becomes a labeled fenced code block and resolves the lint error.

In `@website/versioned_docs/version-0.1.0/architecture/frontends/snowflake.md`:
- Around line 168-170: The fenced code block containing the JDBC URL
"jdbc:snowflake://localhost:8445/?account=queryflux&ssl=off&db=my_catalog&schema=my_schema"
is unlabeled and triggers markdownlint MD040; fix it by adding a language label
to the opening fence (for example ```text or ```sql) so the block becomes a
labeled fenced code block and the lint rule is satisfied.

---

Duplicate comments:
In `@crates/queryflux-frontend/src/dispatch.rs`:
- Around line 255-268: The code currently calls
finalize_trino_async_terminal_on_submit whenever next_uri.is_none() and
initial_body is Some, which invokes Trino-specific parsing for any async
adapter; change this to only run the Trino finalization when the
adapter/transport actually represents a Trino backend (e.g., check the adapter
type or protocol enum) — for example, wrap the existing block in an explicit
match/if that verifies adapter is Trino (or protocol == Protocol::Trino) before
calling finalize_trino_async_terminal_on_submit(state, &cluster_manager,
&executing, &adapter, &session, protocol, ib). This prevents Snowflake or other
adapters from being parsed as Trino.

In `@crates/queryflux-frontend/src/snowflake/http/handlers/query.rs`:
- Around line 140-143: The SessionContext::MySqlWire construction is setting
schema to database.clone(), which ignores the stored session's schema; change
the schema assignment to use the stored session's schema (e.g., the session
struct's schema field or accessor) so SessionContext::MySqlWire.schema is
populated from the session (use the existing session variable/name in scope),
ensuring session_ctx reflects the actual session schema at execution time
instead of the local database variable.

---

Nitpick comments:
In `@crates/queryflux-frontend/src/dispatch.rs`:
- Around line 386-402: The success path currently calls state.record_query(...)
before state.metrics.on_query_finished(...), which is inconsistent with the
error paths where on_query_finished is invoked first; move the call to
state.metrics.on_query_finished(&executing.cluster_group.0,
&executing.cluster_name.0) to occur before state.record_query(...) in the
success branch so the ordering matches the error/FAILED branches (referencing
state.record_query, state.metrics.on_query_finished, QueryOutcome with
QueryStatus::Success, and executing.cluster_group/cluster_name).

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 8fd58a97-9909-4a15-a07c-b99cf9da01c8

📥 Commits

Reviewing files that changed from the base of the PR and between 32ada8a and b5b13f7.

📒 Files selected for processing (7)

• crates/queryflux-frontend/src/dispatch.rs
• crates/queryflux-frontend/src/snowflake/http/handlers/query.rs
• crates/queryflux-frontend/src/snowflake/sql_api/handlers.rs
• website/docs/architecture/frontends/overview.md
• website/docs/architecture/frontends/snowflake.md
• website/versioned_docs/version-0.1.0/architecture/frontends/overview.md
• website/versioned_docs/version-0.1.0/architecture/frontends/snowflake.md

✅ Files skipped from review due to trivial changes (1)

• website/docs/architecture/frontends/snowflake.md

🚧 Files skipped from review as they are similar to previous changes (2)

• website/docs/architecture/frontends/overview.md
• crates/queryflux-frontend/src/snowflake/sql_api/handlers.rs

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Add a language identifier to the architecture diagram code fence.

Line 25 uses an unlabeled fenced block, which triggers markdownlint MD040.

Suggested fix

-```
+```text
 Client  ──(native protocol)──►  Frontend Listener
                                       │
                                 SessionContext + SQL
@@
                                  ResultSink  ──► native protocol response

</details>

<!-- suggestion_start -->

<details>
<summary>📝 Committable suggestion</summary>

> ‼️ **IMPORTANT**
> Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

```suggestion

🧰 Tools

🪛 markdownlint-cli2 (0.22.0)

[warning] 25-25: Fenced code blocks should have a language specified

(MD040, fenced-code-language)

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@website/versioned_docs/version-0.1.0/architecture/frontends/overview.md`
around lines 25 - 44, The fenced code block containing the architecture diagram
(the block that starts with the triple backticks and the ASCII diagram showing
Client, Frontend Listener, Router Chain, ClusterGroupManager, Translation
Service, Engine Adapter, ResultSink) is unlabeled and triggers markdownlint
MD040; add a language identifier (e.g., use ```text) to the opening fence so the
block becomes a labeled fenced code block and resolves the lint error.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Label the JDBC code block language.

Line 168 has an unlabeled fenced code block and fails markdownlint MD040.

Suggested fix

-```
+```text
 jdbc:snowflake://localhost:8445/?account=queryflux&ssl=off&db=my_catalog&schema=my_schema

</details>

<!-- suggestion_start -->

<details>
<summary>📝 Committable suggestion</summary>

> ‼️ **IMPORTANT**
> Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

```suggestion

🧰 Tools

🪛 markdownlint-cli2 (0.22.0)

[warning] 168-168: Fenced code blocks should have a language specified

(MD040, fenced-code-language)

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@website/versioned_docs/version-0.1.0/architecture/frontends/snowflake.md`
around lines 168 - 170, The fenced code block containing the JDBC URL
"jdbc:snowflake://localhost:8445/?account=queryflux&ssl=off&db=my_catalog&schema=my_schema"
is unlabeled and triggers markdownlint MD040; fix it by adding a language label
to the opening fence (for example ```text or ```sql) so the block becomes a
labeled fenced code block and the lint rule is satisfied.

[coderabbitai]

Actionable comments posted: 2

♻️ Duplicate comments (4)

crates/queryflux-frontend/src/dispatch.rs (1)

255-272: ⚠️ Potential issue | 🟠 Major

Handle next_uri == None outside the Trino fast path.

After Line 252 has already persisted executing, this branch only cleans up the Trino + initial_body case. Every other next_uri.is_none() combination still falls through to DispatchOutcome::Async { proxy_next_uri: None }, so the caller has no continuation URI and the cluster lease / execution row stay orphaned. With Snowflake async support landing in this PR, supports_async() is no longer equivalent to Trino semantics; please either make this combination impossible at the adapter contract boundary or add an adapter-specific/generic terminal-submit finalizer here.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@crates/queryflux-frontend/src/dispatch.rs` around lines 255 - 272, The
current branch that checks next_uri.is_none() only finalizes Trino via
finalize_trino_async_terminal_on_submit, leaving non-Trino async executions
orphaned; add a generic or adapter-specific terminal finalizer call here so any
engine that can produce a terminal POST with no nextUri can clean up (or enforce
at the adapter contract that this case cannot occur). Concretely: in the same if
next_uri.is_none() { if let Some(ref ib) = initial_body { ... } } block, after
the Trino-specific call, invoke a new adapter method (e.g.,
adapter.finalize_async_terminal_on_submit(...) or a generic
finalize_async_terminal_on_submit(state, &cluster_manager, &executing, &adapter,
&session, protocol, ib)) for adapters where adapter.supports_async() is true (or
make supports_async() guarantee Trino semantics), ensuring cluster
lease/execution row cleanup for non-Trino engines as well.

crates/queryflux-frontend/src/snowflake/http/handlers/query.rs (1)

162-168: ⚠️ Potential issue | 🟠 Major

Use the stored Snowflake schema for execution.

This still passes database into SessionContext::MySqlWire.schema, so execution targets the wrong schema while the response echoes the requested one. Feed the saved schema into the session context here.

Minimal fix

     let session_ctx = SessionContext::MySqlWire {
         user,
-        schema: Some(database.clone()),
+        schema: Some(schema.clone()),
         session_vars: HashMap::new(),
         tags: QueryTags::default(),
     };

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@crates/queryflux-frontend/src/snowflake/http/handlers/query.rs` around lines
162 - 168, The session context is being initialized with the saved database but
not the saved schema, causing execution to run against the wrong schema; in the
SessionContext::MySqlWire construction (where user, schema:
Some(database.clone()), session_vars: HashMap::new() are set) replace the schema
value to use the saved schema variable (e.g., schema.clone() or
Some(schema.clone())) so execution uses snapshot.schema while keeping database
and user unchanged.

crates/queryflux-core/src/engine_registry.rs (1)

249-275: ⚠️ Potential issue | 🟠 Major

Reject partial auth objects instead of fabricating empty credentials.

Once authType is present, unwrap_or_default() turns malformed JSON into empty usernames, passwords, or tokens. With the new DB-backed factories, broken persisted auth now survives startup and fails later with much noisier runtime errors. Validate the required fields per auth type and return an error instead.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@crates/queryflux-core/src/engine_registry.rs` around lines 249 - 275, The
parser parse_auth_from_config_json should stop fabricating empty credentials:
change it to validate required fields per authType and return a
Result<ClusterAuth, Error> (e.g., anyhow::Error) instead of silently producing
defaults; locate the s closure and each match arm (Some("basic"), "bearer",
"keyPair", "accessKey", "roleArn") and replace unwrap_or_default() uses with
explicit checks that the needed keys (authUsername/authPassword/authToken as
appropriate) are present and non-empty, returning a descriptive Err when a
required field is missing, and only construct ClusterAuth on successful
validation.

crates/queryflux-e2e-tests/src/harness.rs (1)

293-296: ⚠️ Potential issue | 🟠 Major

The harness still never mounts the Snowflake frontend.

snowflake_sessions is now present in the state, but the server still serves only TrinoHttpFrontend::router(). /session/v1/login-request and /api/v2/statements remain unreachable in E2E, so the native Snowflake path isn't exercised here.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@crates/queryflux-e2e-tests/src/harness.rs` around lines 293 - 296, The test
harness creates SnowflakeSessionStore but never mounts the Snowflake frontend
router, so Snowflake endpoints remain unreachable; update the server
construction to mount the Snowflake frontend alongside Trino by instantiating
the Snowflake frontend (e.g., SnowflakeHttpFrontend::new(state, port) or the
actual Snowflake frontend constructor present in the repo) and merge or nest its
router with TrinoHttpFrontend::router() (use Router::merge or the project's
router-composition utility) so /session/v1/login-request and /api/v2/statements
are served; keep SnowflakeSessionStore::new(Default::default()) as the state
backing for that frontend.

🧹 Nitpick comments (5)

Cargo.toml (1)

85-86: Pin the Snowflake connector to an immutable revision.

Tracking branch = "main" leaves update intent implicit. Since this feature depends on fork-specific behavior, pin the exact commit or tag you validated so connector behavior can't drift unexpectedly.

Suggested manifest change

-snowflake-connector-rs = { git = "https://github.com/lakeops-org/snowflake-connector-rs", branch = "main" }
+snowflake-connector-rs = { git = "https://github.com/lakeops-org/snowflake-connector-rs", rev = "<validated-commit-sha>" }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@Cargo.toml` around lines 85 - 86, The dependency declaration for
snowflake-connector-rs currently tracks branch = "main", which can drift; change
the Cargo.toml entry for the snowflake-connector-rs dependency to pin an exact
immutable revision (use rev = "<commit-sha>" or a specific tag) instead of
branch = "main" so the fork-specific QueryExecutor::snowflake_columns behavior
remains stable; after updating the dependency, update Cargo.lock by running
cargo update -p snowflake-connector-rs (or cargo build) and record the chosen
commit SHA or tag in project docs or the PR for verification.

crates/queryflux-frontend/src/snowflake/http/session_store.rs (1)

207-219: Minor: timing-based test may be flaky under load.

The 40ms timeout with 80ms sleep relies on timing precision. On heavily loaded CI, this could fail. Consider using larger margins or mocking time.

However, given the "Chill" review mode and the reasonable margins used, this is acceptable.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@crates/queryflux-frontend/src/snowflake/http/session_store.rs` around lines
207 - 219, The idle_timeout_evicts_session test uses a tight 40ms timeout and
80ms sleep which can flake under CI; update the test by increasing the
idle_timeout and sleep margins (e.g., use 200ms/400ms) or replace real sleeps
with a time-mocking approach so SnowflakeSessionStore::new configured via
SnowflakeHttpSessionPolicy.idle_timeout and the calls to
validate_snowflake_session("t2") and get("t2") use deterministic time control;
locate and modify the idle_timeout value and the subsequent thread::sleep in the
idle_timeout_evicts_session test (or introduce a mockable clock used by
SnowflakeSessionStore) to eliminate timing fragility.

crates/queryflux-engine-adapters/src/snowflake/mod.rs (1)

570-575: Silent null coercion on extraction failure may hide data issues.

When row.at::<T>(col_idx) fails, the code appends null without logging. This could silently corrupt data if the Snowflake type doesn't match the expected Arrow type. Consider logging a warning on the first failure per column.

♻️ Optional improvement

 DataType::Boolean => {
     let mut b = BooleanBuilder::with_capacity(rows.len());
+    let mut warned = false;
     for row in rows {
         match row.at::<bool>(col_idx) {
             Ok(v) => b.append_value(v),
-            Err(_) => b.append_null(),
+            Err(e) => {
+                if !warned {
+                    tracing::warn!(col_idx, error = %e, "Coercing extraction failure to null");
+                    warned = true;
+                }
+                b.append_null()
+            }
         }
     }
     Ok(Arc::new(b.finish()))
 }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@crates/queryflux-engine-adapters/src/snowflake/mod.rs` around lines 570 -
575, The loop over rows currently swallows extraction errors by calling
b.append_null() when row.at::<bool>(col_idx) fails; add a one-time warning per
column to avoid silent data corruption: introduce a per-column "warned" flag
(e.g., a Vec<bool> indexed by col_idx) and when row.at::<bool>(col_idx) returns
Err(_), if warned[col_idx] is false emit a warning (using the crate's
logger/tracing) that the extraction for that column failed and a null was
inserted, then set warned[col_idx] = true; keep calling b.append_null() for
subsequent errors so behavior is unchanged but failures are now surfaced once.

crates/queryflux-frontend/src/snowflake/tests.rs (1)

254-256: Test startup sleep may cause flakiness.

The 20ms sleep assumes the server is ready within that time. On slow CI runners, this could cause intermittent failures. Consider replacing with a retry loop that polls the server's health endpoint.

♻️ Optional improvement

-    tokio::time::sleep(Duration::from_millis(20)).await;
+    // Wait for server to be ready with backoff
+    for _ in 0..50 {
+        if reqwest::get(format!("http://127.0.0.1:{port}/session/heartbeat"))
+            .await
+            .is_ok()
+        {
+            break;
+        }
+        tokio::time::sleep(Duration::from_millis(10)).await;
+    }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@crates/queryflux-frontend/src/snowflake/tests.rs` around lines 254 - 256, The
fixed test helper currently uses a fixed
tokio::time::sleep(Duration::from_millis(20)) before returning (port, tx), which
can flake on slow CI; replace the sleep with a retry loop that polls the spawned
server's health endpoint (e.g., GET /health) until it returns success or a short
overall timeout elapses (e.g., 3–5s), using a small backoff (50–200ms) between
attempts; implement this in the same helper that returns (port, tx) so it only
returns once the server responds OK, using an async HTTP client (reqwest or
hyper) and tokio::time::sleep for backoff and tokio::time::timeout for the
overall deadline.

crates/queryflux-frontend/src/snowflake/http/handlers/session.rs (1)

71-88: SessionContext variant doesn't match protocol, but routing is correct.

Using SessionContext::MySqlWire while passing FrontendProtocol::SnowflakeHttp is intentional since the ProtocolBasedRouter routes solely on FrontendProtocol (ignoring the session). However, this coupling is subtle.

Consider adding a brief comment explaining why MySqlWire is used as the session shape (e.g., "MySqlWire provides the session structure; routing uses SnowflakeHttp protocol").

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@crates/queryflux-frontend/src/snowflake/http/handlers/session.rs` around
lines 71 - 88, The use of SessionContext::MySqlWire while calling
router_chain.route with FrontendProtocol::SnowflakeHttp is intentional but
subtle; update the code around the SessionContext::MySqlWire instantiation
(and/or the router_chain.route call) to add a short comment clarifying that
MySqlWire is used only to satisfy the session shape and that routing decision is
made solely on FrontendProtocol::SnowflakeHttp (i.e., ProtocolBasedRouter
ignores session variant for routing), referencing SessionContext::MySqlWire,
router_chain.route, and FrontendProtocol::SnowflakeHttp so future readers
understand the coupling.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@crates/queryflux-frontend/src/snowflake/http/handlers/query.rs`:
- Around line 31-35: SnowflakeSink currently clones and buffers all RecordBatch
items into its batches Vec (see struct SnowflakeSink and the code paths that
append into batches and call sf_query_response()), which can exhaust memory for
large queries; add a hard cap (e.g., MAX_ROWS and/or MAX_BYTES constants) and
enforce it where batches are appended: before pushing a batch, increment a
tracked total_rows/total_bytes (use batch.num_rows() and approximate serialized
size), and if the cap would be exceeded set SnowflakeSink.error to a descriptive
message and stop buffering/return an error instead of pushing the batch;
optionally avoid extra cloning by storing Arc<RecordBatch> or similar to reduce
copies and ensure sf_query_response() respects the error state and returns early
when the cap is hit.

In `@crates/queryflux-persistence/src/cluster_config.rs`:
- Around line 150-161: The ClusterAuth::KeyPair branch currently only writes
authType and authUsername, so include the keypair fields when serializing to the
config map: insert private_key_pem and private_key_passphrase (or
private_key_passphrase if named differently) into the same config HashMap via
config.insert(...) alongside account/warehouse/role/schema so the JSONB blob
contains the full keypair credentials; update the serialization logic that
handles ClusterAuth::KeyPair (and the equivalent Snowflake config serialization
blocks) to persist these two fields so adapters rebuilt from the DB have the
private key and passphrase available.

---

Duplicate comments:
In `@crates/queryflux-core/src/engine_registry.rs`:
- Around line 249-275: The parser parse_auth_from_config_json should stop
fabricating empty credentials: change it to validate required fields per
authType and return a Result<ClusterAuth, Error> (e.g., anyhow::Error) instead
of silently producing defaults; locate the s closure and each match arm
(Some("basic"), "bearer", "keyPair", "accessKey", "roleArn") and replace
unwrap_or_default() uses with explicit checks that the needed keys
(authUsername/authPassword/authToken as appropriate) are present and non-empty,
returning a descriptive Err when a required field is missing, and only construct
ClusterAuth on successful validation.

In `@crates/queryflux-e2e-tests/src/harness.rs`:
- Around line 293-296: The test harness creates SnowflakeSessionStore but never
mounts the Snowflake frontend router, so Snowflake endpoints remain unreachable;
update the server construction to mount the Snowflake frontend alongside Trino
by instantiating the Snowflake frontend (e.g., SnowflakeHttpFrontend::new(state,
port) or the actual Snowflake frontend constructor present in the repo) and
merge or nest its router with TrinoHttpFrontend::router() (use Router::merge or
the project's router-composition utility) so /session/v1/login-request and
/api/v2/statements are served; keep
SnowflakeSessionStore::new(Default::default()) as the state backing for that
frontend.

In `@crates/queryflux-frontend/src/dispatch.rs`:
- Around line 255-272: The current branch that checks next_uri.is_none() only
finalizes Trino via finalize_trino_async_terminal_on_submit, leaving non-Trino
async executions orphaned; add a generic or adapter-specific terminal finalizer
call here so any engine that can produce a terminal POST with no nextUri can
clean up (or enforce at the adapter contract that this case cannot occur).
Concretely: in the same if next_uri.is_none() { if let Some(ref ib) =
initial_body { ... } } block, after the Trino-specific call, invoke a new
adapter method (e.g., adapter.finalize_async_terminal_on_submit(...) or a
generic finalize_async_terminal_on_submit(state, &cluster_manager, &executing,
&adapter, &session, protocol, ib)) for adapters where adapter.supports_async()
is true (or make supports_async() guarantee Trino semantics), ensuring cluster
lease/execution row cleanup for non-Trino engines as well.

In `@crates/queryflux-frontend/src/snowflake/http/handlers/query.rs`:
- Around line 162-168: The session context is being initialized with the saved
database but not the saved schema, causing execution to run against the wrong
schema; in the SessionContext::MySqlWire construction (where user, schema:
Some(database.clone()), session_vars: HashMap::new() are set) replace the schema
value to use the saved schema variable (e.g., schema.clone() or
Some(schema.clone())) so execution uses snapshot.schema while keeping database
and user unchanged.

---

Nitpick comments:
In `@Cargo.toml`:
- Around line 85-86: The dependency declaration for snowflake-connector-rs
currently tracks branch = "main", which can drift; change the Cargo.toml entry
for the snowflake-connector-rs dependency to pin an exact immutable revision
(use rev = "<commit-sha>" or a specific tag) instead of branch = "main" so the
fork-specific QueryExecutor::snowflake_columns behavior remains stable; after
updating the dependency, update Cargo.lock by running cargo update -p
snowflake-connector-rs (or cargo build) and record the chosen commit SHA or tag
in project docs or the PR for verification.

In `@crates/queryflux-engine-adapters/src/snowflake/mod.rs`:
- Around line 570-575: The loop over rows currently swallows extraction errors
by calling b.append_null() when row.at::<bool>(col_idx) fails; add a one-time
warning per column to avoid silent data corruption: introduce a per-column
"warned" flag (e.g., a Vec<bool> indexed by col_idx) and when
row.at::<bool>(col_idx) returns Err(_), if warned[col_idx] is false emit a
warning (using the crate's logger/tracing) that the extraction for that column
failed and a null was inserted, then set warned[col_idx] = true; keep calling
b.append_null() for subsequent errors so behavior is unchanged but failures are
now surfaced once.

In `@crates/queryflux-frontend/src/snowflake/http/handlers/session.rs`:
- Around line 71-88: The use of SessionContext::MySqlWire while calling
router_chain.route with FrontendProtocol::SnowflakeHttp is intentional but
subtle; update the code around the SessionContext::MySqlWire instantiation
(and/or the router_chain.route call) to add a short comment clarifying that
MySqlWire is used only to satisfy the session shape and that routing decision is
made solely on FrontendProtocol::SnowflakeHttp (i.e., ProtocolBasedRouter
ignores session variant for routing), referencing SessionContext::MySqlWire,
router_chain.route, and FrontendProtocol::SnowflakeHttp so future readers
understand the coupling.

In `@crates/queryflux-frontend/src/snowflake/http/session_store.rs`:
- Around line 207-219: The idle_timeout_evicts_session test uses a tight 40ms
timeout and 80ms sleep which can flake under CI; update the test by increasing
the idle_timeout and sleep margins (e.g., use 200ms/400ms) or replace real
sleeps with a time-mocking approach so SnowflakeSessionStore::new configured via
SnowflakeHttpSessionPolicy.idle_timeout and the calls to
validate_snowflake_session("t2") and get("t2") use deterministic time control;
locate and modify the idle_timeout value and the subsequent thread::sleep in the
idle_timeout_evicts_session test (or introduce a mockable clock used by
SnowflakeSessionStore) to eliminate timing fragility.

In `@crates/queryflux-frontend/src/snowflake/tests.rs`:
- Around line 254-256: The fixed test helper currently uses a fixed
tokio::time::sleep(Duration::from_millis(20)) before returning (port, tx), which
can flake on slow CI; replace the sleep with a retry loop that polls the spawned
server's health endpoint (e.g., GET /health) until it returns success or a short
overall timeout elapses (e.g., 3–5s), using a small backoff (50–200ms) between
attempts; implement this in the same helper that returns (port, tx) so it only
returns once the server responds OK, using an async HTTP client (reqwest or
hyper) and tokio::time::sleep for backoff and tokio::time::timeout for the
overall deadline.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: f91e48d3-6c6c-4f00-a08a-efd9c06d896a

📥 Commits

Reviewing files that changed from the base of the PR and between b5b13f7 and ad6d898.

⛔ Files ignored due to path filters (1)

• Cargo.lock is excluded by !**/*.lock

📒 Files selected for processing (20)

• Cargo.toml
• crates/queryflux-core/src/config.rs
• crates/queryflux-core/src/engine_registry.rs
• crates/queryflux-e2e-tests/Cargo.toml
• crates/queryflux-e2e-tests/src/harness.rs
• crates/queryflux-e2e-tests/src/lib.rs
• crates/queryflux-e2e-tests/src/snowflake_rs_client.rs
• crates/queryflux-e2e-tests/tests/snowflake_connector_rs_tests.rs
• crates/queryflux-engine-adapters/Cargo.toml
• crates/queryflux-engine-adapters/src/snowflake/mod.rs
• crates/queryflux-engine-adapters/src/starrocks/mod.rs
• crates/queryflux-frontend/src/dispatch.rs
• crates/queryflux-frontend/src/snowflake/http/handlers/query.rs
• crates/queryflux-frontend/src/snowflake/http/handlers/session.rs
• crates/queryflux-frontend/src/snowflake/http/handlers/token.rs
• crates/queryflux-frontend/src/snowflake/http/session_store.rs
• crates/queryflux-frontend/src/snowflake/tests.rs
• crates/queryflux-frontend/src/state.rs
• crates/queryflux-persistence/src/cluster_config.rs
• crates/queryflux/src/main.rs

✅ Files skipped from review due to trivial changes (3)

• crates/queryflux-e2e-tests/Cargo.toml
• crates/queryflux-e2e-tests/src/lib.rs
• crates/queryflux-engine-adapters/Cargo.toml

🚧 Files skipped from review as they are similar to previous changes (3)

• crates/queryflux-frontend/src/state.rs
• crates/queryflux-frontend/src/snowflake/http/handlers/token.rs
• crates/queryflux/src/main.rs

[coderabbitai]

⚠️ Potential issue | 🟠 Major

This handler buffers the full result set in memory.

Every batch is cloned into batches and held until sf_query_response() serializes the response. One large query can therefore consume unbounded frontend memory. If streaming isn't ready yet, add a hard row/byte cap and fail early.

Also applies to: 56-59, 92-100

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@crates/queryflux-frontend/src/snowflake/http/handlers/query.rs` around lines
31 - 35, SnowflakeSink currently clones and buffers all RecordBatch items into
its batches Vec (see struct SnowflakeSink and the code paths that append into
batches and call sf_query_response()), which can exhaust memory for large
queries; add a hard cap (e.g., MAX_ROWS and/or MAX_BYTES constants) and enforce
it where batches are appended: before pushing a batch, increment a tracked
total_rows/total_bytes (use batch.num_rows() and approximate serialized size),
and if the cap would be exceeded set SnowflakeSink.error to a descriptive
message and stop buffering/return an error instead of pushing the batch;
optionally avoid extra cloning by storing Arc<RecordBatch> or similar to reduce
copies and ensure sf_query_response() respects the error state and returns early
when the cap is hit.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Snowflake keyPair auth still won't survive a DB round-trip.

These new Snowflake fields are now persisted, but ClusterAuth::KeyPair still writes only authType and authUsername. Because startup now rebuilds adapters from the JSONB blob, a Postgres-backed Snowflake cluster seeded from YAML comes back without private_key_pem/passphrase and can't authenticate.

Also applies to: 242-245

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@crates/queryflux-persistence/src/cluster_config.rs` around lines 150 - 161,
The ClusterAuth::KeyPair branch currently only writes authType and authUsername,
so include the keypair fields when serializing to the config map: insert
private_key_pem and private_key_passphrase (or private_key_passphrase if named
differently) into the same config HashMap via config.insert(...) alongside
account/warehouse/role/schema so the JSONB blob contains the full keypair
credentials; update the serialization logic that handles ClusterAuth::KeyPair
(and the equivalent Snowflake config serialization blocks) to persist these two
fields so adapters rebuilt from the DB have the private key and passphrase
available.

[coderabbitai]

Actionable comments posted: 9

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

crates/queryflux/src/main.rs (1)

1187-1226: ⚠️ Potential issue | 🟠 Major

Compare against the previous fingerprint, not the mutated cache.

cache.config_json is overwritten with the current fingerprint in the adapter loop and then reused for config_unchanged, so that later check always succeeds for enabled clusters. After an engine/endpoint/credential change, the new ClusterState still inherits the old running/queued counters, which can leave capacity artificially consumed even though the adapter was rebuilt.

Also applies to: 1281-1307

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@crates/queryflux/src/main.rs` around lines 1187 - 1226, The loop currently
overwrites cache.config_json with the new cfg_json before the
reuse/config_unchanged decision, causing comparisons to use the mutated cache;
instead capture the previous fingerprint from cache.config_json (e.g., let
prev_cfg = cache.config_json.get(cluster_name_str.as_str()).map(String::as_str))
and compare prev_cfg to the newly computed cfg_json.as_str() to decide reuse,
only inserting/updating cache.config_json after the reuse check and after a
successful adapter build (functions/values: cache.config_json, cfg_json, reuse,
cluster_records, ClusterName, registered_engines::build_adapter_from_record);
alternatively snapshot the entire cache.config_json before the loop and compare
against that snapshot, then update the live cache after building the adapter so
running/queued counters aren’t incorrectly preserved.

♻️ Duplicate comments (1)

crates/queryflux-engine-adapters/src/snowflake/mod.rs (1)

396-401: ⚠️ Potential issue | 🔴 Critical

Escape table before interpolating DESCRIBE TABLE.

catalog and database are quoted, but table is still inserted raw. A crafted table name containing " can break the identifier and inject extra SQL into the metadata query.

Suggested fix

         let qualified = format!(
-            "\"{}\".\"{}\".\"{table}\"",
+            "\"{}\".\"{}\".\"{}\"",
             catalog.replace('"', "\"\""),
             database.replace('"', "\"\""),
+            table.replace('"', "\"\""),
         );

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@crates/queryflux-engine-adapters/src/snowflake/mod.rs` around lines 396 -
401, The DESCRIBE TABLE query builds qualified with catalog and database safely
quoted but interpolates table raw; fix by escaping and quoting the table
identifier before building the query (mirror the existing
catalog.replace('"','""') and database.replace('"','""') logic) so that the
variable used in qualified (or a new escaped_table identifier) replaces
instances of " with "" and is wrapped in quotes; update the construction used by
run_query(&format!("DESCRIBE TABLE {qualified}")) to use the safely quoted table
identifier (or add a small helper like escape_identifier used by qualified,
database, and catalog).

🧹 Nitpick comments (8)

crates/queryflux-cluster-manager/src/cluster_state.rs (1)

98-100: Add documentation consistent with set_running_queries.

The sibling method set_running_queries (lines 91-96) includes a doc comment explaining its purpose, caller, and clamping behavior. This new method should follow the same convention.

Additionally, as noted in the caller at main.rs:1307, when config reconciliation overwrites this counter with a snapshot value, any concurrent increment_queued/decrement_queued operations in flight will be lost. If this is intentional (matching the existing pattern for set_running_queries), documenting that trade-off helps future maintainers.

📝 Suggested documentation

+    /// Overwrite the queued query counter with a ground-truth value.
+    /// Called by the background reconciler when config is unchanged.
+    /// Note: concurrent increment/decrement calls may be lost; this is
+    /// acceptable since the reconciler periodically resyncs from the snapshot.
     pub fn set_queued_queries(&self, count: u64) {
         self.queued_queries.store(count, Ordering::Relaxed);
     }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@crates/queryflux-cluster-manager/src/cluster_state.rs` around lines 98 - 100,
Add a doc comment to the set_queued_queries method matching the style of
set_running_queries: describe that it sets the queued query count (used by
callers to observe or replace the counter), note the caller behavior (config
reconciliation in main.rs overwrites this counter with snapshot values and can
drop any concurrent increment_queued/decrement_queued operations), and document
any clamping or ordering semantics (e.g., it stores the provided count with
Ordering::Relaxed and does not merge concurrent updates). Reference the sibling
method name set_running_queries in the comment so maintainers see the consistent
pattern and trade-offs.

crates/queryflux-frontend/src/snowflake/http/handlers/common.rs (3)

134-163: Consider expanding test coverage.

The existing tests cover the happy paths for gzip decoding. Consider adding tests for:

• extract_snowflake_token: quoted/unquoted tokens, empty tokens, missing header, malformed header
• passthrough_headers: verify excluded headers are filtered and allowed headers pass through
• Malformed gzip data to verify error handling

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@crates/queryflux-frontend/src/snowflake/http/handlers/common.rs` around lines
134 - 163, Add unit tests to cover edge cases beyond the existing gzip happy
paths: for extract_snowflake_token add tests for quoted token (e.g. "token"),
unquoted token, empty token value, missing header, and malformed header formats
to assert correct Ok/Err behavior; for passthrough_headers add tests that
headers in the exclusion set are removed while allowed headers are preserved and
header values are unchanged; and add a test that passes deliberately corrupted
gzip bytes to decode_snowflake_request_body to assert it returns an error.
Reference the functions extract_snowflake_token, passthrough_headers, and
decode_snowflake_request_body when adding these tests.

---

60-71: Unused _status parameter could be removed or utilized.

The _status parameter is accepted but never used. While the documentation explains why HTTP 200 is always returned, accepting an unused parameter may confuse callers who expect their status to be reflected. Consider either removing it or logging it for debugging purposes.

♻️ Option 1: Remove unused parameter

-pub fn sf_error(_status: StatusCode, code: u64, message: &str) -> Response {
+pub fn sf_error(code: u64, message: &str) -> Response {

♻️ Option 2: Document in the body for debugging (if callers rely on passing status)

 pub fn sf_error(_status: StatusCode, code: u64, message: &str) -> Response {
+    // Note: _status is intentionally ignored; Snowflake protocol requires HTTP 200.
+    // See function doc for rationale.
     (

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@crates/queryflux-frontend/src/snowflake/http/handlers/common.rs` around lines
60 - 71, The _status parameter on sf_error is unused and confusing; either
remove it from the sf_error(StatusCode, u64, &str) signature and update all
callers to call sf_error(code, message) (or create an overloaded helper) so only
the code/message are passed, or keep the parameter but use it by renaming to
status: StatusCode and include its value in the JSON payload (e.g., "status":
status.as_u16() or status.to_string()) and/or log it inside sf_error; update all
callers to reflect the chosen signature and ensure the Response still returns
HTTP 200 as intended.

---

41-51: Consider case-insensitive matching for the auth scheme prefix.

Per RFC 7235, authentication scheme names are case-insensitive. The current strip_prefix("Snowflake Token=") requires exact casing, which could reject valid tokens from clients using different casing (e.g., snowflake token=).

♻️ Suggested case-insensitive extraction

 pub fn extract_snowflake_token(headers: &HeaderMap) -> Option<String> {
     let auth = headers.get("authorization")?.to_str().ok()?;
-    // Handle both `Snowflake Token="..."` and `Snowflake Token=...`
-    let rest = auth.strip_prefix("Snowflake Token=")?;
+    // Handle both `Snowflake Token="..."` and `Snowflake Token=...` (case-insensitive prefix)
+    let prefix = "Snowflake Token=";
+    if auth.len() < prefix.len() || !auth[..prefix.len()].eq_ignore_ascii_case(prefix) {
+        return None;
+    }
+    let rest = &auth[prefix.len()..];
     let token = rest.trim_matches('"');
     if token.is_empty() {
         None
     } else {
         Some(token.to_string())
     }
 }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@crates/queryflux-frontend/src/snowflake/http/handlers/common.rs` around lines
41 - 51, The extract_snowflake_token function only accepts the exact "Snowflake
Token=" casing; change it to perform a case-insensitive check for that prefix
(per RFC 7235) by using a lowercase view of the header (e.g.,
auth.to_ascii_lowercase().strip_prefix("snowflake token=")) to detect the
prefix, then derive the original remainder from the original auth string at the
same offset to preserve quoting before trimming quotes and returning the token;
update the logic around strip_prefix("Snowflake Token=") in
extract_snowflake_token to implement this.

crates/queryflux-e2e-tests/src/harness.rs (1)

399-406: Inconsistent default port in fallback.

The FAKESNOW_URL default on line 186 uses port 18085, but is_fakesnow_ready defaults to port 8085 on line 404. This only matters when a URL without a port is passed, but aligning them would be more consistent.

♻️ Suggested fix

 async fn is_fakesnow_ready(url: &str) -> bool {
     let Ok(parsed) = reqwest::Url::parse(url) else {
         return false;
     };
     let host = parsed.host_str().unwrap_or("localhost");
-    let port = parsed.port().unwrap_or(8085);
+    let port = parsed.port().unwrap_or(18085);
     port_is_open(host, port).await
 }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@crates/queryflux-e2e-tests/src/harness.rs` around lines 399 - 406, The
default port used when parsing a URL in is_fakesnow_ready is inconsistent with
the FAKESNOW_URL default; update is_fakesnow_ready so that
parsed.port().unwrap_or(18085) (instead of 8085) is used, leaving the host
extraction and the port_is_open(host, port).await call unchanged; this keeps the
default port consistent with the FAKESNOW_URL constant used elsewhere.

crates/queryflux-core/src/config.rs (1)

355-362: Document that 0 means no limit.

The comments mention "Omitted → 86400 (24h)" and "Omitted → 14400 (4h)" but don't mention that 0 disables the limit. Based on the relevant code snippet from session_store.rs, Some(0) means no limit.

📝 Suggested documentation improvement

-    /// **Snowflake HTTP wire only** — max session lifetime in seconds (from login).
-    /// Omitted → 86400 (24h). `0` = no max-age limit.
+    /// **Snowflake HTTP wire only** — max session lifetime in seconds (from login).
+    /// Omitted or `null` → 86400 (24h). `0` = no max-age limit (sessions never expire by age).
     #[serde(default)]
     pub snowflake_session_max_age_secs: Option<u64>,
-    /// **Snowflake HTTP wire only** — idle timeout in seconds since the last validated request
-    /// (heartbeat, token refresh, query). Omitted → 14400 (4h). `0` = no idle limit.
+    /// **Snowflake HTTP wire only** — idle timeout in seconds since the last validated request
+    /// (heartbeat, token refresh, query). Omitted or `null` → 14400 (4h). `0` = no idle timeout.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@crates/queryflux-core/src/config.rs` around lines 355 - 362, The doc comments
for the config fields snowflake_session_max_age_secs and
snowflake_session_idle_timeout_secs only state the omitted defaults but omit
that a value of 0 disables the limit; update the comments for both fields to
explicitly mention that `0` means "no limit" (consistent with the behavior in
session_store.rs where Some(0) disables the limit) so readers know that
providing Some(0) turns off the max-age/idle timeout.

crates/queryflux-e2e-tests/tests/snowflake_tests.rs (1)

102-144: Consider using unique identifiers for DDL test isolation.

The test creates e2e_db.e2e_schema.test_tbl which could conflict across parallel test runs or leave artifacts on repeated local runs. While CREATE OR REPLACE TABLE handles the table, the database and schema may accumulate.

💡 Optional: Use unique identifiers

// Consider generating unique names per test run:
let suffix = format!("{}_{}", std::process::id(), chrono::Utc::now().timestamp_millis());
let db_name = format!("e2e_db_{suffix}");
// ... and cleanup at the end

Alternatively, since fakesnow is ephemeral per docker-compose session, this may be acceptable as-is.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@crates/queryflux-e2e-tests/tests/snowflake_tests.rs` around lines 102 - 144,
The test function snowflake_create_and_query_table() uses fixed names (e2e_db,
e2e_schema, test_tbl) which can conflict across parallel runs or leave
artifacts; modify the test to generate unique db/schema/table identifiers (e.g.,
build db_name, schema_name, table_name with a suffix from std::process::id() and
chrono::Utc::now().timestamp_millis()), use those variables in every
execute_on() statement (CREATE DATABASE, CREATE SCHEMA, CREATE OR REPLACE TABLE,
INSERT, SELECT), and optionally drop the created schema/database at the end of
the test to clean up.

crates/queryflux-frontend/src/dispatch.rs (1)

362-410: Avoid cloning engine_stats three times.

The engine_stats value is cloned identically into all three branches. Consider moving the record_query call outside the conditional to reduce duplication.

♻️ Suggested refactor

     let engine_stats = Some(QueryEngineStats { ... });
     let backend_id = Some(executing.backend_query_id.0.clone());

+    let (status, error) = if let Some(err) = &trino_resp.error {
+        (QueryStatus::Failed, Some(err.message.clone()))
+    } else if trino_resp.stats.state == "FAILED" {
+        (QueryStatus::Failed, Some("Trino query FAILED".to_string()))
+    } else {
+        (QueryStatus::Success, None)
+    };
+
+    state
+        .metrics
+        .on_query_finished(&executing.cluster_group.0, &executing.cluster_name.0);
+    state.record_query(
+        &ctx,
+        QueryOutcome {
+            backend_query_id: backend_id,
+            status,
+            execution_ms: elapsed_ms,
+            rows: None,
+            error,
+            routing_trace: None,
+            engine_stats,
+        },
+    );
-    if let Some(err) = &trino_resp.error {
-        state.metrics.on_query_finished(...);
-        state.record_query(...);
-    } else if trino_resp.stats.state == "FAILED" {
-        state.metrics.on_query_finished(...);
-        state.record_query(...);
-    } else {
-        state.metrics.on_query_finished(...);
-        state.record_query(...);
-    }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@crates/queryflux-frontend/src/dispatch.rs` around lines 362 - 410, The code
clones engine_stats into three identical QueryOutcome constructions; instead,
determine the status and optional error first (inspect trino_resp.error and
trino_resp.stats.state), then call
state.metrics.on_query_finished(&executing.cluster_group.0,
&executing.cluster_name.0) and make a single state.record_query(&ctx,
QueryOutcome { backend_query_id: backend_id, status: <chosen>, execution_ms:
elapsed_ms, rows: None, error: <opt_error>, routing_trace: None, engine_stats,
}); This removes the duplicated QueryOutcome construction and avoids cloning
engine_stats multiple times while keeping the existing checks around
trino_resp.error and trino_resp.stats.state.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@crates/queryflux-engine-adapters/src/snowflake/mod.rs`:
- Around line 369-387: The methods list_catalogs, list_databases, and
list_tables currently call run_first_col (which returns column 0) but
Snowflake's SHOW commands put the object name in column 1; change each to use
run_second_col (or otherwise fetch index 1) instead of run_first_col so they
return the name rather than created_on, keeping the existing SQL/identifier
escaping (catalog.replace(...) and database.replace(...)) intact.
- Around line 313-366: Replace the unbounded mpsc channel with a bounded one to
restore backpressure: create a
tokio::sync::mpsc::channel::<Result<RecordBatch>>(capacity) instead of
unbounded_channel(), use the async tx.send(batch).await call and check its
Result; if send returns Err (receiver closed) stop the producer and return so
you stop calling executor.fetch_next_chunk() and building more batches. Update
the stream wrapper to use tokio_stream::wrappers::ReceiverStream (or
ReceiverStream::new) instead of UnboundedReceiverStream and return
Box::pin(ReceiverStream::new(rx)). Ensure to adjust all references to
tx.send(...) to await and handle send errors by breaking/returning, and keep
existing error handling around executor.fetch_next_chunk() and
build_snowflake_record_batch().
- Around line 534-543: The Snowflake FIXED type mapping in
snowflake_type_to_arrow is unsafe: change the "fixed" arm to always return
DataType::Utf8 (do not branch on scale) to avoid silent overflow; then update
any code paths that assume Int64 (the places calling row.at::<i64>() and
subsequent conversion that produced nulls) to read the value as a string/Utf8
(e.g., row.at::<String>() or equivalent extraction) or pass through the raw
string value so large DECIMAL/NUMBER values are preserved; alternatively, if the
connector supports Decimal128/Decimal256 extraction, replace the Utf8 mapping
with those Decimal Arrow types and adjust the extraction code accordingly (refer
to snowflake_type_to_arrow and the call sites using row.at::<i64>() to locate
all places to change).

In `@crates/queryflux-frontend/src/snowflake/http/format.rs`:
- Around line 257-260: The split of a signed nanoseconds timestamp into epoch
and fractional parts currently uses normal division and remainder, producing
negative fractions for pre-epoch times; replace the operations on nanos with
Euclidean division such that epoch = nanos.div_euclid(1_000_000_000) and
fraction = nanos.rem_euclid(1_000_000_000) cast to i32 (preserving non-negative
fractional part) in the code that computes (Some(epoch), Some(fraction))—update
the logic in the function/block where nanos is read (the snippet calculating
epoch and fraction) to use div_euclid/rem_euclid and ensure the fraction is
within i32 range before wrapping in Some(...).
- Around line 26-30: SfTypeInfo currently stores precision/scale/char_length as
u32 and casts Arrow's signed scale (i8) into u32, which overflows for negative
scales; update handling by changing SfTypeInfo.scale to a signed type (e.g., i8
or i32) or validate the incoming scale before casting, explicitly rejecting or
handling negative scales when building rowtype/field metadata; ensure
serialization code that emits Snowflake JSON converts the signed scale to the
expected JSON numeric form (and returns an error or maps negative scales
appropriately) rather than performing a blind as u32 cast so negative scales
cannot corrupt metadata.
- Around line 220-223: The Time32 branch currently casts the array directly to
Int64; instead detect the Time32(unit) variant, cast the array to Int64, then
scale by the appropriate multiplier before returning (multiply by 1_000_000_000
for Second and 1_000_000 for Millisecond). Use the same pattern as the
Time64(Microsecond) handling (reference the Time64(Microsecond) case) but for
DataType::Time32: after arrow::compute::cast(arr, &DataType::Int64) use
arrow::compute::multiply (or the equivalent scalar-multiply helper) with an
Int64 scalar multiplier so all Time32 values are converted to nanoseconds before
returning.
- Around line 209-214: The code currently masks multiple Arrow conversion and
IPC errors and still returns success in sf_query_response; fix by propagating
and returning errors instead of fabricating values or logging only.
Specifically: in the Time64 microsecond conversion branch where you downcast
cast.as_any().downcast_ref::<Int64Array>() and currently use unwrap_or_else(||
Int64Array::from(vec![0i64; arr.len()])), return a proper Err on downcast/cast
failure; likewise for the UInt64 cast branch that returns the original uncast
array on failure, propagate the cast error. Ensure RecordBatch::try_new failures
are not silently dropped but returned to the caller, and convert tracing-only
IPC errors (write/finish) into returned errors so sf_query_response can set
"success": false and include the error. Update functions that call these symbols
(cast, Int64Array conversion, RecordBatch::try_new, IPC write/finish, and
sf_query_response) to bubble up Result types instead of swallowing failures.

In `@crates/queryflux-frontend/src/snowflake/sql_api/handlers.rs`:
- Around line 179-206: The handler parses body_json but ignores Snowflake fields
database, schema, warehouse, and role; update the code after obtaining body_json
and sql to extract these keys (e.g., body_json["database"].as_str().filter(|s|
!s.trim().is_empty()) etc.) and apply them to the session context creation: set
SessionContext::MySqlWire.schema to the provided schema (if any) and propagate
database, warehouse, and role into the session (either as session_vars or
appropriate fields used by downstream routing/adaptation) so the execution uses
the requested context instead of user defaults; ensure values are cloned/owned
like sql and preserved when building session_ctx and when calling
routing/adaptation functions.

---

Outside diff comments:
In `@crates/queryflux/src/main.rs`:
- Around line 1187-1226: The loop currently overwrites cache.config_json with
the new cfg_json before the reuse/config_unchanged decision, causing comparisons
to use the mutated cache; instead capture the previous fingerprint from
cache.config_json (e.g., let prev_cfg =
cache.config_json.get(cluster_name_str.as_str()).map(String::as_str)) and
compare prev_cfg to the newly computed cfg_json.as_str() to decide reuse, only
inserting/updating cache.config_json after the reuse check and after a
successful adapter build (functions/values: cache.config_json, cfg_json, reuse,
cluster_records, ClusterName, registered_engines::build_adapter_from_record);
alternatively snapshot the entire cache.config_json before the loop and compare
against that snapshot, then update the live cache after building the adapter so
running/queued counters aren’t incorrectly preserved.

---

Duplicate comments:
In `@crates/queryflux-engine-adapters/src/snowflake/mod.rs`:
- Around line 396-401: The DESCRIBE TABLE query builds qualified with catalog
and database safely quoted but interpolates table raw; fix by escaping and
quoting the table identifier before building the query (mirror the existing
catalog.replace('"','""') and database.replace('"','""') logic) so that the
variable used in qualified (or a new escaped_table identifier) replaces
instances of " with "" and is wrapped in quotes; update the construction used by
run_query(&format!("DESCRIBE TABLE {qualified}")) to use the safely quoted table
identifier (or add a small helper like escape_identifier used by qualified,
database, and catalog).

---

Nitpick comments:
In `@crates/queryflux-cluster-manager/src/cluster_state.rs`:
- Around line 98-100: Add a doc comment to the set_queued_queries method
matching the style of set_running_queries: describe that it sets the queued
query count (used by callers to observe or replace the counter), note the caller
behavior (config reconciliation in main.rs overwrites this counter with snapshot
values and can drop any concurrent increment_queued/decrement_queued
operations), and document any clamping or ordering semantics (e.g., it stores
the provided count with Ordering::Relaxed and does not merge concurrent
updates). Reference the sibling method name set_running_queries in the comment
so maintainers see the consistent pattern and trade-offs.

In `@crates/queryflux-core/src/config.rs`:
- Around line 355-362: The doc comments for the config fields
snowflake_session_max_age_secs and snowflake_session_idle_timeout_secs only
state the omitted defaults but omit that a value of 0 disables the limit; update
the comments for both fields to explicitly mention that `0` means "no limit"
(consistent with the behavior in session_store.rs where Some(0) disables the
limit) so readers know that providing Some(0) turns off the max-age/idle
timeout.

In `@crates/queryflux-e2e-tests/src/harness.rs`:
- Around line 399-406: The default port used when parsing a URL in
is_fakesnow_ready is inconsistent with the FAKESNOW_URL default; update
is_fakesnow_ready so that parsed.port().unwrap_or(18085) (instead of 8085) is
used, leaving the host extraction and the port_is_open(host, port).await call
unchanged; this keeps the default port consistent with the FAKESNOW_URL constant
used elsewhere.

In `@crates/queryflux-e2e-tests/tests/snowflake_tests.rs`:
- Around line 102-144: The test function snowflake_create_and_query_table() uses
fixed names (e2e_db, e2e_schema, test_tbl) which can conflict across parallel
runs or leave artifacts; modify the test to generate unique db/schema/table
identifiers (e.g., build db_name, schema_name, table_name with a suffix from
std::process::id() and chrono::Utc::now().timestamp_millis()), use those
variables in every execute_on() statement (CREATE DATABASE, CREATE SCHEMA,
CREATE OR REPLACE TABLE, INSERT, SELECT), and optionally drop the created
schema/database at the end of the test to clean up.

In `@crates/queryflux-frontend/src/dispatch.rs`:
- Around line 362-410: The code clones engine_stats into three identical
QueryOutcome constructions; instead, determine the status and optional error
first (inspect trino_resp.error and trino_resp.stats.state), then call
state.metrics.on_query_finished(&executing.cluster_group.0,
&executing.cluster_name.0) and make a single state.record_query(&ctx,
QueryOutcome { backend_query_id: backend_id, status: <chosen>, execution_ms:
elapsed_ms, rows: None, error: <opt_error>, routing_trace: None, engine_stats,
}); This removes the duplicated QueryOutcome construction and avoids cloning
engine_stats multiple times while keeping the existing checks around
trino_resp.error and trino_resp.stats.state.

In `@crates/queryflux-frontend/src/snowflake/http/handlers/common.rs`:
- Around line 134-163: Add unit tests to cover edge cases beyond the existing
gzip happy paths: for extract_snowflake_token add tests for quoted token (e.g.
"token"), unquoted token, empty token value, missing header, and malformed
header formats to assert correct Ok/Err behavior; for passthrough_headers add
tests that headers in the exclusion set are removed while allowed headers are
preserved and header values are unchanged; and add a test that passes
deliberately corrupted gzip bytes to decode_snowflake_request_body to assert it
returns an error. Reference the functions extract_snowflake_token,
passthrough_headers, and decode_snowflake_request_body when adding these tests.
- Around line 60-71: The _status parameter on sf_error is unused and confusing;
either remove it from the sf_error(StatusCode, u64, &str) signature and update
all callers to call sf_error(code, message) (or create an overloaded helper) so
only the code/message are passed, or keep the parameter but use it by renaming
to status: StatusCode and include its value in the JSON payload (e.g., "status":
status.as_u16() or status.to_string()) and/or log it inside sf_error; update all
callers to reflect the chosen signature and ensure the Response still returns
HTTP 200 as intended.
- Around line 41-51: The extract_snowflake_token function only accepts the exact
"Snowflake Token=" casing; change it to perform a case-insensitive check for
that prefix (per RFC 7235) by using a lowercase view of the header (e.g.,
auth.to_ascii_lowercase().strip_prefix("snowflake token=")) to detect the
prefix, then derive the original remainder from the original auth string at the
same offset to preserve quoting before trimming quotes and returning the token;
update the logic around strip_prefix("Snowflake Token=") in
extract_snowflake_token to implement this.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 7c65f795-20bc-4189-8b01-6e1429b12f90

📥 Commits

Reviewing files that changed from the base of the PR and between ad6d898 and 39dbbf1.

📒 Files selected for processing (24)

• crates/queryflux-cluster-manager/src/cluster_state.rs
• crates/queryflux-core/src/config.rs
• crates/queryflux-core/src/engine_registry.rs
• crates/queryflux-e2e-tests/src/harness.rs
• crates/queryflux-e2e-tests/src/trino_client.rs
• crates/queryflux-e2e-tests/tests/snowflake_tests.rs
• crates/queryflux-engine-adapters/src/athena/mod.rs
• crates/queryflux-engine-adapters/src/duckdb/http.rs
• crates/queryflux-engine-adapters/src/duckdb/mod.rs
• crates/queryflux-engine-adapters/src/lib.rs
• crates/queryflux-engine-adapters/src/snowflake/mod.rs
• crates/queryflux-engine-adapters/src/starrocks/mod.rs
• crates/queryflux-engine-adapters/src/trino/mod.rs
• crates/queryflux-frontend/src/dispatch.rs
• crates/queryflux-frontend/src/snowflake/http/format.rs
• crates/queryflux-frontend/src/snowflake/http/handlers/common.rs
• crates/queryflux-frontend/src/snowflake/http/handlers/query.rs
• crates/queryflux-frontend/src/snowflake/mod.rs
• crates/queryflux-frontend/src/snowflake/proxy.rs
• crates/queryflux-frontend/src/snowflake/sql_api/handlers.rs
• crates/queryflux/src/main.rs
• crates/queryflux/src/registered_engines.rs
• website/docs/architecture/system-map.md
• website/versioned_docs/version-0.1.0/architecture/system-map.md

✅ Files skipped from review due to trivial changes (2)

• website/versioned_docs/version-0.1.0/architecture/system-map.md
• website/docs/architecture/system-map.md

🚧 Files skipped from review as they are similar to previous changes (5)

• crates/queryflux-engine-adapters/src/duckdb/mod.rs
• crates/queryflux-frontend/src/snowflake/proxy.rs
• crates/queryflux-frontend/src/snowflake/http/handlers/query.rs
• crates/queryflux-core/src/engine_registry.rs
• crates/queryflux-engine-adapters/src/athena/mod.rs

[coderabbitai]

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

# First, let's read the actual file to confirm the code at lines 313-366
sed -n '310,370p' crates/queryflux-engine-adapters/src/snowflake/mod.rs

Repository: lakeops-org/queryflux

Length of output: 2231

---

🏁 Script executed:

# Check the imports at the top of the file to see current usage
head -50 crates/queryflux-engine-adapters/src/snowflake/mod.rs | grep -E "use.*tokio|use.*ReceiverStream"

Repository: lakeops-org/queryflux

Length of output: 117

---

🌐 Web query:

Tokio unbounded_channel buffering behavior documentation

💡 Result:

Tokio's tokio::sync::mpsc::unbounded_channel creates an unbounded multi-producer, single-consumer (MPSC) channel with no fixed buffer capacity limit. Sends always succeed immediately (without backpressure) as long as the receiver is not dropped/closed, and messages are buffered arbitrarily in memory if the receiver lags behind. The sole limit is available system memory; excessive use can exhaust RAM and abort the process. Key behaviors from official docs: - pub fn unbounded_channel -> (UnboundedSender, UnboundedReceiver) - Internally uses a linked list of fixed-size blocks (32 messages per block for MPSC channels, ~1 byte overhead per message). - Blocks are deallocated when fully emptied, with some reuse logic, but allocators may not return memory to the OS promptly. - Receiver methods like recv.await wait for messages or closure; recv_many(&mut buffer, limit) batches up to limit items. Use cautiously in production to avoid OOM crashes; prefer bounded channels for backpressure.

Citations:

• 1: https://docs.rs/tokio/latest/tokio/sync/mpsc/fn.unbounded_channel.html
• 2: https://docs.rs/tokio/1.11.0/tokio/sync/mpsc/fn.unbounded_channel.html
• 3: https://docs.rs/tokio/latest/src/tokio/sync/mpsc/unbounded.rs.html
• 4: https://github.com/tokio-rs/tokio/blob/master/tokio/src/sync/mpsc/unbounded.rs
• 5: https://docs.rs/tokio/latest/tokio/sync/mpsc/index.html
• 6: Document allocation behaviour of mpsc and other channels tokio-rs/tokio#6754

---

Restore backpressure on the Arrow batch stream.

unbounded_channel() has infinite capacity and buffers messages arbitrarily in memory when the receiver falls behind. Combined with ignoring the send() result, this task can accumulate a whole result set in memory and keep fetching from Snowflake even after the consumer drops the receiver. Use a bounded channel and stop the producer when send().await fails.

Suggested fix

-use tokio_stream::wrappers::UnboundedReceiverStream;
+use tokio_stream::wrappers::ReceiverStream;
…
-        let (tx, rx) = tokio::sync::mpsc::unbounded_channel::<Result<RecordBatch>>();
+        let (tx, rx) = tokio::sync::mpsc::channel::<Result<RecordBatch>>(1);
…
-                        let _ = tx.send(Err(QueryFluxError::Engine(format!(
-                            "Snowflake query failed: {e}"
-                        ))));
+                        if tx
+                            .send(Err(QueryFluxError::Engine(format!(
+                                "Snowflake query failed: {e}"
+                            ))))
+                            .await
+                            .is_err()
+                        {
+                            return;
+                        }
                         return;
…
-                        let _ = tx.send(Ok(batch));
+                        if tx.send(Ok(batch)).await.is_err() {
+                            return;
+                        }
…
-                        let _ = tx.send(Err(e));
+                        if tx.send(Err(e)).await.is_err() {
+                            return;
+                        }
                         return;
…
-                let _ = tx.send(Ok(RecordBatch::new_empty(schema)));
+                let _ = tx.send(Ok(RecordBatch::new_empty(schema))).await;
             }
         });
 
-        Ok(Box::pin(UnboundedReceiverStream::new(rx)))
+        Ok(Box::pin(ReceiverStream::new(rx)))

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	let (tx, rx) = tokio::sync::mpsc::unbounded_channel::<Result<RecordBatch>>();
	tokio::spawn(async move {
	// Metadata is on the first response even when `rowSet` / chunks are empty (e.g. LIMIT 0).
	let col_types = executor.snowflake_columns();
	let fields: Vec<Field> = col_types
	.iter()
	.map(|c| {
	Field::new(
	c.name(),
	snowflake_type_to_arrow(c.column_type()),
	c.column_type().nullable(),
	)
	})
	.collect();
	let schema = Arc::new(ArrowSchema::new(fields));
	if schema.fields().is_empty() {
	return;
	}
	let mut emitted_rows = false;
	loop {
	let chunk = match executor.fetch_next_chunk().await {
	Ok(c) => c,
	Err(e) => {
	let _ = tx.send(Err(QueryFluxError::Engine(format!(
	"Snowflake query failed: {e}"
	))));
	return;
	}
	};
	let Some(rows) = chunk else { break };
	if rows.is_empty() {
	continue;
	}
	emitted_rows = true;
	match build_snowflake_record_batch(Arc::clone(&schema), &col_types, &rows) {
	Ok(batch) => {
	let _ = tx.send(Ok(batch));
	}
	Err(e) => {
	let _ = tx.send(Err(e));
	return;
	}
	}
	}
	if !emitted_rows {
	let _ = tx.send(Ok(RecordBatch::new_empty(schema)));
	}
	});
	Ok(Box::pin(UnboundedReceiverStream::new(rx)))
	let (tx, rx) = tokio::sync::mpsc::channel::<Result<RecordBatch>>(1);
	tokio::spawn(async move {
	// Metadata is on the first response even when `rowSet` / chunks are empty (e.g. LIMIT 0).
	let col_types = executor.snowflake_columns();
	let fields: Vec<Field> = col_types
	.iter()
	.map(|c| {
	Field::new(
	c.name(),
	snowflake_type_to_arrow(c.column_type()),
	c.column_type().nullable(),
	)
	})
	.collect();
	let schema = Arc::new(ArrowSchema::new(fields));
	if schema.fields().is_empty() {
	return;
	}
	let mut emitted_rows = false;
	loop {
	let chunk = match executor.fetch_next_chunk().await {
	Ok(c) => c,
	Err(e) => {
	if tx
	.send(Err(QueryFluxError::Engine(format!(
	"Snowflake query failed: {e}"
	))))
	.await
	.is_err()
	{
	return;
	}
	return;
	}
	};
	let Some(rows) = chunk else { break };
	if rows.is_empty() {
	continue;
	}
	emitted_rows = true;
	match build_snowflake_record_batch(Arc::clone(&schema), &col_types, &rows) {
	Ok(batch) => {
	if tx.send(Ok(batch)).await.is_err() {
	return;
	}
	}
	Err(e) => {
	if tx.send(Err(e)).await.is_err() {
	return;
	}
	return;
	}
	}
	}
	if !emitted_rows {
	let _ = tx.send(Ok(RecordBatch::new_empty(schema))).await;
	}
	});
	Ok(Box::pin(ReceiverStream::new(rx)))

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@crates/queryflux-engine-adapters/src/snowflake/mod.rs` around lines 313 -
366, Replace the unbounded mpsc channel with a bounded one to restore
backpressure: create a
tokio::sync::mpsc::channel::<Result<RecordBatch>>(capacity) instead of
unbounded_channel(), use the async tx.send(batch).await call and check its
Result; if send returns Err (receiver closed) stop the producer and return so
you stop calling executor.fetch_next_chunk() and building more batches. Update
the stream wrapper to use tokio_stream::wrappers::ReceiverStream (or
ReceiverStream::new) instead of UnboundedReceiverStream and return
Box::pin(ReceiverStream::new(rx)). Ensure to adjust all references to
tx.send(...) to await and handle send errors by breaking/returning, and keep
existing error handling around executor.fetch_next_chunk() and
build_snowflake_record_batch().

[coderabbitai]

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

# First, let's find the run_first_col implementation
rg "run_first_col" --type rust -A 10 crates/

Repository: lakeops-org/queryflux

Length of output: 6879

---

🌐 Web query:

Snowflake SHOW DATABASES output columns order created_on name

💡 Result:

The Snowflake SHOW DATABASES command returns output columns in the following order, starting with created_on followed by name: created_on, name, is_default, is_current, origin, owner, comment, options, retention_time, kind, owner_role_type, object_visibility, data_quality_monitoring_settings, and additional columns as applicable (e.g., dropped_on when using HISTORY). This is confirmed by the official example output in the Snowflake documentation, which displays these columns in sequence. With the TERSE option, only created_on, name, kind, database_name, schema_name are returned (with kind, database_name, schema_name as NULL). Results are ordered lexicographically by database name.

Citations:

• 1: https://docs.snowflake.com/en/sql-reference/sql/show-databases.html
• 2: https://docs.snowflake.com/en/sql-reference/sql/show-databases

---

🌐 Web query:

Snowflake SHOW SCHEMAS output columns order created_on name

💡 Result:

The Snowflake SHOW SCHEMAS command output columns, in order, begin with created_on followed immediately by name. This is confirmed by multiple partial output examples in the official documentation, such as: | created_on | name | is_default | ... and TERSE mode explicitly lists: created_on, name, kind, database_name, schema_name. Full non-TERSE output includes additional columns after name: is_default, is_current, database_name, owner, comment, options, retention_time, and more (e.g. owner_role_type, object_visibility, and recent additions like CLASSIFICATION_PROFILE_* per 2024_08 bundle). Rows are ordered lexicographically by database and schema name.

Citations:

• 1: https://docs.snowflake.com/en/sql-reference/sql/show-schemas
• 2: https://docs.snowflake.com/en/sql-reference/sql/show-schemas.html
• 3: https://docs.snowflake.com/en/release-notes/bcr-bundles/2024_08/bcr-1757
• 4: https://docs.snowflake.com/en/en/sql-reference/info-schema/schemata

---

🌐 Web query:

Snowflake SHOW TABLES output columns order

💡 Result:

The Snowflake SHOW TABLES command outputs columns in the following order: 1. created_on 2. name 3. database_name 4. schema_name 5. kind 6. comment 7. cluster_by 8. rows 9. bytes 10. owner 11. retention_time 12. dropped_on (only when HISTORY is specified) With SHOW TABLES TERSE, only: created_on, name, kind, database_name, schema_name. The results are ordered lexicographically by database_name, schema_name, and name. Column names in the output are lowercase and require double quotes when referenced (e.g., "created_on").

Citations:

• 1: https://docs.snowflake.com/en/sql-reference/sql/show-tables.html
• 2: https://docs.snowflake.com/en/sql-reference/sql/show-tables

---

Use run_second_col() or fetch index 1 instead of index 0 for object names.

Snowflake's SHOW DATABASES, SHOW SCHEMAS, and SHOW TABLES commands return created_on at column 0 and name at column 1. The current code uses run_first_col() which retrieves column 0, returning timestamps instead of object names. Update these three methods to read the name field at index 1.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@crates/queryflux-engine-adapters/src/snowflake/mod.rs` around lines 369 -
387, The methods list_catalogs, list_databases, and list_tables currently call
run_first_col (which returns column 0) but Snowflake's SHOW commands put the
object name in column 1; change each to use run_second_col (or otherwise fetch
index 1) instead of run_first_col so they return the name rather than
created_on, keeping the existing SQL/identifier escaping (catalog.replace(...)
and database.replace(...)) intact.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

cat -n crates/queryflux-engine-adapters/src/snowflake/mod.rs | sed -n '530,550p'

Repository: lakeops-org/queryflux

Length of output: 909

---

🏁 Script executed:

cat -n crates/queryflux-engine-adapters/src/snowflake/mod.rs | sed -n '575,595p'

Repository: lakeops-org/queryflux

Length of output: 953

---

🏁 Script executed:

rg "DataType::Decimal|snowflake_type_to_arrow" crates/queryflux-engine-adapters/src/snowflake/mod.rs -A 3 -B 3

Repository: lakeops-org/queryflux

Length of output: 633

---

🏁 Script executed:

rg "Decimal|decimal" crates/queryflux-engine-adapters/src/snowflake/ -i

Repository: lakeops-org/queryflux

Length of output: 47

---

🏁 Script executed:

rg "struct SnowflakeColumnType|impl SnowflakeColumnType" crates/queryflux-engine-adapters/src/snowflake/ -A 10

Repository: lakeops-org/queryflux

Length of output: 47

---

🏁 Script executed:

rg "precision|scale" crates/queryflux-engine-adapters/src/snowflake/mod.rs -B 2 -A 2

Repository: lakeops-org/queryflux

Length of output: 277

---

🏁 Script executed:

rg "SnowflakeColumnType" crates/queryflux-engine-adapters/ --type rust

Repository: lakeops-org/queryflux

Length of output: 414

---

🏁 Script executed:

head -50 crates/queryflux-engine-adapters/src/snowflake/mod.rs

Repository: lakeops-org/queryflux

Length of output: 1844

---

🏁 Script executed:

rg "use.*DataType|DataType::Decimal" crates/queryflux-engine-adapters/ --type rust

Repository: lakeops-org/queryflux

Length of output: 529

---

🏁 Script executed:

cat crates/queryflux-engine-adapters/Cargo.toml | grep -A 2 -B 2 "arrow\|snowflake"

Repository: lakeops-org/queryflux

Length of output: 329

---

🏁 Script executed:

cat -n crates/queryflux-engine-adapters/src/snowflake/mod.rs | sed -n '534,610p'

Repository: lakeops-org/queryflux

Length of output: 3202

---

🏁 Script executed:

cat Cargo.toml | grep "arrow"

Repository: lakeops-org/queryflux

Length of output: 205

---

🏁 Script executed:

rg "fixed|number|decimal" crates/queryflux-engine-adapters/src/ --type rust -i -l

Repository: lakeops-org/queryflux

Length of output: 365

---

🏁 Script executed:

cat -n crates/queryflux-engine-adapters/src/snowflake/mod.rs | sed -n '595,615p'

Repository: lakeops-org/queryflux

Length of output: 814

---

🏁 Script executed:

python3 << 'EOF'
import json

# Check Arrow 58.1.0 capabilities
# Arrow has supported Decimal types since early versions
print("Arrow 58.1.0 supports:")
print("- DataType::Decimal128(precision, scale)")
print("- DataType::Decimal256(precision, scale)")
print()
print("Snowflake NUMBER(p,s) can have p up to 38")
print("Int64 max value: 2^63-1 = 9223372036854775807 (~9.2e18, 19 digits)")
print("Int64 min value: -2^63 = -9223372036854775808")
print()
print("Example precision loss:")
print("- Snowflake FIXED(30,0) with value 123456789012345678901234567890")
print("  → Would fail row.at::<i64>() and become NULL")
print()
print("Current behavior: scale=0 → Int64 (can lose precision)")
print("Current behavior: scale>0 → Utf8 (preserves precision)")
EOF

Repository: lakeops-org/queryflux

Length of output: 574

---

🏁 Script executed:

grep -n "NUMERIC\|DECIMAL\|NUMBER" crates/queryflux-engine-adapters/src/trino/mod.rs -i -B 2 -A 2

Repository: lakeops-org/queryflux

Length of output: 637

---

🏁 Script executed:

grep -n "NUMERIC\|DECIMAL\|NUMBER" crates/queryflux-engine-adapters/src/duckdb/mod.rs -i -B 2 -A 2

Repository: lakeops-org/queryflux

Length of output: 47

---

🏁 Script executed:

rg "scale.*0|scale == 0" crates/queryflux-engine-adapters/src/snowflake/mod.rs -B 5 -A 5

Repository: lakeops-org/queryflux

Length of output: 479

---

🏁 Script executed:

find crates/queryflux-engine-adapters -name "*test*" -o -name "*spec*" | head -20

Repository: lakeops-org/queryflux

Length of output: 47

---

🏁 Script executed:

git log --oneline -n 50 -- crates/queryflux-engine-adapters/src/snowflake/mod.rs 2>/dev/null | head -20

Repository: lakeops-org/queryflux

Length of output: 101

---

🏁 Script executed:

git show 39dbbf1 --stat | head -50

Repository: lakeops-org/queryflux

Length of output: 2960

---

🏁 Script executed:

rg "precision()" crates/queryflux-engine-adapters/src/snowflake/ -A 2 -B 2

Repository: lakeops-org/queryflux

Length of output: 47

---

🏁 Script executed:

cat crates/queryflux-engine-adapters/src/snowflake/mod.rs | grep -n "\.precision\|\.scale" | head -20

Repository: lakeops-org/queryflux

Length of output: 117

---

🏁 Script executed:

rg "fn scale|fn precision" crates/queryflux-engine-adapters/src/snowflake/ --type rust -A 3

Repository: lakeops-org/queryflux

Length of output: 47

---

🏁 Script executed:

git log -p --all -S "scale == 0" -- crates/queryflux-engine-adapters/src/snowflake/mod.rs 2>/dev/null | head -80

Repository: lakeops-org/queryflux

Length of output: 47

---

🏁 Script executed:

cat -n crates/queryflux-engine-adapters/src/snowflake/mod.rs | sed -n '1,40p'

Repository: lakeops-org/queryflux

Length of output: 1703

---

Use Utf8 for all Snowflake FIXED columns, not just when scale > 0.

Snowflake NUMBER/DECIMAL supports up to 38 digits of precision, but mapping scale-0 values to DataType::Int64 silently loses data. Int64 maxes out at ~9.2×10¹⁸ (19 digits); any value exceeding that fails the row.at::<i64>() call (line 585) and gets converted to null (line 587). This is silent precision loss.

The current asymmetry—scale-0 → Int64, scale > 0 → Utf8—is inconsistent and unsafe. Either use Arrow Decimal128 or Decimal256 if snowflake_connector_rs supports decimal extraction, or map all FIXED to Utf8 for consistency (matching how Trino handles DECIMAL). The same applies at lines 582–590 where Int64 values are extracted.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@crates/queryflux-engine-adapters/src/snowflake/mod.rs` around lines 534 -
543, The Snowflake FIXED type mapping in snowflake_type_to_arrow is unsafe:
change the "fixed" arm to always return DataType::Utf8 (do not branch on scale)
to avoid silent overflow; then update any code paths that assume Int64 (the
places calling row.at::<i64>() and subsequent conversion that produced nulls) to
read the value as a string/Utf8 (e.g., row.at::<String>() or equivalent
extraction) or pass through the raw string value so large DECIMAL/NUMBER values
are preserved; alternatively, if the connector supports Decimal128/Decimal256
extraction, replace the Utf8 mapping with those Decimal Arrow types and adjust
the extraction code accordingly (refer to snowflake_type_to_arrow and the call
sites using row.at::<i64>() to locate all places to change).

[coderabbitai]

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

find /tmp -type f -name "format.rs" -path "*/snowflake/http/*"

Repository: lakeops-org/queryflux

Length of output: 47

---

🏁 Script executed:

git ls-files | grep -E "snowflake/http/format\.rs"

Repository: lakeops-org/queryflux

Length of output: 119

---

🏁 Script executed:

git ls-files | grep -E "queryflux-frontend.*format\.rs"

Repository: lakeops-org/queryflux

Length of output: 119

---

🏁 Script executed:

fd -t f "format.rs" crates/

Repository: lakeops-org/queryflux

Length of output: 119

---

🏁 Script executed:

git ls-files | head -30

Repository: lakeops-org/queryflux

Length of output: 917

---

🏁 Script executed:

git ls-files "*snowflake*" | head -30

Repository: lakeops-org/queryflux

Length of output: 1341

---

🏁 Script executed:

cat -n crates/queryflux-frontend/src/snowflake/http/format.rs | sed -n '1,80p'

Repository: lakeops-org/queryflux

Length of output: 3473

---

🌐 Web query:

Arrow DataType Decimal128 Decimal256 scale signed

💡 Result:

In Apache Arrow, Decimal128 and Decimal256 are fixed-point decimal types parameterized by precision (total significant digits) and scale (digits after the decimal point, which can be negative). Decimal128 (128-bit width): - Maximum precision: 38 digits - Maximum scale: typically up to the precision value (e.g., DECIMAL128_MAX_SCALE) - Constructed as DataType::Decimal128(precision, scale) or equivalent in language bindings like pyarrow.decimal128(precision, scale) Decimal256 (256-bit width): - Maximum precision: 76 digits - Maximum scale: typically up to the precision value (e.g., DECIMAL256_MAX_SCALE) - Constructed as DataType::Decimal256(precision, scale) or pyarrow.decimal256(precision, scale) Both types are signed, using two's complement representation of a scaled integer. For example, Decimal128(7, 3) represents values like 1234.567 (internally stored as 1234567) and supports negative values like -1234.567. There is no unsigned variant; decimals are always signed.

Citations:

• 1: https://arrow.apache.org/docs/python/generated/pyarrow.decimal128.html
• 2: https://arrow.apache.org/rust/arrow/array/types/struct.Decimal256Type.html
• 3: https://arrow.apache.org/docs/7.0/r/reference/data-type.html
• 4: https://arrow.apache.org/docs/r/reference/data-type.html
• 5: https://docs.rs/arrow/latest/arrow/datatypes/index.html
• 6: https://www.mintlify.com/apache/arrow/format/data-types
• 7: https://arrow.apache.org/docs/12.0/python/generated/pyarrow.decimal128.html

---

🏁 Script executed:

wc -l crates/queryflax-frontend/src/snowflake/http/format.rs

Repository: lakeops-org/queryflux

Length of output: 150

---

🏁 Script executed:

stat crates/queryflux-frontend/src/snowflake/http/format.rs 2>&1 || echo "File not found"

Repository: lakeops-org/queryflux

Length of output: 122

---

🏁 Script executed:

find . -name "format.rs" -type f 2>/dev/null

Repository: lakeops-org/queryflux

Length of output: 121

---

🏁 Script executed:

ls -la | head -20

Repository: lakeops-org/queryflux

Length of output: 1272

---

🏁 Script executed:

find . -path "*snowflake*" -name "*.rs" -type f 2>/dev/null | head -20

Repository: lakeops-org/queryflux

Length of output: 1126

---

🏁 Script executed:

git ls-files | wc -l

Repository: lakeops-org/queryflux

Length of output: 68

---

🏁 Script executed:

git ls-files "*queryflux*" 2>&1 | head -50

Repository: lakeops-org/queryflux

Length of output: 2213

---

🏁 Script executed:

tree -L 3 -d 2>/dev/null || find . -maxdepth 3 -type d | head -30

Repository: lakeops-org/queryflux

Length of output: 1638

---

🏁 Script executed:

pwd && ls -la

Repository: lakeops-org/queryflux

Length of output: 1812

---

🏁 Script executed:

git show HEAD:crates/queryflux-frontend/src/snowflake/http/format.rs 2>/dev/null | head -80

Repository: lakeops-org/queryflux

Length of output: 2913

---

🌐 Web query:

Apache Arrow Rust Decimal128 Decimal256 scale i8 i32

💡 Result:

In Apache Arrow's Rust implementation, the scale parameter for Decimal128 and Decimal256 types is represented as an i8 (8-bit signed integer). This is evident from the DecimalType trait implementations for Decimal128Type and Decimal256Type, where const MAX_SCALE: i8 = DECIMAL128_MAX_SCALE (or DECIMAL256_MAX_SCALE), and function signatures like TYPE_CONSTRUCTOR: fn(u8, i8) -> DataType and validate_decimal_precision(num: Native, precision: u8, scale: i8). There is no use of i32 for scale in these core Arrow types; i32 appears in Parquet's separate Decimal enum for logical types, but not in Arrow's columnar format types. For example, DECIMAL256_MAX_SCALE is defined as i8 = 76.

Citations:

• 1: https://arrow.apache.org/rust/arrow/datatypes/struct.Decimal128Type.html
• 2: https://arrow.apache.org/rust/arrow/array/types/struct.Decimal128Type.html
• 3: https://arrow.apache.org/rust/arrow_data/decimal/index.html
• 4: https://arrow.apache.org/rust/arrow/datatypes/struct.Decimal256Type.html
• 5: https://arrow.apache.org/rust/arrow/array/types/struct.Decimal256Type.html
• 6: https://docs.rs/arrow/latest/arrow/datatypes/trait.DecimalType.html
• 7: https://docs.rs/arrow-data/56.1.0/x86_64-apple-darwin/arrow_data/decimal/constant.DECIMAL256_MAX_SCALE.html
• 8: https://arrow.apache.org/rust/arrow/datatypes/trait.DecimalType.html

---

🏁 Script executed:

cd /repo 2>/dev/null; git show HEAD:crates/queryflux-frontend/src/snowflake/http/format.rs 2>/dev/null | head -80 || pwd

Repository: lakeops-org/queryflux

Length of output: 2913

---

🏁 Script executed:

rg "Decimal128|Decimal256" --type rust -A 3 -B 1 | head -100

Repository: lakeops-org/queryflux

Length of output: 2246

---

🏁 Script executed:

cat Cargo.toml 2>/dev/null | grep -A 5 arrow || cat crates/queryflux-frontend/Cargo.toml 2>/dev/null | grep -A 5 arrow || find . -maxdepth 2 -name "Cargo.toml" -exec cat {} \; | head -50

Repository: lakeops-org/queryflux

Length of output: 347

---

🏁 Script executed:

rg "struct SfTypeInfo" --type rust -A 10 -B 2

Repository: lakeops-org/queryflux

Length of output: 1079

---

🏁 Script executed:

rg "as u32.*scale|scale.*as u32" --type rust -B 3 -A 3

Repository: lakeops-org/queryflux

Length of output: 696

---

🏁 Script executed:

rg "precision.*u32|scale.*u32" --type rust -B 2 -A 2

Repository: lakeops-org/queryflux

Length of output: 1863

---

🏁 Script executed:

git status

Repository: lakeops-org/queryflux

Length of output: 288

---

🏁 Script executed:

rg "fn sf_type_info|sf_type_info\(\)" --type rust -A 20 -B 2

Repository: lakeops-org/queryflux

Length of output: 1976

---

🏁 Script executed:

rg "DataType::Decimal" --type rust -B 3 -A 5 | head -100

Repository: lakeops-org/queryflux

Length of output: 3997

---

🌐 Web query:

Arrow Rust arrow::datatypes::DataType::Decimal128 scale type signature

💡 Result:

The type signature for arrow::datatypes::DataType::Decimal128 is Decimal128(u8, i8), where the first parameter is precision: u8 and the second is scale: i8. This is a variant of the DataType enum. The u8 represents the precision (total number of digits), and i8 represents the scale (number of digits after the decimal point, which can be negative).

Citations:

• 1: https://arrow.apache.org/rust/arrow/datatypes/enum.DataType.html
• 2: https://docs.rs/arrow/latest/arrow/datatypes/enum.DataType.html
• 3: https://docs.rs/arrow/latest/arrow/datatypes/struct.Decimal128Type.html
• 4: https://arrow.apache.org/rust/arrow/datatypes/trait.DecimalType.html
• 5: https://arrow.apache.org/rust/src/arrow_array/types.rs.html

---

Reject or handle negative decimal scales explicitly.

Arrow's Decimal128 and Decimal256 types use signed scales (i8), which can be negative. Casting *s (i8) directly to u32 causes integer overflow for negative values—for example, -2i8 as u32 becomes 4294967294. This corrupted scale leaks into the Snowflake rowtype JSON metadata and field metadata, causing clients to receive incorrect type information.

At lines 50–53, validate the scale before casting, or store it as a signed type and convert explicitly during serialization.

Also applies to: 50-53

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@crates/queryflux-frontend/src/snowflake/http/format.rs` around lines 26 - 30,
SfTypeInfo currently stores precision/scale/char_length as u32 and casts Arrow's
signed scale (i8) into u32, which overflows for negative scales; update handling
by changing SfTypeInfo.scale to a signed type (e.g., i8 or i32) or validate the
incoming scale before casting, explicitly rejecting or handling negative scales
when building rowtype/field metadata; ensure serialization code that emits
Snowflake JSON converts the signed scale to the expected JSON numeric form (and
returns an error or maps negative scales appropriately) rather than performing a
blind as u32 cast so negative scales cannot corrupt metadata.

[coderabbitai]

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

🏁 Script executed:

# First, get an overview of the file and its line count
wc -l crates/queryflux-frontend/src/snowflake/http/format.rs

Repository: lakeops-org/queryflux

Length of output: 123

---

🏁 Script executed:

# Read the file sections mentioned in the review comment
# Lines 209-214, 224-226, 317-333, 352-370
head -n 380 crates/queryflux-frontend/src/snowflake/http/format.rs | tail -n 180 | cat -n

Repository: lakeops-org/queryflux

Length of output: 7978

---

🏁 Script executed:

# Also check the beginning to understand the function structure and success handling
head -n 250 crates/queryflux-frontend/src/snowflake/http/format.rs | cat -n

Repository: lakeops-org/queryflux

Length of output: 11270

---

🏁 Script executed:

# Read lines 250-370 to see the remaining conversions mentioned
sed -n '250,370p' crates/queryflux-frontend/src/snowflake/http/format.rs | cat -n

Repository: lakeops-org/queryflux

Length of output: 5105

---

🏁 Script executed:

# Read lines 317-372 to see the remaining code mentioned in the review
sed -n '317,372p' crates/queryflux-frontend/src/snowflake/http/format.rs | cat -n

Repository: lakeops-org/queryflux

Length of output: 2368

---

Do not return a success payload after a lossy Arrow conversion.

This code path has multiple mechanisms that silently fail without error signaling:

• Lines 209–214: Fabricates zero values on downcast failure in Time64 Microsecond conversion
• Lines 224–226: Returns unconverted original array on cast failure for UInt64
• Lines 317–321: Silently drops entire batches that fail RecordBatch::try_new validation
• Lines 326–327, 330–331: IPC write/finish errors are only logged via tracing, not returned to caller

Despite these failures, sf_query_response (line 368) unconditionally returns "success": true, allowing clients to receive truncated or corrupted rowsetBase64 without any error indication.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@crates/queryflux-frontend/src/snowflake/http/format.rs` around lines 209 -
214, The code currently masks multiple Arrow conversion and IPC errors and still
returns success in sf_query_response; fix by propagating and returning errors
instead of fabricating values or logging only. Specifically: in the Time64
microsecond conversion branch where you downcast
cast.as_any().downcast_ref::<Int64Array>() and currently use unwrap_or_else(||
Int64Array::from(vec![0i64; arr.len()])), return a proper Err on downcast/cast
failure; likewise for the UInt64 cast branch that returns the original uncast
array on failure, propagate the cast error. Ensure RecordBatch::try_new failures
are not silently dropped but returned to the caller, and convert tracing-only
IPC errors (write/finish) into returned errors so sf_query_response can set
"success": false and include the error. Update functions that call these symbols
(cast, Int64Array conversion, RecordBatch::try_new, IPC write/finish, and
sf_query_response) to bubble up Result types instead of swallowing failures.

[coderabbitai]

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

🏁 Script executed:

# First, let's look at the file and understand the context around lines 220-223
cat -n crates/queryflux-frontend/src/snowflake/http/format.rs | sed -n '210,235p'

Repository: lakeops-org/queryflux

Length of output: 1296

---

🏁 Script executed:

# Let's see the full function context to understand the purpose
cat -n crates/queryflux-frontend/src/snowflake/http/format.rs | sed -n '180,230p'

Repository: lakeops-org/queryflux

Length of output: 2556

---

🏁 Script executed:

# Let's search for how Time32 is used and what units it supports
rg "Time32" crates/queryflux-frontend/src/snowflake/http/format.rs -B 3 -A 3

Repository: lakeops-org/queryflux

Length of output: 764

---

🏁 Script executed:

# Search for documentation or comments about Time32 units in the codebase
rg "TimeUnit" crates/queryflux-frontend/src/snowflake/http/format.rs -B 2 -A 2

Repository: lakeops-org/queryflux

Length of output: 1342

---

🏁 Script executed:

# Let's check the Arrow documentation and see what Time32 units are
# First, let's look at the imports and how Time32 is structured
rg "Time32" --type rust -B 2 -A 2 | head -50

Repository: lakeops-org/queryflux

Length of output: 2101

---

🏁 Script executed:

# Let's search for tests or examples that might show Time32 unit information
rg "Time32\(" crates/ -B 2 -A 2 | head -50

Repository: lakeops-org/queryflux

Length of output: 2101

---

🌐 Web query:

Apache Arrow Time32 unit Second Millisecond definition

💡 Result:

Apache Arrow's Time32 is a 32-bit signed integer type representing the time of day (elapsed since midnight) with two possible units: Second ('s') and Millisecond ('ms'). Time32(Second): Number of seconds since midnight, range 0 to 86399 (i32). Time32(Millisecond): Number of milliseconds since midnight, range 0 to 86399999 (i32). This does not include timezone or date information and excludes leap seconds (max < 86400 seconds). Examples from docs: - Python: pa.time32('s') or pa.time32('ms') - Rust: Time32SecondType / Time32MillisecondType, native i32, "elapsed time since midnight in seconds/milliseconds".

Citations:

• 1: https://arrow.apache.org/rust/arrow_array/types/struct.Time32MillisecondType.html
• 2: https://arrow.apache.org/rust/arrow/array/types/struct.Time32MillisecondType.html
• 3: https://arrow.apache.org/docs/python/generated/pyarrow.Time32Type.html
• 4: https://docs.rs/arrow/latest/arrow/datatypes/struct.Time32MillisecondType.html
• 5: https://arrow.apache.org/docs/5.0/r/reference/data-type.html
• 6: https://arrow.apache.org/docs/python/generated/pyarrow.time32.html
• 7: https://arrow.apache.org/rust/arrow_ipc/gen/Schema/struct.Time.html?search=u32+-%3E+bool
• 8: https://arrow.apache.org/docs/20.0/python/generated/pyarrow.Time32Type.html

---

🏁 Script executed:

# Let's verify the schema metadata one more time to confirm scale: 9
cat -n crates/queryflux-frontend/src/snowflake/http/format.rs | sed -n '140,160p'

Repository: lakeops-org/queryflux

Length of output: 1296

---

🏁 Script executed:

# Let's find the sf_type_info function and confirm the scale for Time32
cat -n crates/queryflux-frontend/src/snowflake/http/format.rs | sed -n '70,120p'

Repository: lakeops-org/queryflux

Length of output: 1915

---

Scale Time32 values to nanoseconds before casting to Int64.

The schema metadata declares both Time32 and Time64 with scale 9 (nanoseconds). However, Time32 is cast directly without scaling:

• Time32(Second) values must be multiplied by 1_000_000_000
• Time32(Millisecond) values must be multiplied by 1_000_000

This matches the pattern already implemented for Time64(Microsecond) on line 212, which multiplies by 1000 before casting. Without this scaling, every Time32 value will be incorrect by orders of magnitude.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@crates/queryflux-frontend/src/snowflake/http/format.rs` around lines 220 -
223, The Time32 branch currently casts the array directly to Int64; instead
detect the Time32(unit) variant, cast the array to Int64, then scale by the
appropriate multiplier before returning (multiply by 1_000_000_000 for Second
and 1_000_000 for Millisecond). Use the same pattern as the Time64(Microsecond)
handling (reference the Time64(Microsecond) case) but for DataType::Time32:
after arrow::compute::cast(arr, &DataType::Int64) use arrow::compute::multiply
(or the equivalent scalar-multiply helper) with an Int64 scalar multiplier so
all Time32 values are converted to nanoseconds before returning.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Use Euclidean division for pre-epoch timestamps.

Rust's / and % round toward zero for signed integers, so -1ns currently becomes { epoch: 0, fraction: -1 }. The Snowflake split needs a non-negative fractional part.

🐛 Proposed fix

-                            let epoch = nanos / 1_000_000_000;
-                            let fraction = (nanos % 1_000_000_000) as i32;
+                            let epoch = nanos.div_euclid(1_000_000_000);
+                            let fraction = nanos.rem_euclid(1_000_000_000) as i32;

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@crates/queryflux-frontend/src/snowflake/http/format.rs` around lines 257 -
260, The split of a signed nanoseconds timestamp into epoch and fractional parts
currently uses normal division and remainder, producing negative fractions for
pre-epoch times; replace the operations on nanos with Euclidean division such
that epoch = nanos.div_euclid(1_000_000_000) and fraction =
nanos.rem_euclid(1_000_000_000) cast to i32 (preserving non-negative fractional
part) in the code that computes (Some(epoch), Some(fraction))—update the logic
in the function/block where nanos is read (the snippet calculating epoch and
fraction) to use div_euclid/rem_euclid and ensure the fraction is within i32
range before wrapping in Some(...).

[coderabbitai]

⚠️ Potential issue | 🟠 Major

This endpoint materializes the entire result set in memory.

SqlApiSink stores every row from every batch in rows and only serializes at the end. That throws away the O(1-batch) execution path from execute_to_sink, so a single large result can exhaust memory or stall the process. Please add partitioning/pagination here, or at minimum enforce a hard inline row/byte cap.

Also applies to: 56-67, 100-114

[coderabbitai]

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

cd crates/queryflask-frontend && find . -name "handlers.rs" -type f

Repository: lakeops-org/queryflux

Length of output: 141

---

🏁 Script executed:

find . -path "*snowflake/sql_api*" -type f -name "*.rs" | head -20

Repository: lakeops-org/queryflux

Length of output: 241

---

🏁 Script executed:

find . -name "handlers.rs" -type f

Repository: lakeops-org/queryflux

Length of output: 181

---

🏁 Script executed:

fd -t f "handlers.rs"

Repository: lakeops-org/queryflux

Length of output: 177

---

🏁 Script executed:

git ls-files | grep -E "snowflake.*sql_api.*handlers" | head -20

Repository: lakeops-org/queryflux

Length of output: 124

---

🏁 Script executed:

git ls-files | grep -E "snowflake" | head -30

Repository: lakeops-org/queryflux

Length of output: 1341

---

🏁 Script executed:

git ls-files | grep -E "sql_api" | head -20

Repository: lakeops-org/queryflux

Length of output: 235

---

🏁 Script executed:

git ls-files | head -50

Repository: lakeops-org/queryflux

Length of output: 1707

---

🏁 Script executed:

cat crates/queryflux-frontend/src/snowflake/sql_api/handlers.rs | head -220 | tail -50

Repository: lakeops-org/queryflux

Length of output: 1719

---

🏁 Script executed:

stat crates/queryflux-frontend/src/snowflake/sql_api/handlers.rs 2>&1 | head -5

Repository: lakeops-org/queryflux

Length of output: 107

---

🏁 Script executed:

ls -la crates/ 2>&1 | head -20

Repository: lakeops-org/queryflux

Length of output: 1056

---

🏁 Script executed:

git ls-files crates/ | grep -i sql_api

Repository: lakeops-org/queryflux

Length of output: 235

---

🏁 Script executed:

git ls-files | grep -E "handlers\.rs"

Repository: lakeops-org/queryflux

Length of output: 177

---

🏁 Script executed:

find . -type f -name "*.rs" | grep -i handler | head -20

Repository: lakeops-org/queryflux

Length of output: 507

---

🏁 Script executed:

find . -type d -name snowflake 2>/dev/null

Repository: lakeops-org/queryflux

Length of output: 155

---

🏁 Script executed:

ls -la crates/queryflux-frontend/ 2>&1

Repository: lakeops-org/queryflux

Length of output: 292

---

🏁 Script executed:

ls -la crates/ 2>&1

Repository: lakeops-org/queryflux

Length of output: 1056

---

🏁 Script executed:

git ls-files | head -100

Repository: lakeops-org/queryflux

Length of output: 4229

---

🏁 Script executed:

rg "parse_snowflake_json_body" -t rust

Repository: lakeops-org/queryflux

Length of output: 1072

---

🏁 Script executed:

rg "SessionContext::MySqlWire" -t rust

Repository: lakeops-org/queryflux

Length of output: 1889

---

🏁 Script executed:

rg "Honor the SQL API" -t rust

Repository: lakeops-org/queryflux

Length of output: 47

---

🏁 Script executed:

rg "sql_api" -t rust | head -30

Repository: lakeops-org/queryflux

Length of output: 2478

---

🏁 Script executed:

rg "snowflake" -t rust | head -30

Repository: lakeops-org/queryflux

Length of output: 2546

---

🏁 Script executed:

rg "enum SessionContext|struct SessionContext" -A 10

Repository: lakeops-org/queryflux

Length of output: 3063

---

🏁 Script executed:

rg "statement|warehouse|role" -t rust -w | grep -E "(request|body|json)" | head -20

Repository: lakeops-org/queryflux

Length of output: 470

---

🏁 Script executed:

rg "authenticate.*state.*headers" -A 20

Repository: lakeops-org/queryflux

Length of output: 1986

---

🏁 Script executed:

rg "parse_snowflake_json_body" -B 5 -A 30

Repository: lakeops-org/queryflux

Length of output: 22217

---

🏁 Script executed:

rg '"database"|"schema"|"warehouse"|"role"' -t rust | head -40

Repository: lakeops-org/queryflux

Length of output: 1791

---

🏁 Script executed:

rg "body_json\[" -t rust -A 2 -B 2

Repository: lakeops-org/queryflux

Length of output: 1084

---

🏁 Script executed:

rg "fn.*sql_api\|sql_api.*handler" -t rust -B 2 -A 10

Repository: lakeops-org/queryflux

Length of output: 47

---

🏁 Script executed:

rg "SessionContext" -t rust

Repository: lakeops-org/queryflux

Length of output: 13786

---

🏁 Script executed:

find . -type f -name "*.rs" | xargs grep -l "snowflake.*sql.*api\|sql.*api.*snowflake" | head -10

Repository: lakeops-org/queryflux

Length of output: 298

---

🏁 Script executed:

rg "\.as_str\(\).*filter" -B 5 -A 5

Repository: lakeops-org/queryflux

Length of output: 47

---

🏁 Script executed:

rg "let Some\(sql\)" -B 20 -A 20

Repository: lakeops-org/queryflux

Length of output: 3844

---

🏁 Script executed:

python3 << 'EOF'
import os
import subprocess

# Find all rust files
result = subprocess.run(['git', 'ls-files'], capture_output=True, text=True)
files = result.stdout.strip().split('\n')
rs_files = [f for f in files if f.endswith('.rs')]

# Look for files containing relevant keywords
keywords = ['SessionContext', 'parse_snowflake', 'sql_api']
relevant = set()

for f in rs_files:
    if any(kw.lower() in f.lower() for kw in keywords):
        relevant.add(f)

print("Files matching keywords:")
for f in sorted(relevant):
    print(f)

# Now search in all files for content
print("\n\nSearching for SessionContext definitions...")
for f in rs_files[:100]:  # Check first 100 files to limit output
    try:
        with open(f, 'r', encoding='utf-8', errors='ignore') as file:
            content = file.read()
            if 'SessionContext' in content and ('enum' in content or 'struct' in content):
                print(f"Found in: {f}")
    except:
        pass

EOF

Repository: lakeops-org/queryflux

Length of output: 2049

---

🏁 Script executed:

rg "SessionContext\s*{" -A 15 -t rust

Repository: lakeops-org/queryflux

Length of output: 164

---

🏁 Script executed:

rg "enum SessionContext" -A 20 -t rust

Repository: lakeops-org/queryflux

Length of output: 1674

---

🏁 Script executed:

git ls-files | grep -E "\.(ts|tsx|js|jsx)$" | head -30

Repository: lakeops-org/queryflux

Length of output: 1605

---

🏁 Script executed:

rg "return sql_api_error.*BAD_REQUEST.*390000" -B 30 -A 10

Repository: lakeops-org/queryflux

Length of output: 3884

---

🏁 Script executed:

git ls-files | grep queryflux

Repository: lakeops-org/queryflux

Length of output: 9090

---

🏁 Script executed:

git ls-files | grep -i frontend

Repository: lakeops-org/queryflux

Length of output: 2341

---

🏁 Script executed:

find . -name "Cargo.toml" | xargs grep -l queryflux

Repository: lakeops-org/queryflux

Length of output: 581

---

🏁 Script executed:

git ls-files "crates/*/Cargo.toml" | head -20

Repository: lakeops-org/queryflux

Length of output: 542

---

🏁 Script executed:

git ls-files "crates/" | cut -d/ -f2 | sort -u

Repository: lakeops-org/queryflux

Length of output: 308

---

🏁 Script executed:

git ls-files | grep "snowflake.*handlers"

Repository: lakeops-org/queryflux

Length of output: 440

---

🏁 Script executed:

git ls-files "crates/" --glob="*.rs" | grep -E "sql|snowflake" | head -30

Repository: lakeops-org/queryflux

Length of output: 2148

---

🏁 Script executed:

git ls-files | grep -i snowflake

Repository: lakeops-org/queryflux

Length of output: 1341

---

🏁 Script executed:

git ls-files "**/*.rs" | grep -E "(snowflake|sql_api|handler)" | sort

Repository: lakeops-org/queryflux

Length of output: 1143

---

🏁 Script executed:

find . -type f -name "*.rs" -exec grep -l "SessionContext::MySqlWire" {} \;

Repository: lakeops-org/queryflux

Length of output: 515

---

🏁 Script executed:

find . -type f -name "*.rs" | xargs grep -l "parse_snowflake_json_body\|SessionContext::MySqlWire" 2>/dev/null

Repository: lakeops-org/queryflux

Length of output: 581

---

🏁 Script executed:

rg "parse_snowflake_json_body" --type rust -l

Repository: lakeops-org/queryflux

Length of output: 316

---

🏁 Script executed:

rg "let Some\(sql\) = body_json\[\"statement\"\]" -B 10 -A 30

Repository: lakeops-org/queryflux

Length of output: 3885

---

🌐 Web query:

Snowflake SQL API database schema warehouse role statement request fields

💡 Result:

The Snowflake SQL API (REST API at /api/v2/statements) uses a JSON request body for POST requests to execute SQL statements. Key fields related to database schema, warehouse, and role are: - database: Optional string. Specifies the database (case-sensitive). Defaults to user's DEFAULT_NAMESPACE. - schema: Optional string. Specifies the schema (case-sensitive). Defaults to user's DEFAULT_NAMESPACE. - warehouse: Optional string. Specifies the warehouse (case-sensitive). Defaults to user's DEFAULT_WAREHOUSE. - role: Optional string. Specifies the role (case-sensitive). Defaults to user's DEFAULT_ROLE. Example request body: { "statement": "SELECT * FROM table", "database": "TESTDB", "schema": "TESTSCHEMA", "warehouse": "TESTWH", "role": "TESTROLE" } Other common fields: statement (required SQL), bindings (for parameters), timeout (seconds), statements_parameters. Note: Separate Snowflake REST APIs use headers like X-Snowflake-Role and X-Snowflake-Warehouse for context, but SQL API primarily uses body fields. Values must match case from SHOW commands.

Citations:

• 1: https://docs.snowflake.com/en/developer-guide/sql-api/submitting-requests.md
• 2: https://docs.snowflake.com/en/developer-guide/sql-api/reference
• 3: https://docs.snowflake.com/en/developer-guide/sql-api/reference.html
• 4: https://docs.snowflake.com/en/developer-guide/sql-api/submitting-requests
• 5: https://docs.snowflake.com/en/developer-guide/sql-api/reference.md

---

🏁 Script executed:

wc -l crates/queryflux-frontend/src/snowflake/sql_api/handlers.rs

Repository: lakeops-org/queryflux

Length of output: 128

---

🏁 Script executed:

cat -n crates/queryflux-frontend/src/snowflake/sql_api/handlers.rs | head -300

Repository: lakeops-org/queryflux

Length of output: 11111

---

🏁 Script executed:

cat -n crates/queryflux-core/src/session.rs | head -100

Repository: lakeops-org/queryflux

Length of output: 4826

---

🏁 Script executed:

rg "warehouse|role" crates/queryflux-frontend/src/snowflake/sql_api/ -A 2 -B 2

Repository: lakeops-org/queryflux

Length of output: 47

---

🏁 Script executed:

cat -n crates/queryflux-frontend/src/snowflake/http/handlers/query.rs | sed -n '1,150p'

Repository: lakeops-org/queryflux

Length of output: 5403

---

Extract and honor database, schema, warehouse, and role from Snowflake SQL API request body.

The handler currently ignores these Snowflake-documented request fields, causing clients to silently run against user defaults instead of the requested execution context. Extract these fields from body_json and pass them through the session context or directly to routing/adaptation.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@crates/queryflux-frontend/src/snowflake/sql_api/handlers.rs` around lines 179
- 206, The handler parses body_json but ignores Snowflake fields database,
schema, warehouse, and role; update the code after obtaining body_json and sql
to extract these keys (e.g., body_json["database"].as_str().filter(|s|
!s.trim().is_empty()) etc.) and apply them to the session context creation: set
SessionContext::MySqlWire.schema to the provided schema (if any) and propagate
database, warehouse, and role into the session (either as session_vars or
appropriate fields used by downstream routing/adaptation) so the execution uses
the requested context instead of user defaults; ensure values are cloned/owned
like sql and preserved when building session_ctx and when calling
routing/adaptation functions.