fix(#10442): Gremlin, GraphQL and MongoDB QL no longer answer nothing over a time-series type - #10468

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

_{Comment @coderabbitai help to get the list of available commands.}

This pull request does not currently match the merge queue conditions, so it cannot be queued from here. The box comes back if it matches again.

Review of #10468

Overall this is a clean, well-scoped fix. The "serve the samples or fail naming the type" invariant is easy to reason about, the error messages are actionable, and the tests pin the before/after behavior across all three engines. I read the diff only and did not run the tests. Notes below, none blocking.

Gremlin (TimeSeriesLabelGuard)

• Running the guard first in apply(), before the rewrites strip the label from has(), is the right place. Skipping the body of not() and only refusing eq/within (recursing through and/or) matches the Cypher semantics well.
• The guard only runs when traversal.getGraph() is an ArcadeGraph. The PR description already lists the remote ArcadeGraph case as unverified. If the remote schema doesn't expose TimeSeriesType, that path silently returns the old empty result. A follow-up issue would be good so the gap doesn't get lost.
• Some predicate shapes are still silent. A non-eq/within positive predicate such as P.test(...) or a custom BiPredicate is not checked. That is probably fine, but a one-line comment saying so would help the next reader.
• The check does a schema lookup per label on every strategy application, including each child traversal. That is cheap, but a has() with a very large within() list means N lookups per compile. This is fine for realistic use.
• getParent() instanceof NotStep only covers a direct not() parent. where(not(...)) goes through NotStep as well, so it is covered. A test for not(and(hasLabel('Ev'), ...)) would pin the nested case.

GraphQL

• requireNotTimeSeriesType does a schema lookup on every @relationship evaluation, i.e. per row, in the result-set hot path. Resolving the type once per field, or caching the check, would avoid repeating it per row. The lookup is a hash get, so the impact is small, but the check is per row without needing to be.
• getDatabase() is package-private, so the surface stays small.

MongoDB

• Splitting scanMatches (RID contract) from scanMatchingRows is a good way to keep the update/delete paths unchanged. The Javadoc on both explains why.
• The getTypeOrNull(collectionName) instanceof TimeSeriesType check in queryDocuments is a schema lookup per find, which is negligible.
• Known gap: filtered update/delete and upsert still silently match nothing (mongodbw: a filtered update/delete over a TIMESERIES type silently matches nothing (#10442) #10464). Good that it is tracked.

Tests

• Coverage is good, and I like that the PR records which cases fail without the fix. The parameterized Gremlin test is a nice way to cover the matrix.
• Per CLAUDE.md, a test using a hardcoded DB path under ./target is fine. Please confirm that the Mongo test, which uses a real wire client, takes its port from the fixture rather than a fixed one (HardcodedTestServerPortsTest will flag it otherwise).
• The PR notes one MongoDBAuthorizationTest connect timeout in the full run. It passed on rerun, but keep an eye on it in CI.

Style

• Conventions match (final params, imports instead of FQNs, assertThat(...) style). No System.out left behind.

LGTM pending CI.

Up to standards ✅

_{NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.}

Review of #10468 (TIMESERIES handling in Gremlin, GraphQL, MongoDB QL)

Overall this is a solid, well-scoped change. I read the diff but did not build it or run the tests.

What looks good

• Failing loudly with an error that names the type is better than a silently empty answer, and it matches the Cypher behaviour from Cypher MATCH on a time-series type silently returns no rows (SQL sees the samples), while the docs list Cypher as a time-series query language #10440.
• TimeSeriesLabelGuard runs before the rewrites remove the label from the has() step. It treats negations (neq, without, not()) as answerable, and isNegated counts nested not() parity correctly.
• The cost is one schema hash lookup per label at compile time, and no per-element cost.
• Test coverage is broad: the Gremlin cases are parameterized, the negation cases are tested, and there are regression tests for graph labels that should be unaffected.

Points to consider

1. GraphQL, GraphQLResultSet: requireNotTimeSeriesType runs inside evaluateDirectives, which is evaluated per row. It costs one schema lookup per row per @relationship field. It is cheap, but it could be checked once per field (for example at schema build time, or cached on the field). That would also fail an empty result set consistently. Today, if no Station rows exist, the misconfiguration is never reported.
2. Gremlin guard scope: the guard only fires when traversal.getGraph() is an ArcadeGraph. Please confirm that the SQL/gremlin() and remote (server) paths always hit that branch. The tests cover graph.gremlin(query) and graph.traversal(), but none go through the server's Gremlin endpoint.
3. Gremlin predicates not covered: Compare.eq and Contains.within are handled, but a TextP or a custom predicate that matches the label (for example startingWith('E')) still silently returns nothing. This is probably acceptable, but it deserves a line in the Javadoc, which currently says only that "any other predicate ... names no label it asks for".
4. MongoDB, MongoDBCollectionWrapper: the getTypeOrNull(collectionName) instanceof TimeSeriesType check is now on the hot path of every unfiltered find. It is a hash lookup, so this is fine. MongoFilter.scanMatches now silently skips rows with no identity, as before, so update/delete paths cannot touch samples. A test that deleteMany or updateMany on a time-series collection is a no-op or an error would pin that.
5. Minor: the GraphQL test uses ./target/testgraphql-timeseries-10442 and the Gremlin test uses ./target/testgremlin-timeseries-10442. Both clean up in tearDown, but they should also guard against a leftover directory from a crashed run, as the Gremlin setUp doesn't. The GraphQL test already does this with factory.exists().

None of this blocks the merge. Nice work.

Review of #10468 (TIMESERIES types in Gremlin, GraphQL and MongoDB QL)

I read the diff but did not build the PR or run its tests. The approach is consistent across the three engines: each one either serves the samples or fails with an error naming the type. The new tests pin both the new behavior and the paths that already worked.

What looks good

• TimeSeriesLabelGuard runs first in ArcadeTraversalStrategy.apply(), before the rewrites remove the label from the has() step. The comment explains why.
• Negations (neq, without, not()) are deliberately answered, which matches Cypher's WHERE NOT n:Ev. The isNegated walk up the parent chain handles nested not() correctly.
• The cost is one schema hash lookup per label at compile time, which is acceptable.
• In Mongo, scanMatches is kept as a thin wrapper over the new scanMatchingRows. Callers that write (update/delete) still skip rows with no identity, so a time-series sample can't be targeted by a mutation.

Points to consider

1. Gremlin guard coverage gaps.
   • TextP.startingWith and custom predicates are not refused. The Javadoc admits this, so it is a documented limit. A test that pins it would stop a later change from regressing it silently.
   • The guard only covers read-side has() and hop labels. addV('Ev') and addE('Ev') are not covered. That may be intended, since writes to a time-series type through Gremlin look out of scope here, but it is worth confirming.
2. Guard cost on every traversal. check() runs for the root and every child traversal. It is cheap, but ArcadeGraph arcadeGraph is only pattern-matched on the root's graph. Please confirm that child traversals always have getGraph() set when the strategy is applied. If one doesn't, its labels silently skip the guard. A test with where(__.out('Ev')) already seems to cover this, so I'd just double-check it is in the suite.
3. Mongo queryDocuments adds a schema lookup per unfiltered find. getTypeOrNull(collectionName) is a hash lookup, so this is fine. The check could move into MongoFilter or a small helper, because count() and queryDocuments now both reason about time-series types separately.
4. scanMatchingRows now passes rows without an identity. count() with skip/limit uses it correctly. If a future caller uses it for anything that needs an RID, it will need to handle the empty identity. Mentioning that in the Javadoc would help. It partly does already.
5. GraphQL getDatabase() is added to GraphQLSchema only so GraphQLResultSet can reach the schema. That's fine at package-private scope. Alternatively, requireNotTimeSeriesType could run once when the SDL is parsed, if the @relationship type is known then. That would fail fast at schema time instead of on every resolution, although it would require the type to exist when the SDL is submitted.
6. Tests.
   • They look solid and follow the repo style (assertThat(...), no fixed ports, ports derived through the base fixture).
   • The GraphQL tests use a fixed ./target/... DB path. That matches other graphql tests, so it's OK.
   • Please check that none of them rely on wall-clock timing. I didn't see any.

Style

• CLAUDE.md conventions are followed: final is used, imports are explicit, and there are no debug System.out calls.
• The instanceof pattern matching is idiomatic for this codebase.

No blocking issues found. The Gremlin coverage gaps in item 1 are the main thing I'd want settled or documented before merge.

Review

Overall this is a well-scoped fix. It follows the Cypher choice from #10440 (fail loudly, or serve the samples through SQL), and the tests cover the before/after cases. I only read the diff and did not run the tests. Notes below, none blocking.

Gremlin (TimeSeriesLabelGuard)

• The guard runs before the rewrites that remove the label from the has() step, and the cost is a single schema hash lookup per label at compile time. Both are good.
• checkName calls name.toString() on whatever eq or within holds. A non-String label such as a T token or a number just misses the lookup, which is harmless. An early instanceof String check would make that explicit.
• isNegated walks parents counting NotStep. Other steps that negate or invert (for example none()-style steps, or where(P.not(...))) are not modelled. The Javadoc already records the pattern-predicate limit, so adding this one to the "known limits" note would be enough.
• The guard is skipped when the graph is not an ArcadeGraph (the instanceof guard). That is reasonable, but a short comment on when that happens would help.

GraphQL

• requireNotTimeSeriesType does a schema lookup each time a @relationship field is resolved. If that happens per parent row, resolve it once per field, or cache it in the result set. It is cheap, but it sits on a hot path.
• Adding the package-private GraphQLSchema.getDatabase() accessor is fine. CommandSemanticException is consistent with the other engines.

MongoDB

• scanMatches now silently skips rows without an identity. For update and delete on a TIMESERIES collection this means a no-op that reports success, which is the same class of silent-empty answer this PR removes elsewhere. Consider failing explicitly for writes on a time-series collection, or pin the current behaviour with a test.
• queryDocuments repeats the getTypeOrNull(...) instanceof TimeSeriesType check inline. A small helper would let count and find share it and keep them from drifting.
• Moving count to scanMatchingRows is a good change. It removes the old inconsistency where the unfiltered count saw 3 and the filtered count saw 0.

Tests

• Coverage is good, including the negation and nested-not() cases and the "graph labels unaffected" regression guard.
• There are no tests for Mongo update/delete on a time-series collection (see above).
• Tests use fixed relative DB paths with manual cleanup. That matches the existing style and does not bind any ports, so it is fine.

Style (CLAUDE.md)

• The code adheres to the conventions: single-statement if without braces, final parameters, assertj assertThat style, and no new dependencies.

Nice work.

Codecov Report

❌ Patch coverage is 83.05085% with 10 lines in your changes missing coverage. Please review.
✅ Project coverage is 78.68%. Comparing base (b4a6cb6) to head (1e6dc94).
⚠️ Report is 4 commits behind head on main.

@@            Coverage Diff            @@
##               main   #10468   +/-   ##
=========================================
  Coverage     78.67%   78.68%           
+ Complexity     4733     4731    -2     
=========================================
  Files          2278     2279    +1     
  Lines        244964   245020   +56     
  Branches      53204    53222   +18     
=========================================
+ Hits         192720   192783   +63     
+ Misses        32413    32412    -1     
+ Partials      19831    19825    -6     

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