feat: upgrade Apache Tinkerpop Gremlin from 3.7.5 to 3.8.0 by robfrank · Pull Request #3667 · ArcadeData/arcadedb (original) (raw)

Code Review

Overall this is a clean, well-scoped upgrade. The diff is minimal and the test plan is solid. A few points worth considering:

IllegalStateException catch is semantically too broad

File: GremlinValueComparator.java:133

The old code caught GremlinTypeErrorException, a purpose-built exception with a single, well-defined cause. The replacement catches IllegalStateException, which is a standard Java runtime exception thrown by many unrelated things (iterators, concurrent modification, framework internals, etc.).

The risk: if an unrelated IllegalStateException is thrown deep inside comparator(type).compare(f, s) (e.g., inside a custom Comparator registered by a user type), it will be silently swallowed and equals() will return false — a hard-to-diagnose silent failure.

Suggestion: Consider wrapping the thrown exception with a sentinel message to narrow the catch:

// in throwTypeError(): throw new IllegalStateException("GremlinTypeError");

// in the catch block: } catch (IllegalStateException ex) { if (!"GremlinTypeError".equals(ex.getMessage())) throw ex; // type error -> false for equality return false; }

This preserves the intent of the original GremlinTypeErrorException pattern without requiring the removed class.

throwTypeError() message is empty

File: GremlinValueComparator.java:161

throw new IllegalStateException() has no message, making stack traces and logs much harder to diagnose. At minimum, add a descriptive message:

throw new IllegalStateException("Gremlin type error: incompatible types for comparison");

comparable() visibility change to public

File: GremlinValueComparator.java:303

This is a necessary adaptation to Gremlin 3.8.0's API change (the upstream now calls this method externally). The change is correct. It does expose internal logic, but since this class lives in the org.apache.tinkerpop.gremlin.util package — already a Gremlin namespace — it aligns with the upstream's design intent.

Behavioral breaking change — long overflow

File: GremlinTest.java:438-440

The longOverflow test correctly reflects the new Gremlin 3.8.0 behavior. However, this is a user-visible breaking change: queries that previously relied on silent wrapping (e.g., g.inject(Long.MAX_VALUE, 1).sum() silently returning Long.MIN_VALUE) will now throw ArithmeticException. This should be called out in the release notes / CHANGELOG so users can update their queries to use BigInteger when overflow is possible.

Test coverage looks good

The three test scenarios (no-overflow path still works, overflow throws ArithmeticException, BigInteger path unaffected) provide adequate regression coverage for this change. The existing 236 gremlin tests passing gives good confidence. No additional tests needed.

Summary: The upgrade is correct and the test plan is well-executed. The main actionable item is the overly-broad IllegalStateException catch — worth addressing to prevent silent miscomparisons in edge cases.