[FEATURE] Add a proper SQL read-only check for run_select_query
- 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
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