wangzhigang1999 commented on PR #7400: URL: https://github.com/apache/kyuubi/pull/7400#issuecomment-4285575295
Thanks @yaooqinn for triggering the Copilot review. Addressed the feedback in ab5b658 + 9245098: **Fixed** - `SqlExecutor`: switched to `ResultSetMetaData.getColumnLabel` so SQL `AS` aliases show up correctly in the markdown header the LLM sees. - `ToolRegistry`: replaced unbounded `newCachedThreadPool` with a bounded `ThreadPoolExecutor` (max 8, `SynchronousQueue`, reject-fast) and implemented `AutoCloseable` for clean shutdown. - `KyuubiConf`: added `checkValue(>= 1s)` on both data-agent timeout entries; units stay consistent with existing `timeConf` conventions. - MySQL test harness: tool-args JSON now built via `ObjectMapper` instead of manual string concat. **Kept as-is (by design)** - **`SqlReadOnlyChecker` first-token whitelist.** This is a guardrail against LLM misrouting, not a security boundary. Writes go through the separate `run_mutation_query` tool, and an LLM with DML intent calls that directly. The first-token check catches the realistic failure mode (LLM hallucinates a mutation into the read-only tool). A real SQL parser (e.g. Calcite Babel) would be stronger and worth considering as a follow-up — not urgent for 2a. - **`SqlExecutor` returns full result set; no client-side row cap.** Output-size control belongs in the context layer above the tool, not inside the tool itself. PR description updated to reflect this. - **`SystemPromptBuilderTest` uses `LocalDate.now()` directly.** The two `now()` calls sit sub-millisecond apart; the midnight-UTC race is theoretical. Not worth widening `SystemPromptBuilder`'s public API with a `Clock` seam just to close it. PTAL when you have time — thanks! cc @pan3793 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
