Code Review

This is a solid, well-scoped cleanup that resolves the concrete bug from #3738 (parser tests silently testing the wrong parser). The core migration is correct and the grammar/AST fixes are legitimate gap-fills. A few items worth discussing:

Issues to address before merging

1. Overly broad exception catch in AbstractProperty.getDefaultValue()

// Before (precise)
} catch (ParseException e) {
  // IGNORE IT

// After (too broad)
} catch (Exception e) {
  // IGNORE IT

catch (Exception e) swallows everything including NullPointerException, ClassCastException, etc. — bugs that should be visible. Consider catch (CommandSQLParsingException e) (what ANTLR throws) instead. If you want to also guard against unexpected runtime errors, at minimum log at DEBUG level.

2. UpdatePutItem access widening: protected → public

-  protected Identifier left;
-  protected Expression key;
-  protected Expression value;
+  public Identifier left;
+  public Expression key;
+  public Expression value;

Widening access modifiers is a red flag — it enlarges the public API surface. If the ANTLR AST builder needs to set these, a package-private accessor or a constructor that accepts the fields would be preferable. What's the motivation for making them public?

3. Duplicate createIndexBody grammar alternatives

| identifier (IF NOT EXISTS)? indexType
  (METADATA json)?
  (ENGINE identifier)?
| identifier (IF NOT EXISTS)? indexType
  (ENGINE identifier)?
  (METADATA json)?

These two alternatives exist only to allow ENGINE and METADATA in either order. ANTLR supports optional clauses in any combinator — consider something like:

| identifier (IF NOT EXISTS)? indexType
  ( (METADATA json) | (ENGINE identifier) )*

The duplicate alternatives will cause ambiguity warnings and are harder to maintain.

4. SQL_PARSER_IMPLEMENTATION silently ignored at runtime

Users who have arcadedb.sql.parserImplementation=javacc in their config will get no feedback that the setting has no effect. A one-time LogManager.instance().log(...) warning when a non-default value is detected (in GlobalConfiguration or StatementCache) would help operators discover the dead setting.

Minor observations

5. Plan files committed to docs/plans/

docs/plans/2026-03-28-seal-javacc-escape-hatches.md (624 lines) is an internal implementation plan — it even contains a local machine path (/Users/frank/projects/arcade/worktrees/...). The design document (-design.md) is fine as a record of the architectural decision, but the task-by-task execution plan is more of a working note than repository documentation worth preserving.

6. Unused import added to SQLASTBuilder.java

+import org.antlr.v4.runtime.ParserRuleContext;

This import was added but ParserRuleContext does not appear to be used in the shown diff. Confirm it's actually referenced, or remove it.

What's good

• The InCondition.toString() fix (not ? " NOT IN " : " IN ") is a genuine serialization bug — good catch.
• The checkRightSyntax roundtrip (parse → toString() → re-parse) in AbstractParserTest is a smart regression guard, and the SubqueryExpression/StatementExpression toString() overrides are necessary to make it work.
• Restricting matchFilterItemKey to an explicit keyword list (removing the open identifier fallback) is the right call — the old rule accepted any identifier as a filter key, hiding typos silently.
• The commitStatement grammar fix properly handles the ELSE-branch structure.
• Removing throwParsingException(TokenMgrError, ...) is correct — TokenMgrError is JavaCC-specific dead code now.
• SQLScriptQueryEngine.parseScript() simplification is clean.

Overall this is good work. The catch (Exception e) in AbstractProperty and the UpdatePutItem access widening are the two items I'd want resolved before merging.

🤖 Generated with Claude Code

Coverage summary from Codacy

See diff coverage on Codacy

See your quality gate settings    Change summary preferences

PR Review: Seal JavaCC SQL Parser Escape Hatches

Good overall goal — removing the JavaCC escape hatches is an important correctness fix after #3738. The core migration in StatementCache, SQLScriptQueryEngine, AbstractProperty, and SQLFunctionEval is clean and correct. The grammar bug-fixes bundled here are a nice bonus.

That said, I have several concerns worth addressing before merge.

🔴 Potential Breaking Change: matchProperties now required on --> / <--

File: SQLParser.g4, lines ~248–264

- | DECR GT matchProperties?    // -->{as:x} anonymous outgoing
+ | DECR GT matchProperties     // -->{as:x} anonymous outgoing (properties required)
- | ARROW_LEFT MINUS matchProperties?  // <--{as:x}
+ | ARROW_LEFT MINUS matchProperties  // <--{as:x}

The grammar for matchMethod and nestedMatchMethod now requires a matchProperties block after --> and <--. Since matchNode and matchProperties have the same structure (LBRACE matchFilter? RBRACE), this creates a risk: ANTLR4 will now greedily consume the next {...} block as the edge's matchProperties rather than the next node's matchNode.

For example, MATCH {as:a}-->{as:b} — previously ANTLR resolved {as:b} as the next matchNode; now it may be consumed as matchProperties, leaving matchPath without the required trailing matchNode. This is a potential regression for all MATCH queries using bare -->.

Please add an explicit test covering MATCH {as:a}-->{as:b} and MATCH {as:a}<--{as:b} to the MatchStatementTest. If these are indeed broken, the optional ? should be restored.

🔴 fromIdentifier alias removed silently

File: SQLParser.g4, line ~1036; SQLASTBuilder.java

-    | identifier (modifier)* (AS? identifier)?    # fromIdentifier
+    | identifier (modifier)*                       # fromIdentifier

- // Handle alias (second identifier after AS)
- if (ctx.identifier().size() > 1) {
-   fromItem.alias = (Identifier) visit(ctx.identifier(1));
- }

This removes alias support from the bare-identifier form of FROM. Was this intentional? Queries like SELECT FROM Person AS p WHERE p.name = 'x' could be affected. If the alias in fromSubquery (the LPAREN statement RPAREN ... (AS? identifier)? alternative) covers the needed cases, please add a comment explaining why the fromIdentifier alias is intentionally dropped — and a regression test.

🟡 UpdatePutItem visibility widened to public without justification

File: UpdatePutItem.java

-  protected Identifier left;
-  protected Expression key;
-  protected Expression value;
+  public Identifier left;
+  public Expression key;
+  public Expression value;

Changing protected → public widens the API surface unnecessarily. The visitUpdatePutItem handler in SQLASTBuilder could use setters or just set the fields directly (it's in the same package hierarchy). Please either revert to protected or add proper setters like the pattern used for UpdateIncrementItem.

🟡 AbstractProperty.getDefaultValue() swallows all exceptions

File: AbstractProperty.java

- } catch (ParseException e) {
+ } catch (Exception e) {
    // IGNORE IT

Catching bare Exception silently suppresses NullPointerException, ClassCastException, OutOfMemoryError (via RuntimeException), etc. This can hide real bugs. Please narrow to CommandSQLParsingException (what ANTLR throws) or at worst RuntimeException.

🟡 UNWIND AS-alias handling dropped from visitor

File: SQLASTBuilder.java (visitUnwind)

The grammar retains UNWIND expression (AS? identifier)? but the new visitor completely ignores ctx.identifier() — any AS alias is silently discarded. Either:

• Remove the AS? identifier alternative from the grammar (if UNWIND aliases aren't supported), or
• Restore the alias-handling code in the visitor

As-is, this is a quiet behaviour difference between old and new parser.

🟡 commitStatement grammar now requires AND before FAIL/CONTINUE

File: SQLParser.g4

- : COMMIT (RETRY INTEGER_LITERAL (ELSE (LBRACE (scriptStatement SEMICOLON?)* RBRACE)? (AND? (FAIL | CONTINUE))?)?)?
+ : COMMIT (RETRY INTEGER_LITERAL (ELSE (LBRACE (scriptStatement SEMICOLON?)+ RBRACE AND (FAIL | CONTINUE) | (FAIL | CONTINUE)))?)?

AND is now mandatory between the brace block and FAIL/CONTINUE (previously AND?). Also * → + means the brace block must be non-empty. Please confirm that COMMIT RETRY 3 ELSE FAIL and COMMIT RETRY 3 ELSE { stmt1; } FAIL (without AND) are still tested and pass.

🟢 Minor: Plan documents in docs/plans/ contain machine-local paths

Files: docs/plans/2026-03-28-seal-javacc-escape-hatches.md, docs/plans/2026-03-28-seal-javacc-escape-hatches-design.md

These AI-generated planning documents reference an absolute local path (/Users/frank/projects/arcade/worktrees/worktrees/seal-javacc) and are not useful reference documentation for other contributors. They're fine as ephemeral design notes but probably don't belong committed to main. Consider removing them or moving to a CONTRIBUTING-style internal wiki.

✅ What looks good

• StatementCache.parse() and SQLScriptQueryEngine.parseScript() simplifications are clean
• SQLFunctionEval.execute() migration is correct — removing the ByteArrayInputStream wrapper is a nice cleanup
• GlobalConfiguration.SQL_PARSER_IMPLEMENTATION deprecation is handled correctly (no more validation set, description updated)
• InCondition.toString() — correctly emits NOT IN when not == true
• matchFilterItemKey restricted to the documented valid keys (removes the open-ended identifier fallback that hid the parser divergence)
• StatementExpression and SubqueryExpression now implement toString() — good fix
• visitInputParameter now handles :from (reserved keyword as named param) correctly
• NearOperator/WithinOperator added to mapComparisonOperator — good fix

Codecov Report

❌ Patch coverage is 69.42675% with 48 lines in your changes missing coverage. Please review.
✅ Project coverage is 64.84%. Comparing base (9cd49e9) to head (84a1bc9).
⚠️ Report is 28 commits behind head on main.

@@            Coverage Diff             @@
##             main    #3756      +/-   ##
==========================================
+ Coverage   64.09%   64.84%   +0.75%     
==========================================
  Files        1580     1580              
  Lines      115475   115571      +96     
  Branches    24445    24472      +27     
==========================================
+ Hits        74010    74946     +936     
+ Misses      31348    30464     -884     
- Partials    10117    10161      +44     

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

Code Review (supplemental)

Good cleanup overall — the core goal of sealing JavaCC escape hatches is achieved cleanly. These are additional items not covered in the prior review.

Issues to address before merging

1. truncateRecordBody grammar change is a breaking API change

The comma-separated form TRUNCATE RECORD #1:1, #2:2 is gone — replaced by TRUNCATE RECORD [#1:1, #2:2]. This is a backwards-incompatible change with no mention in the PR description. Either restore the old form as a parallel alternative, or document this as a deliberate breaking change.

2. matchMethod/nestedMatchMethod: making --> and <-- require matchProperties breaks existing MATCH queries

- | DECR GT matchProperties?   // -->{as:x} anonymous outgoing
+ | DECR GT matchProperties    // -->{as:x} anonymous outgoing (properties required)

Any MATCH query using bare --> or <-- without {...} will now fail to parse. For example:

MATCH {class:Person, as:a} --> {class:Address, as:b} RETURN a, b

The --> here has no {...} block of its own — the {class:Address, as:b} is the destination node's properties, not the edge's. The bidirectional -- form kept ?, making the asymmetry surprising. If there is a parser ambiguity reason for this change, it needs a comment and a regression test. If unintentional, revert to matchProperties?.

3. PARALLEL keyword added to grammar but not handled in the AST builder or SelectStatement.toString()

SQLParser.g4 now accepts PARALLEL at the end of selectStatement and a new PARALLEL token is in the lexer. But there is no corresponding visitor in SQLASTBuilder.java and SelectStatement.toString() does not emit PARALLEL. The round-trip check in AbstractParserTest.checkRightSyntax() (parse -> toString -> re-parse) will silently drop PARALLEL from any query. Either wire it through to the AST and add a round-trip test, or remove it from this PR.

4. GlobalConfiguration.SQL_PARSER_IMPLEMENTATION is missing the Java @Deprecated annotation

The description text says "Deprecated" but there is no @Deprecated annotation on the enum constant. Without it, IDEs and tools will not flag usages at compile time. Add @Deprecated to the constant itself.

5. DROP PROPERTY ... FORCE added to grammar but needs AST handling

dropPropertyBody now accepts FORCE?, but there is no corresponding field, visitor update, or toString() change visible in this diff. If FORCE is not wired through to execution, it will be silently ignored. Please confirm this is handled (or add a tracking TODO).

Minor

6. select from Foo bar (alias) silently dropped

The tests flip select from Foo bar from wrong-syntax to right-syntax, and the comment in visitFromIdentifier says the alias "is ignored in execution". This means SELECT a.name FROM Person p WHERE p.age > 18 parses without error but the alias binding has no effect at runtime. At minimum, add a comment in the grammar so users are not surprised.

What looks good

• StatementCache/SQLScriptQueryEngine simplifications are clean and correct
• SQLFunctionEval migration is correct — removing the ByteArrayInputStream wrapper is a nice cleanup
• InCondition.toString() correctly emits NOT IN when not == true (genuine bug fix)
• StatementExpression/SubqueryExpression now implement toString() — good fix
• matchFilterItemKey restricted to documented valid keys only — correct fix for the divergence that hid test failures
• visitInputParameter now handles :from (reserved keyword as named param) correctly
• AbstractParserTest refactor cleanly removes 40+ lines of duplicated boilerplate across 11 test files

Review response

Comment 1 (rejected) — @Deprecated prefix in description string
The description is a user-facing runtime string, not a Javadoc comment. Adding @Deprecated as text in a string has no effect on Java's deprecation mechanism. The codebase already shows the correct pattern: GREMLIN_COMMAND_TIMEOUT uses the actual @Deprecated Java annotation on the enum constant. The description string text "Deprecated, has no effect..." is sufficient.

Comment 2 (implemented) — protected fields + fluent setters in UpdatePutItem
Valid. Reverted fields to protected and added setLeft(), setKey(), setValue() fluent setters matching the UpdateIncrementItem pattern. Updated SQLASTBuilder.visitUpdatePutItem() to use the setters.

Review Summary (follow-up): Three prior reviews have covered this PR thoroughly. Summarizing unresolved items after the author response.

ADDRESSED: UpdatePutItem setters added (confirmed). The @deprecated annotation should also be placed on the enum constant, not just the description string.

STILL UNRESOLVED:

1. catch (Exception e) in AbstractProperty.getDefaultValue() - silently swallows NullPointerException, ClassCastException, etc. Please narrow to catch (CommandSQLParsingException e).

2. matchProperties now required after --> and <-- - this is a breaking grammar change. MATCH {class:Person, as:a} --> {class:Address, as:b} may now fail because ANTLR will consume {class:Address, as:b} as the edge properties instead of the next node. Please add a MatchStatementTest covering this exact pattern.

3. PARALLEL keyword not wired to AST - PARALLEL was added to grammar/lexer but there is no visitor in SQLASTBuilder and SelectStatement.toString() does not emit it. Wire it through or remove from this PR.

4. TRUNCATE RECORD comma-separated form removed - replacing TRUNCATE RECORD rid1, rid2 with TRUNCATE RECORD [rid1, rid2] is an undocumented breaking change.

The core parser migration (JavaCC to ANTLR everywhere) is correct and well-executed. Items 2 and 3 need explicit confirmation before merge.

Generated with Claude Code