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 a bug in the SQL parser that prevented correct retrieval of internal record properties (like @Rid or @type) when accessed as a suffix on the result of a function call. The fix involves refactoring the AST builder to properly identify these special attributes, whether they are unquoted or backtick-quoted, ensuring that SQL queries involving such expressions now return the expected values.

Highlights

• SQL Parsing Fix: Resolved an issue where accessing internal properties (e.g., @Rid, @type) on function results in SQL queries (e.g., inE().@Rid) incorrectly returned null.
• Record Attribute Recognition: Enhanced the SQL AST builder to correctly recognize both unquoted and backtick-quoted internal record attributes.
• Code Refactoring: Introduced new helper methods, buildSuffixForIdentifier and isRecordAttributeName, to centralize and improve the logic for parsing record attributes.
• Improved Test Coverage: Added a dedicated test file to validate the fix for various scenarios involving internal property access on function results.

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

🧪 CI Insights

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

🟢 All jobs passed!

But CI Insights is watching 👀

Code Review

Good fix for a clear and well-described bug. The approach is sound. Here are my observations:

Summary

The PR correctly identifies that visitModifier() was creating a plain Identifier node for record attribute tokens, causing inE().@rid to return null. Extracting buildSuffixForIdentifier() and applying it in both visitModifier and visitIdentifierChain is a clean refactoring.

Issues / Observations

1. Inconsistency: PROPS_ATTR not checked in the unquoted token path

buildSuffixForIdentifier checks for RID_ATTR, TYPE_ATTR, IN_ATTR, OUT_ATTR, and THIS in the token path, but not PROPS_ATTR (@props). The same gap exists in visitIdentifierChain's handling of the first identifier (lines 3106-3108 in the current code). Meanwhile, isRecordAttributeName does cover PROPERTY_TYPES_PROPERTY (@props).

If the grammar's identifier rule includes PROPS_ATTR as an alternative (it defines the token in the lexer), then .@props as an unquoted suffix would not be caught by the token check and would fall through to isRecordAttributeName. Whether that fallback actually catches it depends on how visit(idCtx) resolves an PROPS_ATTR token. Worth checking to ensure @props behaves consistently with @rid.

Similarly, @cat has no grammar token (CAT_ATTR is absent from the lexer), so it relies entirely on the isRecordAttributeName string-matching fallback. That seems intentional, but a comment explaining this would help future readers.

2. First-identifier handling in visitIdentifierChain not fully aligned

At line 3106-3108, the record-attribute check on the first identifier still uses the raw token checks but not the isRecordAttributeName fallback for backtick-quoted variants like `@rid`. So `@rid`.someProp at the start of a chain would not get RecordAttribute treatment, while .`@rid` in the middle (via buildSuffixForIdentifier) would. This asymmetry may or may not matter in practice, but it's worth documenting or aligning.

3. Test data accumulates across runs

Each test inserts vertices/edges without cleanup. IF NOT EXISTS prevents DDL failures on reruns, but data accumulates — on a second run, SELECT inE().@rid FROM V3571a would return a vertex row with two inbound edges instead of one. The tests only assert isNotEmpty(), which still passes, but it reduces determinism. Consider adding a @BeforeEach cleanup or asserting exact counts.

4. Test coverage: only @rid is exercised

All four test cases exercise @rid. It would strengthen confidence to add at least one case for @type and @cat (since they rely on the isRecordAttributeName string-matching path rather than the grammar token path), confirming the fallback path works correctly too.

Minor Notes

• Using Property.RID_PROPERTY etc. instead of magic strings is the right call.
• equalsIgnoreCase is appropriate for attribute name matching.
• The Javadoc on buildSuffixForIdentifier and isRecordAttributeName is clear and helpful.
• Test naming is clear and consistent with the issue number.

Overall this is a focused, well-targeted fix. Addressing the PROPS_ATTR token-path gap and expanding test coverage to non-@rid attributes would make it more complete.

Code Review

This is a clean, well-scoped fix for issue #3571. Here is my feedback.

What the fix does well

• Root cause correctly identified: visitModifier() was always creating a plain Identifier node even when encountering record-attribute tokens like @rid, so .@rid on a function result fell through to document.get("@rid") and returned null. The fix routes through buildSuffixForIdentifier() which creates a RecordAttribute node instead.
• Good extraction of the helper: buildSuffixForIdentifier() removes the duplicated attribute-detection block that existed in both visitModifier and visitIdentifierChain, keeping things DRY.
• Test coverage is solid: Six scenarios covering unquoted/quoted variants, @rid, @type, and @cat, including both list access and indexed [0] access. The test class follows project conventions (TestHelper, assertThat, IF NOT EXISTS, etc.).

Issues / suggestions

1. equalsIgnoreCase in isRecordAttributeName — is case-insensitivity intentional?

The method uses equalsIgnoreCase, which means a backtick-quoted @RID would also be treated as a record attribute. The unquoted path (RID_ATTR, TYPE_ATTR, etc.) is driven by the lexer, which presumably already handles case. For the backtick-quoted path the original string is preserved, so equalsIgnoreCase could silently accept @RID as a record attribute. If that is intentional (for user-friendliness), it should be noted; if not, a plain equals is safer and more consistent with the token path.

2. Namespace-qualified function check still duplicates attribute detection

In visitIdentifierChain, the early-return guard for namespace-qualified calls still lists the grammar tokens explicitly:

firstIdCtx.RID_ATTR() == null && firstIdCtx.TYPE_ATTR() == null
    && firstIdCtx.IN_ATTR() == null && firstIdCtx.OUT_ATTR() == null && firstIdCtx.THIS() == null

This does not cover backtick-quoted variants. Since buildSuffixForIdentifier now centralises that logic, a small helper isRecordAttributeContext(IdentifierContext) that combines the token checks with the string check could make this guard consistent with the rest of the fix. In practice the scenario is unlikely, but the inconsistency is worth noting.

3. @props is included in isRecordAttributeName but has no test

PROPERTY_TYPES_PROPERTY (@props) is listed in the method but there is no test exercising it via dot notation on a function result. Not a blocker, but a test would confirm the code path works end-to-end.

4. Minor: @this as a suffix attribute

@this is recognised by isRecordAttributeName, and it also has a dedicated grammar token (THIS()), so the unquoted path already handles it. The backtick-quoted variant is now also handled. Worth confirming that RecordAttribute for @this in a suffix position (e.g. inE()[0].@this) produces the expected result (the record itself) rather than null.

Summary

The fix is correct and production-ready as-is. Points 1 and 2 are the most substantive; the others are minor gaps in coverage and consistency. Overall a well-handled bug fix with good test coverage.

Coverage summary from Codacy

See diff coverage on Codacy

See your quality gate settings    Change summary preferences

Codecov Report

❌ Patch coverage is 74.07407% with 7 lines in your changes missing coverage. Please review.
✅ Project coverage is 65.30%. Comparing base (687b972) to head (474698e).
⚠️ Report is 40 commits behind head on main.

@@            Coverage Diff             @@
##             main    #3572      +/-   ##
==========================================
- Coverage   65.79%   65.30%   -0.49%     
==========================================
  Files        1506     1506              
  Lines      103053   103092      +39     
  Branches    21366    21381      +15     
==========================================
- Hits        67801    67329     -472     
- Misses      26030    26566     +536     
+ Partials     9222     9197      -25     

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