fix: ORDER BY on aliased RETURN DISTINCT property no longer raises

[DecisionNerd]

Closes #481

Summary

• RETURN DISTINCT a.year AS year ORDER BY a.year raised UndefinedVariable because the planner's projected_prop_paths set excluded aliased PropertyAccess items (guarded by not item.alias)
• One-line fix: remove that guard so any PropertyAccess in RETURN DISTINCT is tracked, aliased or not
• ORDER BY a.age on a completely unprojected property still raises correctly

Test plan

• test_distinct_return_aliased_property_allows_same_path_in_order_by — was failing, now passes
• test_distinct_return_aliased_property_disallows_other_path_in_order_by — still raises for truly unprojected property
• test_order_by_aliased_property_works — integration: dedup + correct sort order
• test_order_by_aliased_property_desc — DESC variant
• All existing TestReturnDistinctOrderBy tests still pass
• make pre-push green (87.07% total coverage)

🤖 Generated with Claude Code

---

^{Need help on this PR? Tag @codesmith with what you need.}

• Let Codesmith autofix CI failures and bot reviews

Summary by CodeRabbit

• Bug Fixes

  • Fixes validation so RETURN DISTINCT queries can be ordered by aliased return properties when the underlying property path matches.

• Tests

  • Added integration and unit tests covering allowed and disallowed ORDER BY cases involving aliased RETURN properties to prevent regressions.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 12ed036d-5eb3-4098-a1e0-f0aa5c9c646f

📥 Commits

Reviewing files that changed from the base of the PR and between b40d456 and 6e94e55.

📒 Files selected for processing (1)

• tests/integration/test_distinct_clause.py

---

Walkthrough

Adjusted RETURN DISTINCT validation so projected property paths include PropertyAccess expressions even when the RETURN item has an alias; tests added to cover valid and invalid ORDER BY usages with aliased properties.

Changes

RETURN DISTINCT + ORDER BY Validation Fix

Layer / File(s)	Summary
Core Logic src/graphforge/planner/planner.py	Removed the not item.alias guard when collecting projected_prop_paths for PropertyAccess expressions in the RETURN DISTINCT (non-aggregate) branch.
Unit Tests tests/unit/planner/test_syntax_error_validation.py	Added tests: one allowing ORDER BY the same property path used in a RETURN DISTINCT alias, and one asserting ORDER BY of a different path raises SyntaxError/UndefinedVariable.
Integration Tests tests/integration/test_distinct_clause.py	Added end-to-end parameterized test verifying ORDER BY works when it references the same property path as a RETURN DISTINCT alias (ASC and DESC cases).

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

