Add support for querying Iceberg from the CLI#325
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe catalogs CLI adds a DuckDB-backed ChangesCatalog querying
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant CLI
participant do_query
participant IcebergCatalog
participant DuckDB
CLI->>do_query: SQL and catalog arguments
do_query->>IcebergCatalog: Validate catalog and vend credentials
do_query->>DuckDB: Attach catalog and execute SQL
DuckDB-->>do_query: Query rows and columns
do_query-->>CLI: Render table and row count
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
@CodeRabbit please review |
|
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
crates/tower-cmd/src/catalogs.rs (2)
283-329: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated vend-credentials/attach/query pattern.
fetch_catalog_tablesandexecute_catalog_queryboth vend read credentials, callattach_statements, and runspawn_blocking(|| run_duckdb_query(...)), differing only in the spinner message and how failures are surfaced (returnErr(String)vs.output::die). Consider extracting the shared "vend + attach + run" step into a helper that returns aResult, letting each caller decide how to react to failure.Also applies to: 365-402
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/tower-cmd/src/catalogs.rs` around lines 283 - 329, Extract the shared vend-credentials, attach-statements, and spawn_blocking/run_duckdb_query flow from fetch_catalog_tables and execute_catalog_query into a helper returning Result. Keep each caller’s existing spinner message and failure behavior intact, with fetch_catalog_tables converting failures to Err(String) and execute_catalog_query retaining its output::die handling.
472-531: 🎯 Functional Correctness | 🔵 Trivial | 🏗️ Heavy liftComplex/nested DuckDB types fall back to Rust debug formatting in JSON output.
The catch-all
other => json!(format!("{:?}", other))means List/Struct/Map/Array columns (common in Iceberg tables with nested schemas) render as Rust debug strings (e.g.List([Int(1), Int(2)])) instead of proper JSON arrays/objects. This is a safe fallback (no crash), but degrades usability of--jsonoutput for realistic Iceberg data.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/tower-cmd/src/catalogs.rs` around lines 472 - 531, Update duckdb_value_to_json to recursively convert DuckDB List/Array values into JSON arrays and Struct/Map values into JSON objects, reusing duckdb_value_to_json for nested elements and fields. Preserve the existing scalar and temporal conversions, and retain the debug-string fallback only for unsupported value variants.Cargo.toml (1)
31-31: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPin with a tilde requirement to avoid silent DuckDB version drift.
duckdb-rsencodes the bundled DuckDB version in the crate's second version component, and upstream explicitly recommends a tilde requirement so patch releases ofduckdb-rsdon't silently pull in a different bundled DuckDB version. As written,"1.10504.0"is a caret requirement (^1.10504.0), which permitscargo updateto move to1.10600.0,1.20000.0, etc. — a different bundled DuckDB engine — which is exactly what the tilde requirement is meant to prevent.📌 Proposed fix
-duckdb = { version = "1.10504.0", features = ["bundled"] } +duckdb = { version = "~1.10504.0", features = ["bundled"] }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Cargo.toml` at line 31, Update the duckdb dependency requirement to use a tilde constraint instead of Cargo’s default caret constraint, while preserving version 1.10504.0 and the bundled feature so dependency updates cannot change the bundled DuckDB version.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/tower-cmd/src/catalogs.rs`:
- Around line 217-281: Update the JSON construction in the catalog-details flow
so an Err from fetch_catalog_tables is represented distinctly instead of being
converted to an empty array by unwrap_or_default. Preserve null for unsupported
catalog types and an array for successful listings, while encoding the listing
error in the JSON using the existing response shape or error convention.
- Around line 425-439: The attach_statements function embeds the OAuth token in
SQL that may be exposed through run_duckdb_query errors. Change this setup flow
to keep the token out of the generated batch, or ensure failures from CREATE OR
REPLACE SECRET and ATTACH redact it before output::die receives the error, while
preserving the existing catalog attachment behavior.
---
Nitpick comments:
In `@Cargo.toml`:
- Line 31: Update the duckdb dependency requirement to use a tilde constraint
instead of Cargo’s default caret constraint, while preserving version 1.10504.0
and the bundled feature so dependency updates cannot change the bundled DuckDB
version.
In `@crates/tower-cmd/src/catalogs.rs`:
- Around line 283-329: Extract the shared vend-credentials, attach-statements,
and spawn_blocking/run_duckdb_query flow from fetch_catalog_tables and
execute_catalog_query into a helper returning Result. Keep each caller’s
existing spinner message and failure behavior intact, with fetch_catalog_tables
converting failures to Err(String) and execute_catalog_query retaining its
output::die handling.
- Around line 472-531: Update duckdb_value_to_json to recursively convert DuckDB
List/Array values into JSON arrays and Struct/Map values into JSON objects,
reusing duckdb_value_to_json for nested elements and fields. Preserve the
existing scalar and temporal conversions, and retain the debug-string fallback
only for unsupported value variants.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8df08685-a65c-4eca-ba23-193a4fca7b62
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
Cargo.tomlcrates/tower-cmd/Cargo.tomlcrates/tower-cmd/src/catalogs.rscrates/tower-cmd/src/lib.rs
| let response = match api::describe_catalog(&config, name, &env).await { | ||
| Ok(response) => response, | ||
| Err(err) => output::tower_error_and_die(err, "Fetching catalog details failed"), | ||
| }; | ||
|
|
||
| let tables = if is_storage_catalog(&response.catalog.r#type) { | ||
| Some(fetch_catalog_tables(&config, name, &env).await) | ||
| } else { | ||
| None | ||
| }; | ||
|
|
||
| let mut human = catalog_details_text(&response); | ||
| human.push('\n'); | ||
| human.push_str(&header_line("Tables")); | ||
| match &tables { | ||
| None => { | ||
| human.push_str(&format!( | ||
| " Table listing is not supported for {} catalogs.\n", | ||
| response.catalog.r#type | ||
| )); | ||
| } | ||
| Some(Ok(result)) if result.rows.is_empty() => { | ||
| human.push_str(" No tables found.\n"); | ||
| } | ||
| Some(Ok(result)) => { | ||
| let headers = vec!["Schema".to_string(), "Table".to_string()]; | ||
| let data = result | ||
| .rows | ||
| .iter() | ||
| .map(|row| row.iter().map(json_value_to_cell).collect()) | ||
| .collect(); | ||
| human.push_str(&output::table_text(headers, data)); | ||
| } | ||
| Some(Err(err)) => { | ||
| human.push_str(&format!(" Unable to list tables: {}\n", err)); | ||
| } | ||
| } | ||
|
|
||
| // `tables` is null when the catalog type doesn't support listing, and an | ||
| // array (possibly empty) when it does. | ||
| let json_tables = match tables { | ||
| None => serde_json::Value::Null, | ||
| Some(result) => serde_json::Value::Array( | ||
| result | ||
| .map(|result| { | ||
| result | ||
| .rows | ||
| .iter() | ||
| .map(|row| { | ||
| serde_json::json!({ | ||
| "schema": row.first().cloned().unwrap_or(serde_json::Value::Null), | ||
| "table": row.get(1).cloned().unwrap_or(serde_json::Value::Null), | ||
| }) | ||
| }) | ||
| .collect() | ||
| }) | ||
| .unwrap_or_default(), | ||
| ), | ||
| }; | ||
| let json_data = serde_json::json!({ | ||
| "catalog": response.catalog, | ||
| "tables": json_tables, | ||
| }); | ||
| output::text(&human, &json_data); | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
JSON tables output can't distinguish "no tables" from "listing failed".
When fetch_catalog_tables returns Err, the human output prints "Unable to list tables: ...", but the JSON branch (result.map(...).unwrap_or_default()) silently discards the error and emits "tables": [] — identical to a catalog that genuinely has zero tables. A --json consumer has no way to detect the failure.
🐛 Proposed fix
- let json_tables = match tables {
- None => serde_json::Value::Null,
- Some(result) => serde_json::Value::Array(
- result
- .map(|result| {
- result
- .rows
- .iter()
- .map(|row| {
- serde_json::json!({
- "schema": row.first().cloned().unwrap_or(serde_json::Value::Null),
- "table": row.get(1).cloned().unwrap_or(serde_json::Value::Null),
- })
- })
- .collect()
- })
- .unwrap_or_default(),
- ),
- };
- let json_data = serde_json::json!({
- "catalog": response.catalog,
- "tables": json_tables,
- });
+ let (json_tables, tables_error) = match tables {
+ None => (serde_json::Value::Null, None),
+ Some(Ok(result)) => (
+ serde_json::Value::Array(
+ result
+ .rows
+ .iter()
+ .map(|row| {
+ serde_json::json!({
+ "schema": row.first().cloned().unwrap_or(serde_json::Value::Null),
+ "table": row.get(1).cloned().unwrap_or(serde_json::Value::Null),
+ })
+ })
+ .collect(),
+ ),
+ None,
+ ),
+ Some(Err(err)) => (serde_json::Value::Null, Some(err)),
+ };
+ let json_data = serde_json::json!({
+ "catalog": response.catalog,
+ "tables": json_tables,
+ "tables_error": tables_error,
+ });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/tower-cmd/src/catalogs.rs` around lines 217 - 281, Update the JSON
construction in the catalog-details flow so an Err from fetch_catalog_tables is
represented distinctly instead of being converted to an empty array by
unwrap_or_default. Preserve null for unsupported catalog types and an array for
successful listings, while encoding the listing error in the JSON using the
existing response shape or error convention.
| fn attach_statements(name: &str, credentials: &CatalogCredentials) -> String { | ||
| format!( | ||
| "INSTALL httpfs;\n\ | ||
| LOAD httpfs;\n\ | ||
| INSTALL iceberg;\n\ | ||
| LOAD iceberg;\n\ | ||
| SET s3_region='eu-central-1';\n\ | ||
| CREATE OR REPLACE SECRET tower_cat (TYPE iceberg, TOKEN {token});\n\ | ||
| ATTACH {warehouse} AS {name} (TYPE iceberg, READ_ONLY, SECRET tower_cat, ENDPOINT {uri}, DEFAULT_REGION 'eu-central-1');\n", | ||
| token = sql_string(&credentials.oauth_token), | ||
| warehouse = sql_string(&credentials.warehouse), | ||
| name = sql_ident(name), | ||
| uri = sql_string(&credentials.catalog_uri), | ||
| ) | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
# Confirm whether region is intended to be static or should be dynamic per-catalog:
rg -n "eu-central-1|s3_region|DEFAULT_REGION" --type=rust
fd duckdb.sql.tmplRepository: tower/tower-cli
Length of output: 153
🏁 Script executed:
set -euo pipefail
ast-grep outline crates/tower-cmd/src/catalogs.rs --view expanded
printf '\n--- attach_statements and nearby lines ---\n'
sed -n '360,470p' crates/tower-cmd/src/catalogs.rs
printf '\n--- execute_catalog_query / do_show error paths ---\n'
rg -n "execute_catalog_query|do_show|Unable to list tables|Query failed" crates/tower-cmd/src/catalogs.rs -n -A4 -B4
printf '\n--- CatalogCredentials definition / region fields ---\n'
rg -n "struct CatalogCredentials|catalog_uri|warehouse|oauth_token|region|DEFAULT_REGION|s3_region" crates -n -A3 -B3
printf '\n--- duckdb snippet tests ---\n'
rg -n "duckdb_snippet_attaches_catalog_with_secret|credentials --format duckdb|TOKEN" crates/tower-cmd/src -n -A8 -B8Repository: tower/tower-cli
Length of output: 50371
🌐 Web query:
DuckDB execute_batch error includes SQL text ATTACH credential leak docs
💡 Result:
In DuckDB, including credentials directly in an ATTACH connection string has historically led to security risks where sensitive information, such as passwords, could be exposed in error messages if the connection attempt fails [1][2][3][4]. This issue specifically occurs because the full connection string—containing the credentials—is sometimes included in the exception text or logs generated when the ATTACH operation encounters an IO or connection error [1][2][3]. To mitigate this risk, DuckDB documentation strongly advises against including credentials directly in connection strings [4]. Instead, you should use the DuckDB Secrets Manager [1][4]. By creating a managed secret, you can store credentials securely and reference the secret by name during the ATTACH operation, which prevents the sensitive information from appearing in logs or error messages [1]. Example of using the Secrets Manager to avoid credential exposure: CREATE SECRET my_secret ( TYPE mysql, HOST 'localhost', USER 'root', PASSWORD 'your_password'); ATTACH '' AS mydb (TYPE mysql, SECRET my_secret); This approach decouples the sensitive credentials from the connection string [1]. Efforts have been made by the DuckDB team to address these leaks in various extensions (such as the PostgreSQL extension) by sanitizing error messages to display only the attach path rather than the full Data Source Name (DSN) [2][3].
Citations:
- 1: Password leaks when using ATTACH and there's an IOException in the Python client duckdb/duckdb#21420
- 2: On connection failure logs are leaking secrets duckdb/duckdb-postgres#368
- 3: Database secrets are leaked in error messages duckdb/ducklake#553
- 4: https://raw.githubusercontent.com/duckdb/duckdb-web/refs/heads/main/docs/current/core_extensions/postgres/secrets.md
🌐 Web query:
Tower catalog credentials region metadata eu-central-1 duckdb.sql.tmpl
💡 Result:
The term duckdb.sql.tmpl is typically associated with templating workflows for SQL generation or configuration in projects using DuckDB, often seen in environments like dbt or custom automation scripts [1][2]. In the context of "Tower" (a platform for data lakes and Iceberg tables) and its catalog metadata management, the system handles credentials and region specifications as follows: 1. Tower Catalog Management: Tower environments use secrets and catalogs to connect to datasets [3]. When configuring a catalog in Tower, you must specify the URI, Credential, Scope, and Warehouse [3]. For AWS-specific services like S3 Tables, Tower automatically configures REST catalog endpoints, SigV4 signing, and AWS Glue integration when a region (e.g., eu-central-1) is provided [3]. 2. DuckDB Configuration: DuckDB manages AWS regions and credentials through its secrets_manager [4][5]. Configurations for specific regions (like eu-central-1) are defined in secrets objects using the CREATE SECRET syntax, often specifying TYPE s3, PROVIDER credential_chain, and the REGION parameter [6][5]. 3. Template Files (.sql.tmpl): These files are used for parameterized SQL queries. In a pipeline, a template might include placeholders for catalog names, table paths, or environment-specific metadata that are populated at runtime by the application (e.g., Tower or a dbt runner) to ensure correct connectivity and region selection [1][2]. If you are encountering a duckdb.sql.tmpl file, it is likely a template for constructing DuckDB ATTACH statements, where region and credential metadata are injected dynamically based on the target environment [3][6].
Citations:
- 1: Feat: support Template Strings, eg `.sql(t"SELECT {my_col} * 2 from {my_table} JOIN {other_table}")` duckdb/duckdb-python#370
- 2: https://github.com/jwills/dbt-duckdb/blob/master/README.md
- 3: https://docs.tower.dev/docs/concepts/environments
- 4: https://duckdb.org/docs/current/configuration/secrets_manager.html
- 5: https://raw.githubusercontent.com/duckdb/duckdb-web/refs/heads/main/docs/lts/core%5Fextensions/aws.md
- 6: https://docs.getdbt.com/reference/resource-configs/duckdb-configs
🌐 Web query:
DuckDB CREATE SECRET error echoes statement execute_batch token exposed
💡 Result:
The error message "token exposed" is not a standard, built-in DuckDB error code. Based on the context of DuckDB security and secrets management, this likely refers to a custom application-level check or a third-party security scanner flagging the presence of credentials (tokens) within logs or database statements. 1. Technical Context of execute_batch: In the Rust duckdb bindings, execute_batch is a convenience method used to run multiple semicolon-separated SQL statements [1][2]. Because it executes strings directly, any sensitive information—such as a token provided in a CREATE SECRET statement—is transmitted as plain text to the DuckDB engine [1]. If application code, middleware, or logging utilities echo these statements to logs, stdout, or error reports, the token can be inadvertently exposed [3]. 2. Potential Causes: - Logging: Application-level logging that captures the full SQL string passed to execute_batch will inevitably log any tokens contained within CREATE SECRET commands. - Debugging/Tracing: Tools that trace executed SQL statements for debugging often output the raw query text, which contains the secret. - Application Logic: If code constructs CREATE SECRET statements dynamically by concatenating raw token strings, developers must ensure these strings are not inadvertently printed or saved in insecure locations [3]. 3. Recommended Mitigation: - Use Parameterized Queries: While execute_batch does not support parameters [1], you should use prepared statements with placeholders (e.g., ?) whenever possible to separate data from the SQL command structure. - Avoid Logging Secrets: Ensure that logging middleware or error handlers sanitize SQL queries to scrub potential secret patterns (e.g., regex-based masking of token strings) before outputting them to logs. - Environment Variables: Instead of embedding tokens directly into SQL, use DuckDB's ability to pull secrets from environment variables. For example, if a secret is stored in an environment variable, you can reference it directly or configure DuckDB to read it, avoiding the need to hardcode the token in your SQL string. - Audit Sensitive Commands: If your application must handle secrets, treat logs containing CREATE SECRET as sensitive material and implement restricted access policies for these logs. If you are encountering this error from a security tool, it is likely flagging the presence of the secret in plain text within your source code or command history. The most secure approach is to move secret management out of application code and into environment-based configuration or a dedicated secret management service [4].
Citations:
- 1: https://context7.com/duckdb/duckdb-rs/llms.txt
- 2: https://docs.rs/duckdb/latest/src/duckdb/lib.rs.html
- 3: https://github.com/jojo0094/quacklake
- 4: https://github.com/lukaskostka99/mcp-server-motherduck
🏁 Script executed:
set -euo pipefail
printf '\n--- error display helpers ---\n'
rg -n "fn die|tower_error_and_die|redact|sanitize|mask" crates/tower-cmd/src -n -A4 -B4
printf '\n--- DuckDB failure handling in this file ---\n'
rg -n "execute_batch|Query failed|Unable to list tables|panicked" crates/tower-cmd/src/catalogs.rs -n -A3 -B3Repository: tower/tower-cli
Length of output: 17561
Avoid embedding the OAuth token in the DuckDB setup SQL. run_duckdb_query forwards duckdb::Error straight to output::die, so a failed CREATE OR REPLACE SECRET / ATTACH can echo the raw SQL and expose the token. Redact the failure or keep the secret out of the generated batch.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/tower-cmd/src/catalogs.rs` around lines 425 - 439, The
attach_statements function embeds the OAuth token in SQL that may be exposed
through run_duckdb_query errors. Change this setup flow to keep the token out of
the generated batch, or ensure failures from CREATE OR REPLACE SECRET and ATTACH
redact it before output::die receives the error, while preserving the existing
catalog attachment behavior.
| let mut human = catalog_details_text(&response); | ||
| human.push('\n'); | ||
| human.push_str(&header_line("Tables")); | ||
| match &tables { | ||
| None => { | ||
| human.push_str(&format!( | ||
| " Table listing is not supported for {} catalogs.\n", | ||
| response.catalog.r#type | ||
| )); | ||
| } | ||
| Some(Ok(result)) if result.rows.is_empty() => { | ||
| human.push_str(" No tables found.\n"); | ||
| } | ||
| Some(Ok(result)) => { | ||
| let headers = vec!["Schema".to_string(), "Table".to_string()]; | ||
| let data = result | ||
| .rows | ||
| .iter() | ||
| .map(|row| row.iter().map(json_value_to_cell).collect()) | ||
| .collect(); | ||
| human.push_str(&output::table_text(headers, data)); | ||
| } | ||
| Some(Err(err)) => { | ||
| human.push_str(&format!(" Unable to list tables: {}\n", err)); | ||
| } | ||
| } | ||
|
|
||
| // `tables` is an array (possibly empty) on success and null otherwise; | ||
| // `tables_error` distinguishes a failed listing (message) from a catalog | ||
| // type that doesn't support listing (null). | ||
| let (json_tables, tables_error) = match tables { | ||
| None => (serde_json::Value::Null, None), | ||
| Some(Err(err)) => (serde_json::Value::Null, Some(err)), | ||
| Some(Ok(result)) => ( | ||
| serde_json::Value::Array( | ||
| result | ||
| .rows | ||
| .iter() | ||
| .map(|row| { | ||
| serde_json::json!({ | ||
| "schema": row.first().cloned().unwrap_or(serde_json::Value::Null), | ||
| "table": row.get(1).cloned().unwrap_or(serde_json::Value::Null), | ||
| }) | ||
| }) | ||
| .collect(), | ||
| ), | ||
| None, | ||
| ), | ||
| }; | ||
| let json_data = serde_json::json!({ | ||
| "catalog": response.catalog, | ||
| "tables": json_tables, | ||
| "tables_error": tables_error, | ||
| }); | ||
| output::text(&human, &json_data); |
There was a problem hiding this comment.
could all this be replaced with out.table()?
There was a problem hiding this comment.
Bottom part is specifically for JSON data
bradhe
force-pushed
the
features/query-iceberg-from-cli
branch
from
b6bf62f to
1302ad5
Compare
Escaping now lives in SqlLiteral/SqlIdent Display wrappers instead of free functions each call site must remember, the table listing binds the catalog name as a real prepared-statement parameter, and attach_statements takes the vended credential Mode so call sites no longer pass an invertible read_only bool.
List/Array/Struct/Map/Union/Enum columns now render as proper JSON arrays and objects in --json output instead of Rust debug strings. The duckdb requirement uses a tilde constraint per upstream guidance, since the crate's second version component encodes the bundled DuckDB engine and a caret requirement would let cargo update swap engines silently.
4 participants
Why would you want to do this you ask? I don't want to do it. I want Claude to do it. The best way to get him to do it is with a CLI that can talk to our Iceberg catalogs. Isn't that cool? Pretty powerful stuff.
Note that the query must be read-only. It'll fail otherwise.
Example
Summary by CodeRabbit
New Features
catalogs querycommand for running SQL against storage catalogs.catalogs showto include available tables and structured JSON output.Bug Fixes