Summary of Changes

Hello, I'm Gemini Code Assist¹! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request addresses an issue where read-only SQLScripts, specifically those using LET with SELECT sub-queries and RETURN statements, were incorrectly flagged as non-idempotent by the /query API endpoint. The changes ensure that the system accurately identifies the idempotent nature of these statements, allowing legitimate read-only operations to proceed without error, while still correctly rejecting scripts that perform write operations.

Highlights

• Idempotency fix for SQLScript: Resolved QueryNotIdempotentException for read-only SQLScripts containing LET and RETURN statements when executed via the /query endpoint.
• LetStatement idempotency logic: Modified LetStatement.isIdempotent() to correctly assess idempotency by delegating to its sub-statement or returning true for expression-only LET clauses.
• ReturnStatement idempotency logic: Updated ReturnStatement.isIdempotent() to always return true, as RETURN statements are inherently read-only.
• Regression tests: Added two new regression tests to SQLScriptTest.java to verify the fix and ensure write statements are still correctly rejected.

🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console.

🧪 CI Insights

Here's what we observed from your CI run for bd9fae5.

🟢 All jobs passed!

But CI Insights is watching 👀

Code Review

The fix correctly addresses the root cause: both LetStatement and ReturnStatement inherited Statement.isIdempotent() which unconditionally returns false, causing any script with these statements to be rejected by the /query endpoint regardless of actual content.

✅ What looks good

• LetStatement.isIdempotent() — delegating to statement.isIdempotent() when a sub-statement is present is the right approach, consistent with IfStatement.isIdempotent() which does the same for its child statements.
• ReturnStatement.isIdempotent() — returning true unconditionally is correct; RETURN only reads/outputs already-computed values.
• Logic is minimal and focused — no over-engineering.
• Two regression tests that cover both the happy path and the rejected case.

Issues to address

1. Stray documentation file in repo root

3664-sqlscript-querynotidempotentexception.md is committed to the repository root. This appears to be a Claude Code work-plan document (similar to the ones in docs/plans/). It shouldn't live in the repo root. Please either:

• Move it to docs/plans/ to be consistent with other design/fix notes, or
• Remove it entirely (the PR description already captures the explanation).

2. Test uses fully-qualified Assertions.assertThrows instead of AssertJ

queryScriptWithWriteStatementIsNotIdempotent uses org.junit.jupiter.api.Assertions.assertThrows(...) with a fully-qualified class name. The codebase standard (and the pattern used elsewhere in the SQL test package) is AssertJ's assertThatThrownBy. For example, in SelectStatementExecutionTest.java:

assertThatThrownBy(() -> database.query("sqlscript", script))
    .isInstanceOf(QueryNotIdempotentException.class);

This is both more readable and consistent with the rest of the codebase. The full-qualified form also bypasses the import section and is harder to scan.

3. Minor: expression idempotency caveat (FYI, not a blocker)

The comment // expressions (literals, variable references, method calls) do not write data is accurate for built-in expressions. However, user-defined SQL functions (FunctionCall) could theoretically execute DML. This is the same assumption IfStatement makes (it checks sub-statements but not condition expressions), so it's consistent with the existing design — just worth a note in case UDF side-effects are ever revisited.

Summary

The core logic is correct and the approach is sound. The two things that should be fixed before merging are: remove/relocate the stray .md file, and update the test to use assertThatThrownBy for style consistency.

Code Review

This is a clean, focused fix for issue #3664. The approach is correct and well-scoped.

What's Good

• LetStatement.isIdempotent() correctly delegates to its sub-statement when one exists, and returns true for expression-only LETs. This matches the same pattern used in IfStatement (which iterates over all its child statements). The logic is sound.
• ReturnStatement.isIdempotent() returning true unconditionally is correct — RETURN only reads/outputs context variables, it never writes data.
• Tests cover both the happy path and the guard: the write-rejection test ensures the fix doesn't accidentally open the door to non-idempotent scripts via the query endpoint.
• Tests follow the project style (assertThat(...), assertThatThrownBy(...), database.transaction(...) for setup).

Minor Observations

1. Expression-based LET and polyglot/UDF functions — LET $x = someFunction(...) where someFunction is a user-defined function or a polyglot script could in theory perform writes. The current implementation always returns true for expression-only LETs, which is consistent with how the rest of the engine treats expressions (there is no isIdempotent() on Expression). This is a pre-existing limitation of the idempotency model, not introduced by this PR, and is acceptable given the overall design.

2. Test setup uses database.transaction() inside a TestHelper-based test, which is the correct pattern. One very minor nit: the queryScriptWithLetAndReturnIsIdempotent test creates a Chunk type and inserts a row without cleanup — if the test class reuses a database across tests this could cause interference. Looking at the class, other tests in SQLScriptTest follow the same pattern without cleanup, so this is consistent with the existing convention.

3. Javadoc comment on the test references the GitHub issue URL inline — this is fine and helps traceability, consistent with the project style.

Verdict

The fix is minimal, correct, and well-tested. No issues found that would block merging. ✅

Code review feedback addressed (bd9fae5)

Applied:

• Removed 3664-sqlscript-querynotidempotentexception.md from repo root — the PR description already captures the explanation.
• Switched queryScriptWithWriteStatementIsNotIdempotent from org.junit.jupiter.api.Assertions.assertThrows to AssertJ's assertThatThrownBy for codebase consistency.

Rejected:

• Gemini's suggestion to add braces around the single-line if in LetStatement.isIdempotent() — the project's CLAUDE.md coding style explicitly states: "if statements with only one child sub-statement don't require a curly brace open/close, keep it simple". The current form is consistent with the rest of the codebase.

Coverage summary from Codacy

See diff coverage on Codacy

See your quality gate settings    Change summary preferences

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 65.76%. Comparing base (cb5e508) to head (bd9fae5).
⚠️ Report is 1 commits behind head on main.

@@            Coverage Diff             @@
##             main    #3666      +/-   ##
==========================================
- Coverage   65.86%   65.76%   -0.10%     
==========================================
  Files        1550     1550              
  Lines      109689   109689              
  Branches    22875    22875              
==========================================
- Hits        72245    72141     -104     
- Misses      27762    27822      +60     
- Partials     9682     9726      +44     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.