|
| 1 | +# Bug Fix: Issue #3315 - CREATE EDGE Expression Resolution |
| 2 | + |
| 3 | +## Issue Summary |
| 4 | +**Title:** SQL: CREATE EDGE fails to resolve argument expressions correctly |
| 5 | + |
| 6 | +**Issue URL:** https://github.com/ArcadeData/arcadedb/issues/3315 |
| 7 | + |
| 8 | +**Reporter:** gramian (COLLABORATOR) |
| 9 | + |
| 10 | +**Labels:** sql |
| 11 | + |
| 12 | +**Status:** Open |
| 13 | + |
| 14 | +## Problem Description |
| 15 | + |
| 16 | +The `CREATE EDGE` SQL command fails to properly resolve argument expressions when using query result sets as arguments. This causes the error: |
| 17 | +``` |
| 18 | +Invalid vertex for edge creation: {@rid: #1:0} |
| 19 | +``` |
| 20 | + |
| 21 | +### Failing Cases |
| 22 | + |
| 23 | +1. **Using LET variable with property access:** |
| 24 | + ```sql |
| 25 | + LET $x = SELECT @rid FROM V; |
| 26 | + CREATE EDGE E FROM $x.@rid TO $x.@rid |
| 27 | + ``` |
| 28 | + |
| 29 | +2. **Using subquery with property access:** |
| 30 | + ```sql |
| 31 | + CREATE EDGE E FROM (SELECT @rid FROM V).@rid TO (SELECT @rid FROM V).@rid |
| 32 | + ``` |
| 33 | + |
| 34 | +### Working Case |
| 35 | + |
| 36 | +```sql |
| 37 | +LET $x = (SELECT @rid FROM V).@rid; |
| 38 | +CREATE EDGE E FROM $x TO $x |
| 39 | +``` |
| 40 | + |
| 41 | +## Root Cause Analysis |
| 42 | + |
| 43 | +### Technical Analysis (by java-architect agent) |
| 44 | + |
| 45 | +**Bug Location:** `SuffixIdentifier.java` - Property extraction mechanism |
| 46 | + |
| 47 | +**Execution Flow:** |
| 48 | +1. `CreateEdgeStatement` → `CreateEdgeExecutionPlanner` creates execution plan |
| 49 | +2. FROM/TO expressions stored as global variables via `GlobalLetExpressionStep` |
| 50 | +3. Expressions evaluated: `$x.@rid` or `(SELECT ...).@rid` |
| 51 | +4. `SuffixIdentifier.execute(Iterable, CommandContext)` iterates over Result list |
| 52 | +5. **BUG:** Property extraction returns `Result` wrappers instead of unwrapped RID values |
| 53 | +6. `CreateEdgesStep.asVertex()` receives wrapped Results and fails |
| 54 | + |
| 55 | +**Key Files:** |
| 56 | +- `/engine/src/main/java/com/arcadedb/query/sql/parser/SuffixIdentifier.java` (lines 104-184) |
| 57 | +- `/engine/src/main/java/com/arcadedb/query/sql/executor/CreateEdgesStep.java` (lines 291-309) |
| 58 | +- `/engine/src/main/java/com/arcadedb/query/sql/executor/GlobalLetExpressionStep.java` (line 56) |
| 59 | + |
| 60 | +**Why It Fails:** |
| 61 | +- When `$x.@rid` is evaluated where `$x` is `List<Result>` |
| 62 | +- Property access extracts `@rid` but keeps Result wrapper: `[Result{@rid: #1:0}]` |
| 63 | +- Should return unwrapped RIDs: `[#1:0]` |
| 64 | +- `asVertex()` fails on Result wrapper: "Invalid vertex for edge creation: {@rid: #1:0}" |
| 65 | + |
| 66 | +**Why Working Case Succeeds:** |
| 67 | +- `LET $x = (SELECT @rid FROM V).@rid` evaluates property access during LET |
| 68 | +- Result is clean list of RIDs stored in `$x` |
| 69 | +- CREATE EDGE receives proper RID objects, not wrapped Results |
| 70 | + |
| 71 | +## Workflow Steps |
| 72 | + |
| 73 | +### Step 1: Branch Creation ✅ |
| 74 | +- Branch: `fix/3315-create-edge` (already created) |
| 75 | +- Documentation: `3315-create-edge.md` (this file) |
| 76 | + |
| 77 | +### Step 2: Analysis and Test Creation |
| 78 | +- [x] Analyze the SQL query executor code for CREATE EDGE (java-architect agent) |
| 79 | +- [x] Identify the expression resolution mechanism (SuffixIdentifier.java) |
| 80 | +- [x] Write failing tests that reproduce all bug scenarios (test-automator agent) |
| 81 | +- [x] Verify tests fail with current code |
| 82 | + |
| 83 | +**Test Details:** |
| 84 | +- File: `engine/src/test/java/com/arcadedb/query/sql/QueryTest.java` |
| 85 | +- Method: `createEdgeWithPropertyAccessOnResultSet()` (lines 677-767) |
| 86 | +- Test command: `mvn test -Dtest=QueryTest#createEdgeWithPropertyAccessOnResultSet` |
| 87 | +- Status: FAILS with expected error "Invalid vertex for edge creation: {@rid: #4:0}" |
| 88 | + |
| 89 | +### Step 3: Implementation |
| 90 | +- [x] Implement fix for expression resolution (backend-developer agent) |
| 91 | +- [x] Verify all new tests pass |
| 92 | +- [x] Run existing test suite to prevent regressions |
| 93 | +- [x] Clean up any debug code |
| 94 | + |
| 95 | +### Step 4: Verification |
| 96 | +- [x] All new tests pass (QueryTest#createEdgeWithPropertyAccessOnResultSet) |
| 97 | +- [x] No regressions in existing tests (40 tests passed) |
| 98 | +- [x] Code quality standards met |
| 99 | + |
| 100 | +## Progress Log |
| 101 | + |
| 102 | +### Initial Setup |
| 103 | +- Created documentation file |
| 104 | +- Confirmed branch is correct |
| 105 | +- Ready to begin agent coordination |
| 106 | + |
| 107 | +### Analysis Phase (java-architect agent) |
| 108 | +- Analyzed CREATE EDGE execution path |
| 109 | +- Identified bug in SuffixIdentifier.java property extraction |
| 110 | +- Root cause: Property access on Result collections returns wrapped Results instead of unwrapped values |
| 111 | +- Documented execution flow and key classes involved |
| 112 | + |
| 113 | +### Test Creation Phase (test-automator agent) |
| 114 | +- Created comprehensive test in QueryTest.java |
| 115 | +- Test method: `createEdgeWithPropertyAccessOnResultSet()` (lines 677-767) |
| 116 | +- Covers all three scenarios from the issue |
| 117 | +- Test initially fails with expected error message |
| 118 | +- Test file: `engine/src/test/java/com/arcadedb/query/sql/QueryTest.java` |
| 119 | + |
| 120 | +### Implementation Phase (backend-developer agent) |
| 121 | +- Fixed SuffixIdentifier.java (lines 175-201) |
| 122 | + - Modified `execute(Iterable)` and `execute(Iterator)` methods |
| 123 | + - Added unwrapping logic for Result objects when extracting properties |
| 124 | + - Properly handles record attributes like `@rid` |
| 125 | +- Enhanced CreateEdgesStep.java (lines 291-312) |
| 126 | + - Improved `asVertex()` method to handle projection Results |
| 127 | + - Extracts `@rid` property from Results without vertex elements |
| 128 | +- All tests pass: 40 related tests, 0 failures |
| 129 | + |
| 130 | +### Verification Phase |
| 131 | +- New test passes all three scenarios |
| 132 | +- No regressions in existing test suite |
| 133 | +- Code follows project standards and patterns |
| 134 | +- Changes staged in git (not committed per instructions) |
| 135 | + |
| 136 | +## Implementation Summary |
| 137 | + |
| 138 | +### Files Modified |
| 139 | + |
| 140 | +1. **SuffixIdentifier.java** (`engine/src/main/java/com/arcadedb/query/sql/parser/SuffixIdentifier.java`) |
| 141 | + - Fixed property extraction from Result collections |
| 142 | + - Added unwrapping logic for Result objects |
| 143 | + - Lines modified: 175-201 |
| 144 | + |
| 145 | +2. **CreateEdgesStep.java** (`engine/src/main/java/com/arcadedb/query/sql/executor/CreateEdgesStep.java`) |
| 146 | + - Enhanced vertex resolution for projection Results |
| 147 | + - Added `@rid` property extraction fallback |
| 148 | + - Lines modified: 291-312 |
| 149 | + |
| 150 | +3. **QueryTest.java** (`engine/src/test/java/com/arcadedb/query/sql/QueryTest.java`) |
| 151 | + - Added comprehensive test method |
| 152 | + - Method: `createEdgeWithPropertyAccessOnResultSet()` |
| 153 | + - Lines added: 677-767 |
| 154 | + |
| 155 | +### Changes Summary |
| 156 | +- 3 files changed |
| 157 | +- 143 insertions (+) |
| 158 | +- 4 deletions (-) |
| 159 | +- Net change: +139 lines |
| 160 | + |
| 161 | +### Test Results |
| 162 | + |
| 163 | +**New Test:** `QueryTest#createEdgeWithPropertyAccessOnResultSet` |
| 164 | +- ✅ Scenario 1: LET variable with property access - PASS |
| 165 | +- ✅ Scenario 2: Direct subquery with property access - PASS |
| 166 | +- ✅ Scenario 3: Baseline working case - PASS |
| 167 | + |
| 168 | +**Regression Testing:** |
| 169 | +- ✅ QueryTest: 26 tests passed |
| 170 | +- ✅ CreateEdgeStatementTest: 6 tests passed |
| 171 | +- ✅ CreateEdgeParametersTest: 1 test passed |
| 172 | +- ✅ CreateEdgeStatementExecutionTest: 7 tests passed |
| 173 | +- ✅ Total: 40 tests passed, 0 failures |
| 174 | + |
| 175 | +## Impact Analysis |
| 176 | + |
| 177 | +### What Changed |
| 178 | +The fix ensures that when extracting properties from collections of Result objects (e.g., from SELECT queries), the actual property values are returned instead of Result wrappers. This allows CREATE EDGE statements to properly resolve vertex references from query expressions. |
| 179 | + |
| 180 | +### Components Affected |
| 181 | +- **SQL Parser:** SuffixIdentifier property extraction mechanism |
| 182 | +- **SQL Executor:** CreateEdgesStep vertex resolution |
| 183 | +- **Query Execution:** Expression evaluation for CREATE EDGE statements |
| 184 | + |
| 185 | +### Backward Compatibility |
| 186 | +- ✅ All existing tests pass |
| 187 | +- ✅ No breaking changes to API |
| 188 | +- ✅ Enhancement only - fixes broken functionality |
| 189 | +- ✅ Working cases remain working |
| 190 | + |
| 191 | +### Performance Impact |
| 192 | +- Minimal: Only affects property extraction from Result collections |
| 193 | +- No additional overhead for non-Result objects |
| 194 | +- Unwrapping happens during expression evaluation (already existing cost) |
| 195 | + |
| 196 | +## Recommendations |
| 197 | + |
| 198 | +### Monitoring |
| 199 | +- Monitor CREATE EDGE statements in production |
| 200 | +- Watch for any edge cases with complex property access patterns |
| 201 | +- Verify performance remains consistent |
| 202 | + |
| 203 | +### Future Improvements |
| 204 | +1. Consider adding more edge cases to test suite: |
| 205 | + - Nested property access (e.g., `$x.property.subproperty`) |
| 206 | + - Multiple property access in single query |
| 207 | + - Property access on different Result types (projection vs element) |
| 208 | + |
| 209 | +2. Documentation updates: |
| 210 | + - Add examples of property access with CREATE EDGE to SQL documentation |
| 211 | + - Document the difference between `(SELECT).property` and pre-evaluated variables |
| 212 | + |
| 213 | +### Testing |
| 214 | +- ✅ Comprehensive test coverage added |
| 215 | +- ✅ All scenarios from issue #3315 tested |
| 216 | +- ✅ No regression in existing functionality |
| 217 | +- ✅ Ready for code review and merge |
0 commit comments