Visitar URL original
feat(#10684): SQL reads out / in on an edge as its endpoints by robfrank · Pull Request #10806 · ArcadeData/arcadedb · GitHub
Skip to content

feat(#10684): SQL reads out / in on an edge as its endpoints - #10806

Merged
robfrank merged 3 commits into
mainfrom
feat/10684-sql-edge-out-in-endpoints
Oct 27, 2026
Merged

feat(#10684): SQL reads out / in on an edge as its endpoints#10806
robfrank merged 3 commits into
mainfrom
feat/10684-sql-edge-out-in-endpoints

Conversation

@robfrank

@robfrank robfrank commented Oct 27, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator

Closes #10684

Summary

On an edge, SQL reads the endpoints as @out / @in. The plain names out and in (the OrientDB spelling, or a natural guess) are not properties of an edge, so they silently answered null for every path through them: SELECT out.id FROM E, WHERE in.name = 'x', ORDER BY out. Of the three options the issue offers (a docs line, a warning, an error), this PR takes a fourth that removes the trap instead of documenting it: on an edge that has no property of that name, out / in now resolve to the edge's endpoints, exactly like @out / @in.

Precedence, so nothing that worked before changes:

  • a property named out / in on the edge record always wins (even with a null value);
  • a property the edge's TYPE declares under that name wins too, set on the record or not;
  • vertices, documents and projection rows (no element) are unchanged: still null.

An error was rejected: an edge type is schemaless, so a query reading out on a type where some records carry an out property and others do not would fail for one record and not the next.

Behavior change (release note)

Queries that read out / in on an edge with no such property used to get null and now get the endpoint RID. For example SELECT FROM E WHERE out IS NULL used to match every edge without an out property and now matches none. A record or schema property named out / in keeps its old meaning. Suggested release-note line: SQL: on an edge, out / in now read the endpoints (@out / @in) when the edge has no property of that name (#10684). The SQL docs (arcadedb-docs) could add the same sentence next to @out / @in.

Changes

  • SuffixIdentifier: both the Identifiable and the Result overloads fall back to the endpoint when the name is out / in, the record is an edge, and neither the record nor its type has such a property. The check runs only after the normal lookup missed and only for those two names, so the hot path of every other property read is untouched (one extra String.equals pair on a miss).
  • OrderByItem: the bare-name (alias) branch reads the row directly instead of through a SuffixIdentifier, so it asks the same helper (SuffixIdentifier.edgeEndpointIfUnshadowed); without it SELECT FROM E ORDER BY out ordered by null.

Test plan

  • Issue10684EdgeOutInEndpointsTest (13 tests): projection of out.id / in.name and bare out / in; WHERE out = <rid>, WHERE out.id = 1 AND in.name = 'x'; edges reached through a traversal (outE().in.name, outE('E')[0].out.id); ORDER BY out ASC/DESC with and without a projection; GROUP BY in.name; MATCH where: on an edge alias; a real out property wins; a schema-declared in property wins while unset; an explicit null out property stays null; a projected column named out (value or null) is not replaced, including under ORDER BY on an edge row (SELECT *, null AS out ... ORDER BY out); lightweight edges; vertices and documents unchanged.
  • Proven red without the fix: 5/6 of the initial tests fail against main's SuffixIdentifier; the ORDER BY test fails with only the OrderByItem change reverted.
  • engine packages com.arcadedb.query.sql.** and com.arcadedb.graph.** (excluding benchmark/slow/vector): 4400 tests, 0 failures, 0 errors.

Coverage

Invariant: on an edge with no property (record or schema) named out / in, every SQL read of that bare name answers the corresponding endpoint.

Entry point Status
SuffixIdentifier.execute(Result, ...) - projections, WHERE, GROUP BY, MATCH where on rows wrapping an edge fixed here, tested
SuffixIdentifier.execute(Identifiable, ...) - collections of edges (outE().in), chained paths fixed here, tested
OrderByItem.compare alias branch (ORDER BY out) fixed here, tested
SuffixIdentifier.execute(Map, ...) not applicable: a map is not an edge
ResultInternal.getProperty("out") Java API deliberately unchanged: the API reads properties; @out stays the structural attribute
Planner index/adjacency shortcut for WHERE @out = #x not extended to WHERE out = #x: the plain name still answers correctly, via a scan. Performance only, filed as #10807

Known gaps: planner shortcut for WHERE out = #rid (performance only) - #10807.

Docs: the SQL docs (separate arcadedb-docs repo) could mention that out / in are accepted as aliases of @out / @in on edges; @out / @in remain the canonical spelling.

Adversarial pass

The Phase 1.5 independent subagent could not be spawned (no subagent tool in this run). A self-review in its place found one real in-scope corner, fixed here: an edge TYPE that declares a property out / in but a record that leaves it unset answered the endpoint instead of null; the schema now wins (aPropertyNamedInDeclaredOnTheEdgeTypeWinsEvenWhenUnset).

Review cycles

Cycle Head Changes claude review
1 08873b5 initial fix + 9 tests looks good; non-blocking: release note, test explicit-null out, projected alias, lightweight edge, comment the ORDER BY null fallback
2 4c302dd tests for explicit null, projected out column, lightweight edges; ORDER BY comment looks good; non-blocking: test SELECT *, null AS out ... ORDER BY out on edge rows, plain-case comment, release note
3 a4c3813 that test; plain-case comment looks good to merge once the behavior change is acknowledged; planner follow-up suggested (filed #10807); test-count nit (fixed in this body)

Deferred / skipped items

  • "Lookup cost on a miss: existsPolymorphicProperty per row" - skipped: the reviewer calls it acceptable; it runs only after a miss on the two names out / in, and is a map lookup on the type.
  • "Confirm doc.has does not load anything extra for lightweight edges" - no change: has is called only after get already missed on the same record and only for out / in; lightweightEdgesResolveTheirEndpointsToo exercises the path.
  • Release note / docs line - acknowledged in the Behavior change section above; the docs repo is separate.

Final state: clean-approval

🤖 Generated with Claude Code

@robfrank @claude
feat(#10684): SQL reads out / in on an edge as its endpoints when the…
08873b5 
… edge has no such property

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Oct 27, 2026 •
edited
Loading

Copy link
Copy Markdown

Review in Change Stack →

Warning

Review limit reached

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Next included review available in 3 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used all 4 included reviews currently available.

Learn how review limits work.

Review configuration:

Run configuration
  • Source mode: Auto
  • Repository: Unavailable
  • Branch: Unavailable
  • File: No configuration file from this repository applied
  • Commit: Unavailable
  • Layers: defaults
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 9a718b49-847d-485c-bced-c9e34a16bbca
Commits

Reviewing files that changed from the base of the PR and between 08873b5 and a4c3813.

Files selected for processing: 2
  • engine/src/main/java/com/arcadedb/query/sql/parser/OrderByItem.java
  • engine/src/test/java/com/arcadedb/query/sql/Issue10684EdgeOutInEndpointsTest.java

No actionable comments were generated in the recent review.

Recent review info
Run configuration
  • Source mode: Auto
  • Repository: Unavailable
  • Branch: Unavailable
  • File: No configuration file from this repository applied
  • Commit: Unavailable
  • Layers: defaults
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 414cc3bc-8abd-4d5c-95f5-22fcf45c7ac1
Commits

Reviewing files that changed from the base of the PR and between 491dfa9 and 08873b5.

Files selected for processing: 3
  • engine/src/main/java/com/arcadedb/query/sql/parser/OrderByItem.java
  • engine/src/main/java/com/arcadedb/query/sql/parser/SuffixIdentifier.java
  • engine/src/test/java/com/arcadedb/query/sql/Issue10684EdgeOutInEndpointsTest.java

Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 0 remain after this review.

Walkthrough

Plain out and in now resolve to edge endpoints when no edge property shadows those names. The change applies to document and result evaluation, including ORDER BY alias comparison. Tests cover endpoint access across SQL query forms and property precedence.

Changes: Edge Endpoint Resolution

Layer / File(s) Summary
Endpoint fallback rules
engine/src/main/java/com/arcadedb/query/sql/parser/SuffixIdentifier.java		 SuffixIdentifier recognizes plain out and in and resolves them to edge endpoints when the edge does not define a property with that name. Document and result evaluation use this fallback.
Ordering and SQL query coverage
engine/src/main/java/com/arcadedb/query/sql/parser/OrderByItem.java, engine/src/test/java/com/arcadedb/query/sql/Issue10684EdgeOutInEndpointsTest.java		 ORDER BY alias comparison uses the endpoint fallback. Tests cover projections, filters, traversals, ordering, grouping, MATCH, property precedence, and non-edge records.

Priority: Pending latest changes

Estimated code review effort: 3 (Moderate) | ~15 minutes

Change: Bug fix · Severity of issue fixed: Medium

Possibly related issues

Suggested reviewers: lvca

Merge Risk: Minimal · previous review 08873

Bare edge endpoint names appear ready to merge after normal checks; no actionable merge-blocking issue was identified.

Pre-merge checks: Passed 4 · Failed 1

Failed Failed checks: 1 warning

Check name Status Explanation Resolution
Docstring Coverage Warning Docstring coverage is 16.67% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Documentation-only edits and deletions within surv… Write docstrings for the functions missing them to satisfy the coverage threshold.
Passed Passed checks: 4 passed
Check name Status Explanation
Title check Passed The title "feat(#10684): SQL reads out / in on an edge as its endpoints" matches the main change. The changeset makes bare out and in on edges resolve to the edge endpoints in SuffixIdentifier and the…
Linked Issues check Passed Issue #10684 reports that bare out and in on an edge silently return null. The issue offers a docs line, a warning, or an error as expected outcomes. The PR takes a different route: on an edge wit…
Out of Scope Changes check Passed The whole-PR diff against the merge base changes three areas. SuffixIdentifier.java adds the endpoint fallback and helpers for both the Identifiable and Result overloads. OrderByItem.java adds a f…
Description check Passed The description is detailed and directly addresses the change, motivation, related issue, implementation, behavior change, tests, coverage, and review notes. It does not use every template heading exa…
Full details: Title check

Explanation

The title "feat(#10684): SQL reads out / in on an edge as its endpoints" matches the main change. The changeset makes bare out and in on edges resolve to the edge endpoints in SuffixIdentifier and the OrderByItem alias branch, and it adds tests for that behavior. The title is concise and specific. It does not mention the precedence rules, but it does not need to.

Full details: Linked Issues check

Explanation

Issue #10684 reports that bare out and in on an edge silently return null. The issue offers a docs line, a warning, or an error as expected outcomes. The PR takes a different route: on an edge with no record or type property of that name, out and in resolve to the endpoints, as @out and @in do. Code at the reviewed head confirms this. In SuffixIdentifier.execute(Identifiable, ...), a null document value for out/in on an Edge with no such property returns edgeEndpoint(...). In SuffixIdentifier.execute(Result, ...), the miss path ends with edgeEndpointIfUnshadowed(...). edgeEndpoint returns null when edge.getType().existsPolymorphicProperty(name) is true, so declared properties win. OrderByItem.compare applies the same helper to the alias branch, so ORDER BY out matches the projection. The silent null for these reads on edges no longer occurs. The issue's docs option is non-coding and lives in a separate repository, so it does not apply to this assessment. The issue's listed options are examples, and the issue asks for a fix of the silent null. The chosen fix meets that need. Test coverage is reported as Issue10684EdgeOutInEndpointsTest (9 tests). I did not open the test file or run the tests. The API calls used in the code (existsPolymorphicProperty, getPropertyIfPresent, isEdge, getEdge) were not compiled or checked here.

Full details: Out of Scope Changes check

Explanation

The whole-PR diff against the merge base changes three areas. SuffixIdentifier.java adds the endpoint fallback and helpers for both the Identifiable and Result overloads. OrderByItem.java adds a fallback in the bare-name alias branch, so that ORDER BY out agrees with the projection. This is needed for the same behavior and is tied to #10684. Issue10684EdgeOutInEndpointsTest adds tests for the new behavior. The diff hunks read for SuffixIdentifier and OrderByItem contain no unrelated logic. The Map overload and ResultInternal.getProperty are left unchanged, as the PR states, and the diff shows no edits to them. The hot path for other property reads changes only by a miss-path check limited to the names out and in. No unrelated files appear in the change summary.

Full details: Description check

Explanation

The description is detailed and directly addresses the change, motivation, related issue, implementation, behavior change, tests, coverage, and review notes. It does not use every template heading exactly, and it does not explicitly confirm mvn clean package, but the required information is substantially present.

Full details: Docstring Coverage

Explanation

Docstring coverage is 16.67% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 18 functions across 3 files.

Resolution

Write docstrings for the functions missing them to satisfy the coverage threshold.

Finishing Touches: 💡 1
📝 Generate docstrings: 💡
  • Commit to this branch
  • Create a new PR
🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR
  • 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.

❤️ Share

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

@claude

claude Bot commented Oct 27, 2026

Copy link
Copy Markdown

Review of #10806

Summary: The change is small and well scoped. The fallback runs only after the normal lookup misses, and only for the names out and in, so the hot path is unaffected. Precedence is sensible: a record property wins, then a type-declared property, and vertices and documents are unchanged. I found no security concerns. It adds no dependencies, no new input surface, and no I/O.

Notes / suggestions (non-blocking):

  1. Behavior change. Queries that relied on out / in being null on edges will now see RIDs. One example is WHERE out IS NULL used to find edges without a property. That seems an acceptable trade for removing the silent trap, but please add a changelog or docs line saying so.
  2. Result path in SuffixIdentifier.execute(Result, ...). The fallback sits after the temporary-property check, which looks right. Please confirm it is also reached for a projection row that wraps an edge but carries a computed or aliased out property. row.hasProperty(name) should cover that, and a test would pin it down.
  3. OrderByItem. The fallback fires whenever aVal == null. That includes a projected alias out whose value is legitimately null, as long as the row is also an edge. Such a row is rare, and edgeEndpointIfUnshadowed checks hasProperty first, so the impact is probably nil. A short comment would make that intent clear.
  4. Schema lookup cost. existsPolymorphicProperty is called per row on a miss. It is cheap, but an edge query that selects a nonexistent out or in column repeats it for every row. That is fine.
  5. Tests. Coverage is good, and I like the red-without-fix check. Two cases would round it out: an edge row with an explicit null out property (UPDATE E SET out = null, expecting null), and a lightweight edge, if ArcadeDB supports one in this path.
  6. Style. The code follows CLAUDE.md conventions: imports, final, no System.out.

Overall this looks good to merge after the docs note.

@codacy-production

codacy-production Bot commented Oct 27, 2026 •
edited
Loading

Copy link
Copy Markdown

Up to standards ✅

🟢 Issues 0 issues

Results:
0 new issues

View in Codacy

🟢 Metrics 17 complexity

Metric Results
Complexity 17

View in Codacy

🟢 Coverage ∅ diff coverage

Metric Results
Coverage variation Report missing for 491dfa91
Diff coverage ✅ ∅ diff coverage

View coverage diff in Codacy

Coverage variation details
Coverable lines Covered lines Coverage
Common ancestor commit (491dfa9) Report Missing Report Missing Report Missing
Head commit (a4c3813) 283980 233450 82.21%

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 (#10806) 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%

1 Codacy didn't receive coverage data for the commit, or there was an error processing the received data. Check your integration for errors and validate that your coverage setup is correct.

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.

@mergify

mergify Bot commented Oct 27, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

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.

@robfrank @claude
address review: pin explicit-null, projected-alias and lightweight-ed…
4c302dd 
…ge cases; comment the ORDER BY null fallback

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
@claude

claude Bot commented Oct 27, 2026

Copy link
Copy Markdown

Review of #10806: edge out / in resolve to the endpoints

I read the diff only and did not build it or run the tests. I found no blocking issues and no security concerns. The change adds no dependencies, no new input handling and no new attack surface.

What looks good

  • Precedence is conservative. A record property wins (including an explicit null), a type-declared property wins (checked with existsPolymorphicProperty), and vertices, documents and projection rows are unchanged.
  • The hot path is untouched. The fallback only runs after a lookup has already missed, and the first check is two String.equals calls.
  • OrderByItem reuses SuffixIdentifier.edgeEndpointIfUnshadowed instead of duplicating the logic.
  • Test coverage is good. It covers projection, WHERE, ORDER BY, GROUP BY, MATCH, traversal-reached edges, lightweight edges, a real property named out, an explicit null property, a schema-declared in, a projected column named out, and vertices and documents.

Minor suggestions (non-blocking)

  1. In OrderByItem, the aVal == null fallback also runs when the alias is a projected column whose value is null. edgeEndpointIfUnshadowed guards this with isEdge() and hasProperty(), so it is correct, but a row that wraps an edge and projects null AS out is an edge case worth a test.
  2. The all-caps comment in OrderByItem is hard to read. Check that the surrounding code uses that style, and use normal case if it does not.
  3. SuffixIdentifier now returns an endpoint where it used to return null. Existing user queries that relied on out or in being null on edges (for example WHERE out IS NULL) will change behavior. That is the intended fix, but a note in the release notes or docs would help.

Looks good to merge once those points are addressed or acknowledged.

@robfrank @claude
address review: ORDER BY on a projected null out column of an edge ro…
a4c3813 
…w keeps the null; plain-case comment

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
@claude

claude Bot commented Oct 27, 2026

Copy link
Copy Markdown

Review of #10806 (out / in on edges resolve to endpoints)

No security concerns: the change only adds a read-side fallback, with no new dependencies, I/O or deserialization.

What looks good

  • The precedence rules are sensible and tested: a record property wins, a type-declared property wins, and vertices and documents are unchanged.
  • The fallback runs only after the normal lookup misses, and only for two names, so the hot path is unaffected.
  • The tests cover projection, WHERE, ORDER BY, GROUP BY, MATCH, lightweight edges and the shadowing cases. They use the assertThat(...) style the repo asks for.

Points to consider

  1. Behaviour change, not only a fix. Queries that relied on out / in returning null on edges now get RIDs. For example, WHERE out IS NULL used to match every edge and now matches none. That is probably fine, but it deserves a release-note line, and the docs repo should get the @out / @in alias note.
  2. Lookup cost on a miss. edgeEndpoint calls edge.getType().existsPolymorphicProperty(name). That is cheap, but it runs once per row for every query that reads out / in on edges. This is acceptable. If it ever shows up in a profile, you could cache the type check per query.
  3. SuffixIdentifier.execute(Identifiable). The fallback is applied inside the Document branch via doc.has(varName). Please confirm that has doesn't load or deserialize anything extra for EdgeSegment / lightweight edges. The lightweight test passes, so this is probably fine.
  4. OrderByItem. The fallback runs only when the value is null, and hasProperty then keeps explicit nulls. The behaviour is correct, and the SELECT *, null AS out test covers it.
  5. Planner shortcut. WHERE out = #x falls back to a full scan, while WHERE @out = #x can use adjacency. The PR notes this as performance only. A follow-up issue would be worth opening.
  6. PR description nit. It says 9 tests, but the file has 14.

Overall this looks good to merge once point 1 is acknowledged.

@robfrank robfrank mentioned this pull request Oct 27, 2026
Closed
@codecov

codecov Bot commented Oct 27, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 78.99%. Comparing base (d37fed8) to head (a4c3813).
⚠️ Report is 2 commits behind head on main.

Additional details and impacted files 
@@             Coverage Diff              @@
##               main   #10806      +/-   ##
============================================
+ Coverage     78.97%   78.99%   +0.02%     
+ Complexity     4768     4766       -2     
============================================
  Files          2299     2299              
  Lines        251208   251279      +71     
  Branches      54790    54812      +22     
============================================
+ Hits         198389   198506     +117     
+ Misses        32597    32557      -40     
+ Partials      20222    20216       -6

☔ View full report in Codecov by Harness.
📢 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.

@lvca lvca mentioned this pull request Oct 27, 2026
Merged
@robfrank
robfrank merged commit 2a205e8 into main Oct 27, 2026
35 of 37 checks passed
@tae898 tae898 mentioned this pull request Oct 27, 2026
Closed
robfrank added a commit that referenced this pull request Nov 5, 2026
@robfrank
feat(#10684): SQL reads out / in on an edge as its endpoints (#10806)
4390ce4 
(cherry picked from commit 2a205e8)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

SQL on an edge: out and in silently return null (the endpoints are @out / @in); a docs line or an error would help

1 participant

@robfrank