apache / apache/kyuubi

[FEATURE] Add a proper SQL read-only check for run_select_query

Open
#7,412 1 comment 0 reactions 0 assignees View on GitHub
kind:feature priority:major
Dominant language
Scala
Stars
2.4k
Forks
1k
PR merge metrics
No merged PRs in 30d

Description

### Code of Conduct

- [x] I agree to follow this project's [Code of Conduct](https://www.apache.org/foundation/policies/conduct)

### Search before asking

- [x] I have searched in the [issues](https://github.com/apache/kyuubi/issues?q=is%3Aissue) and found no similar issues.

### Describe the feature

The `run_select_query` tool introduced in #7400 uses `SqlReadOnlyChecker`, which whitelists only the first significant token of a SQL statement as the read-only guard. This is bypassed by two legitimate Spark/Hive patterns, as pointed out by @wForget in #7400:

```sql
-- Hive multi-insert (first token = FROM, whitelisted for the
-- `FROM t SELECT ...` read variant)
FROM t1
INSERT INTO t2
SELECT c1, c2;

-- Spark/Hive CTE attached to INSERT (first token = WITH, whitelisted
-- for SELECT CTEs)
WITH tmp AS (SELECT * FROM t1)
INSERT INTO t2 SELECT * FROM tmp;
```

Both execute writes despite passing the first-token check. This request tracks replacing the first-token heuristic with a real SQL read-only check so the tool's name matches its behavior for the configured dialects.

### Motivation

`SqlReadOnlyChecker` is a guardrail against LLM misrouting, not a security boundary — writes are routed to the separate `run_mutation_query` tool at the prompt layer. In practice the two bypass patterns above are thin in LLM training data and rarely produced unsolicited; the realistic exposure is user-supplied SQL pasted into the conversation. That said, the contract is off: a tool named "read-only" should actually be read-only for the configured dialects, and reviewers reasonably expect that guarantee.

### Describe the solution

Parse with Apache Calcite Babel (or others) + the matching `SqlDialect` (Spark, Hive, Trino, MySQL, SQLite) and reject if:

- the root statement is not a query / DQL node
- any CTE body contains a DML/DDL node
- the statement is a multi-statement block containing any DML/DDL node

### Additional context

- Parent umbrella: #7379 (Data Agent Engine).
- Follow-up from #7400 (PR 2a of the data-agent series).
- Calcite is already a transitive dependency in the wider Kyuubi tree via Spark/Hive, so the delta for the data-agent module is modest.
- `Connection.setReadOnly(true)` was considered and rejected during #7400 review — Spark and Hive Thrift JDBC drivers throw or silently ignore it.

### Are you willing to submit PR?

- [x] Yes. I would be willing to submit a PR with guidance from the Kyuubi community to improve.
- [ ] No. I cannot submit a PR at this time.

Contributor guide

Open the contributing guide

Research direction

Start by tracing the run_select_query tool and its existing SqlReadOnlyChecker, using #7400 and the two example statements as context. Replace the first-token heuristic with parsing for the configured Spark, Hive, Trino, MySQL, and SQLite dialects; done means queries with DML or DDL in the root, CTEs, or multi-statement blocks are rejected.

Written by the indexing model from the issue text.

Assessment

Tech stack
scala, spark, sql
Domain
backend-api-design, databases
Issue type
Feature
Difficulty
4/5
Estimated time
3-5 days
Activity status
Quiet
Clarity
Mostly clear
Newbie friendliness
48/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.