• DecisionNerd/graphforge#374: Modifies RETURN validation logic in src/graphforge/planner/planner.py.
• DecisionNerd/graphforge#329: Related changes touching RETURN/ORDER BY handling in the planner.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately describes the fix: removing validation that incorrectly raised errors for ORDER BY on aliased RETURN DISTINCT properties.
Description check	✅ Passed	The description provides clear context (issue #481), root cause, one-line fix, and comprehensive test plan covering both acceptance criteria and regression scenarios.
Linked Issues check	✅ Passed	The changes directly address all acceptance criteria: ORDER BY now works with aliased properties [481], aliases continue working, ORDER BY on unprojected properties still raises correctly, and existing tests pass.
Out of Scope Changes check	✅ Passed	All changes are scoped to fixing issue #481: one-line planner fix and three test files covering unit/integration validation with no extraneous modifications.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/481-order-by-return-distinct

---

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

[codecov]

Codecov Report

❌ Patch coverage is 0% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 88.01%. Comparing base (e5f5945) to head (6e94e55).
✅ All tests successful. No failed tests found.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #494      +/-   ##
==========================================
- Coverage   88.01%   88.01%   -0.01%     
==========================================
  Files          40       40              
  Lines       14449    14449              
  Branches     3430     3430              
==========================================
- Hits        12718    12717       -1     
  Misses       1141     1141              
- Partials      590      591       +1     

Flag	Coverage Δ
full-coverage	88.01% <0.00%> (-0.01%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Components	Coverage Δ
parser	95.67% <ø> (ø)
planner	82.89% <0.00%> (-0.05%)	⬇️
executor	83.63% <ø> (ø)
storage	91.25% <ø> (ø)
ast	98.20% <ø> (ø)
types	94.75% <ø> (ø)

---

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e5f5945...6e94e55. Read the comment docs.

[coderabbitai]

🧹 Nitpick comments (1)

tests/integration/test_distinct_clause.py (1)

146-178: ⚡ Quick win

Parametrize the ASC/DESC aliased-property ORDER BY integration cases.

These two tests exercise the same behavior with different inputs/directions; @pytest.mark.parametrize would reduce duplication and make expansions easier.

♻️ Suggested refactor shape

 class TestReturnDistinctOrderByAliasedProperty:
-    def test_order_by_aliased_property_works(self):
-        ...
-    def test_order_by_aliased_property_desc(self):
-        ...
+    `@pytest.mark.parametrize`(
+        ("create_query", "run_query", "key", "expected"),
+        [
+            (
+                """
+                CREATE (:Item {name: 'C', year: 2023}),
+                       (:Item {name: 'A', year: 2021}),
+                       (:Item {name: 'A', year: 2021}),
+                       (:Item {name: 'B', year: 2022})
+                """,
+                """
+                MATCH (a:Item)
+                RETURN DISTINCT a.year AS year
+                ORDER BY a.year ASC
+                """,
+                "year",
+                [2021, 2022, 2023],
+            ),
+            (
+                """
+                CREATE (:Person {name: 'Alice'}),
+                       (:Person {name: 'Bob'}),
+                       (:Person {name: 'Alice'})
+                """,
+                """
+                MATCH (p:Person)
+                RETURN DISTINCT p.name AS name
+                ORDER BY p.name DESC
+                """,
+                "name",
+                ["Bob", "Alice"],
+            ),
+        ],
+    )
+    def test_order_by_aliased_property(self, create_query, run_query, key, expected):
+        gf = GraphForge()
+        gf.execute(create_query)
+        results = gf.execute(run_query)
+        assert [r[key].value for r in results] == expected

As per coding guidelines, tests/**/*.py: "Use pytest parametrization (@pytest.mark.parametrize) when testing the same logic with different inputs to avoid code duplication".

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/integration/test_distinct_clause.py` around lines 146 - 178, Replace
the two nearly identical tests test_order_by_aliased_property_works and
test_order_by_aliased_property_desc with a single parametrized test using
pytest.mark.parametrize that iterates over ORDER BY directions (e.g., "ASC" and
"DESC") and expected outputs; inside the new test (keep using GraphForge and
gf.execute to create the same fixtures), build the query string with the
direction parameter, execute it, and assert the result length and the ordered
values (for years or names) match the expected list per parameter set; reference
the existing test names (test_order_by_aliased_property_works,
test_order_by_aliased_property_desc) and the RETURN DISTINCT ... AS usage when
locating where to consolidate and assert.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@tests/integration/test_distinct_clause.py`:
- Around line 146-178: Replace the two nearly identical tests
test_order_by_aliased_property_works and test_order_by_aliased_property_desc
with a single parametrized test using pytest.mark.parametrize that iterates over
ORDER BY directions (e.g., "ASC" and "DESC") and expected outputs; inside the
new test (keep using GraphForge and gf.execute to create the same fixtures),
build the query string with the direction parameter, execute it, and assert the
result length and the ordered values (for years or names) match the expected
list per parameter set; reference the existing test names
(test_order_by_aliased_property_works, test_order_by_aliased_property_desc) and
the RETURN DISTINCT ... AS usage when locating where to consolidate and assert.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 3e5e49e8-4b63-42bf-b35a-e7fe5c6a8b3e

📥 Commits

Reviewing files that changed from the base of the PR and between e5f5945 and b40d456.

📒 Files selected for processing (3)

• src/graphforge/planner/planner.py
• tests/integration/test_distinct_clause.py
• tests/unit/planner/test_syntax_error_validation.py