#3664 fix: SQLScript with LET/RETURN no longer throws QueryNotIdempotentException

[robfrank]

Summary

• Fixes SQLScript QueryNotIdempotentException #3664 — read-only SQLScript (LET+SELECT, RETURN) was rejected by the /query endpoint with QueryNotIdempotentException
• LetStatement.isIdempotent() now delegates to its sub-statement (so LET $x = (SELECT ...) is idempotent, LET $x = (INSERT ...) is not); expression-only LETs return true
• ReturnStatement.isIdempotent() now returns true unconditionally since RETURN only outputs data

Test plan

• queryScriptWithLetAndReturnIsIdempotent — read-only script succeeds via database.query("sqlscript", ...)
• queryScriptWithWriteStatementIsNotIdempotent — script with INSERT is still rejected
• All 26 SQLScriptTest tests pass
• All related tests pass (OperationTypeTest, ScriptExecutionTest, etc. — 112 total, 0 failures)

🤖 Generated with Claude Code

[gemini-code-assist]

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.

Changelog

• 3664-sqlscript-querynotidempotentexception.md
  • Added a new markdown document detailing the problem, root cause, and solution for the QueryNotIdempotentException.
  • Included a summary of the files changed and tests added.
• engine/src/main/java/com/arcadedb/query/sql/parser/LetStatement.java
  • Overrode the isIdempotent() method to implement conditional idempotency logic.
• engine/src/main/java/com/arcadedb/query/sql/parser/ReturnStatement.java
  • Overrode the isIdempotent() method to unconditionally return true.
• engine/src/test/java/com/arcadedb/query/sql/SQLScriptTest.java
  • Added queryScriptWithLetAndReturnIsIdempotent test case.
  • Added queryScriptWithWriteStatementIsNotIdempotent test case.
  • Imported QueryNotIdempotentException.

Activity

• The pull request was generated with Claude Code.

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature	Command	Description
Code Review	/gemini review	Performs a code review for the current pull request in its current state.
Pull Request Summary	/gemini summary	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help	/gemini help	Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Footnotes

1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

[mergify]

🧪 CI Insights

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

🟢 All jobs passed!

But CI Insights is watching 👀

[gemini-code-assist]

Code Review

This pull request effectively addresses an issue where read-only SQL scripts with LET and RETURN were incorrectly flagged as non-idempotent. The changes are concise and correct, with isIdempotent() now properly implemented for LetStatement and ReturnStatement. The addition of comprehensive regression tests is excellent, verifying both the fix and ensuring that write operations remain non-idempotent. The detailed markdown file explaining the change is also a great addition.

[gemini-code-assist]

For improved code clarity and to prevent potential maintenance issues, it's a good practice to use braces for all if statements, even when the body contains only a single line. This makes the code more robust against future modifications.

Suggested change

	if (statement != null)
	return statement.isIdempotent();
	if (statement != null) {
	return statement.isIdempotent();
	}

[claude]

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.

[claude]

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. ✅

[robfrank]

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.

[codacy-production]

Coverage summary from Codacy

See diff coverage on Codacy

Coverage variation	Diff coverage
✅ -9.49%	✅ ∅

Coverage variation details

	Coverable lines	Covered lines	Coverage
Common ancestor commit (cb5e508)	109689	81954	74.71%
Head commit (bd9fae5)	140633 (+30944)	91721 (+9767)	65.22% (-9.49%)

Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: <coverage of head commit> - <coverage of common ancestor commit>

Diff coverage details

	Coverable lines	Covered lines	Diff coverage
Pull request (#3666)	0	0	∅ (not applicable)

Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: <covered lines added or modified>/<coverable lines added or modified> * 100%

See your quality gate settings    Change summary preferences

[codecov]

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.

Additional details and impacted files

@@            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.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.