feat: add support for DISTINCT clause to array_agg()

[martin-georgiev]

Summary by CodeRabbit

• New Features

  • Enhanced SQL aggregation capabilities with improved support for distinct and order functionalities, including additional parameter handling.

• Refactor

  • Reorganized query parsing and aggregation logic for cleaner, more modular processing of distinct and ordering clauses.

• Tests

  • Expanded test scenarios and updated expected outputs to cover DISTINCT and ordering cases, ensuring clarity and reliability.

[coderabbitai]

Walkthrough

This pull request refactors the aggregation functions ArrayAgg and StringAgg by changing their inheritance from BaseOrderableFunction to BaseFunction. Both classes now utilize the OrderableTrait and a newly introduced DistinctableTrait to handle ORDER BY and DISTINCT clauses. Their parsing methods have been made public and extended to call trait-based methods (parseDistinctClause and parseOrderByClause). Additionally, the generated SQL is updated to include optional DISTINCT clauses. Test files have been reorganized into associative arrays with descriptive keys and now include scenarios with DISTINCT, concatenation, and ordering.

Changes

File(s)	Summary
src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/ArrayAgg.php src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/StringAgg.php	Updated class inheritance from BaseOrderableFunction to BaseFunction; incorporated OrderableTrait and DistinctableTrait; changed the visibility of the parse methods to public; updated SQL generation to integrate DISTINCT and ORDER BY clauses.
src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Traits/DistinctableTrait.php	Introduced a new trait to manage DISTINCT clause logic, including a boolean flag and methods to parse and optionally append the DISTINCT keyword.
src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Traits/OrderableTrait.php	Converted BaseOrderableFunction into OrderableTrait; renamed the parse method to parseOrderByClause and simplified its implementation by removing obsolete lines.
tests/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/ArrayAggTest.php tests/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/StringAggTest.php	Reorganized the expected SQL and DQL statements into key-value pairs with descriptive keys; added new test cases covering DISTINCT, concatenation, and ordering scenarios.

Sequence Diagram(s)

sequenceDiagram
    participant P as Parser
    participant F as ArrayAgg/StringAgg (Function)
    participant D as DistinctableTrait
    participant O as OrderableTrait

    P->>F: parse(Parser)
    F->>D: parseDistinctClause(Parser)
    D-->>F: Set isDistinct flag
    F->>O: parseOrderByClause(Parser)
    O-->>F: Process ORDER BY clause
    F->>F: Customize function parameters
    F->>P: Return parsed structure with DISTINCT & ORDER BY info

Loading

Possibly related PRs

• fix: add support for Lexer v1 (allowed by ORM < v2.15) #300: Enhances aggregation functions with similar trait-based distinct and order parsing, aligning with the changes in this PR.
• feat: add support for ORDER BY clause for array_agg() #267: Focuses on modifications in ArrayAgg regarding ORDER BY clause handling and parse function structure.
• chore: test the BaseType and BaseFunction abstractions #309: Addresses related test adjustments in the BaseFunctionTest class, complementing the functionality improvements made here.

Poem

Oh, I’m a rabbit in a code-filled glen,
Hopping through changes again and again,
With traits and methods dancing in my view,
DISTINCT and ORDER now shine bright and true,
I nibble on code like a carrot so sweet—celebration complete!
🐰🥕

---

📜 Recent review details

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 556df87 and f15012a.

📒 Files selected for processing (6)

• src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/ArrayAgg.php (2 hunks)
• src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/StringAgg.php (2 hunks)
• src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Traits/DistinctableTrait.php (1 hunks)
• src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Traits/OrderableTrait.php (2 hunks)
• tests/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/ArrayAggTest.php (1 hunks)
• tests/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/StringAggTest.php (1 hunks)

🧰 Additional context used

📓 Path-based instructions (1)

`tests/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/**/*.php`: Use the PostgreSQL official documentation to verify that tests include comprehensive use cases and example SQL que...

tests/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/**/*.php: Use the PostgreSQL official documentation to verify that tests include comprehensive use cases and example SQL queries for the tested SQL functions and operators.

• tests/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/ArrayAggTest.php
• tests/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/StringAggTest.php

🧠 Learnings (1)

src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/ArrayAgg.php (1)

Learnt from: martin-georgiev
PR: martin-georgiev/postgresql-for-doctrine#267
File: src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/ArrayAgg.php:30-37
Timestamp: 2025-03-26T16:52:34.228Z
Learning: In Doctrine ORM, the `OrderByClause->dispatch()` method already includes the complete "ORDER BY" phrase with proper spacing when converting DQL to SQL. No additional spacing is needed when using this method's output in SQL string construction.

🧬 Code Definitions (4)

tests/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/ArrayAggTest.php (1)

tests/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/StringAggTest.php (1)

• getDqlStatements (35-49)

tests/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/StringAggTest.php (1)

tests/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/ArrayAggTest.php (1)

• getDqlStatements (35-49)

src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/StringAgg.php (4)

src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/BaseFunction.php (6)

• BaseFunction (20-105)
• customizeFunction (43-46)
• setFunctionPrototype (57-60)
• addNodeMapping (62-65)
• parse (67-77)
• getSql (96-104)

src/MartinGeorgiev/Utils/DoctrineOrm.php (2)

• DoctrineOrm (12-18)
• isPre219 (14-17)

src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Traits/DistinctableTrait.php (2)

• parseDistinctClause (16-25)
• getOptionalDistinctClause (27-30)

src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Traits/OrderableTrait.php (1)

• parseOrderByClause (21-29)

src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/ArrayAgg.php (2)

src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Traits/DistinctableTrait.php (2)

• parseDistinctClause (16-25)
• getOptionalDistinctClause (27-30)

src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Traits/OrderableTrait.php (1)

• parseOrderByClause (21-29)

⏰ Context from checks skipped due to timeout of 90000ms (7)

• GitHub Check: PHP 8.4 + Doctrine ORM latest + Doctrine Lexer latest
• GitHub Check: PHP 8.4 + Doctrine ORM 2.14 + Doctrine Lexer latest
• GitHub Check: PHP 8.3 + Doctrine ORM latest + Doctrine Lexer 2.1
• GitHub Check: PHP 8.4 + Doctrine ORM 2.18 + Doctrine Lexer 3.0
• GitHub Check: PHP 8.4 + Doctrine ORM latest + Doctrine Lexer 2.1
• GitHub Check: PHP 8.3 + Doctrine ORM 2.18 + Doctrine Lexer 2.1
• GitHub Check: sloth

🔇 Additional comments (21)

tests/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/StringAggTest.php (2)

22-32: Well-structured test cases with comprehensive coverage

The reorganization of expected SQL statements into an associative array with descriptive keys greatly improves readability and maintainability. The test cases appropriately cover:

1. Basic usage
2. Concatenation
3. DISTINCT clause usage
4. ORDER BY clauses in different combinations
5. Multiple sorting columns
6. Custom delimiters

This approach aligns with PostgreSQL's documentation for the string_agg function, which supports all these scenarios.

---

38-48: Complete test coverage for DQL statements matching PostgreSQL functionality

The DQL statements appropriately mirror the expected SQL statements, ensuring that all PostgreSQL string_agg features are properly tested:

1. DISTINCT clause is properly represented in different contexts
2. ORDER BY with both ASC and DESC directions
3. Combination scenarios (DISTINCT + concatenation + ORDER BY)
4. Custom delimiter support

This comprehensive coverage ensures that the function's implementation matches PostgreSQL's capabilities.

tests/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/ArrayAggTest.php (2)

22-31: Good test coverage for array_agg with DISTINCT support

The test cases properly cover all PostgreSQL array_agg functionality including:

1. Basic aggregation
2. String concatenation within aggregation
3. DISTINCT clause in various contexts
4. ORDER BY with both ASC and DESC sorting
5. Combined scenarios with DISTINCT, concatenation and ordering
6. Multiple ORDER BY columns

This aligns with PostgreSQL's documentation for the array_agg function and properly tests the newly added DISTINCT support.

---

38-47: Well-structured DQL statements matching expected SQL output

The DQL statements properly mirror all test cases in the expected SQL output, with clear descriptive keys. The implementation correctly tests:

1. The DISTINCT clause in various contexts
2. Combinations of DISTINCT with ORDER BY and concatenation
3. Multiple column sorting

This provides thorough test coverage for the new DISTINCT feature being added.

src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Traits/DistinctableTrait.php (2)

12-31: Well-implemented trait for DISTINCT clause handling

The new DistinctableTrait provides a clean implementation for DISTINCT clause support:

1. The parseDistinctClause method correctly handles different Doctrine ORM versions by checking for the appropriate token type
2. The getOptionalDistinctClause method elegantly returns the DISTINCT keyword only when needed
3. The implementation correctly maintains state with the $isDistinct property

This trait design follows good practices for code reusability and separation of concerns, allowing DISTINCT functionality to be shared across different aggregate functions.

---

21-25:

✅ Verification successful

Ensure version compatibility is properly tested

The compatibility check for pre-2.19 versions of Doctrine ORM is important for maintaining backward compatibility.

---

🏁 Script executed:

#!/bin/bash
# Check if the DoctrineOrm::isPre219() method is properly implemented and used consistently
rg -A 3 "isPre219" --type php

Length of output: 3445

---

Compatibility check implementation verified; no changes required.

The usage of DoctrineOrm::isPre219() in this file is consistent with its implementation across the repository. The conditional handling—choosing between Lexer::T_DISTINCT and TokenType::T_DISTINCT based on the ORM version—is applied uniformly in similar traits and functions (e.g., in OrderableTrait.php and others). This confirms that the compatibility check for pre-2.19 versions of Doctrine ORM is properly implemented and tested.

src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Traits/OrderableTrait.php (3)

5-5: Appropriate namespace for trait-based approach

The namespace change to MartinGeorgiev\Doctrine\ORM\Query\AST\Functions\Traits follows good practices for organizing traits separately from classes.

---

15-15: Good refactoring from class to trait

Converting from BaseOrderableFunction class to OrderableTrait trait improves code composition flexibility, allowing multiple inheritance of behavior through trait usage rather than class inheritance.

---

21-29: Simplified parsing logic with improved visibility

The refactored parseOrderByClause method:

1. Appropriately changes visibility from public to protected as this is an internal implementation detail
2. Correctly maintains version compatibility with Doctrine ORM
3. Simplifies the parsing logic by removing unnecessary calls

This approach makes the code more maintainable and aligns with the trait-based design pattern being implemented.

src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/ArrayAgg.php (6)

7-13: All new imports appear correct.
These imports align with the new usage of DistinctableTrait and OrderableTrait, as well as referencing TokenType and DoctrineOrm for version checks. No issues identified.

---

23-23: Inheritance change to BaseFunction is consistent.
Switching from BaseOrderableFunction to BaseFunction is aligned with adopting a trait-based approach for DISTINCT and ORDER BY handling.

---

25-26: Traits usage is valid.
Using OrderableTrait and DistinctableTrait centralizes and standardizes the logic for parsing ORDER BY and DISTINCT clauses.

---

30-31: Function prototype updated for optional DISTINCT and ordering.
The refactored prototype array_agg(%s%s%s) and the addition of the StringPrimary mapping look correct for supporting distinct and ordered arguments.

---

54-54: Optional DISTINCT clause included in final SQL generation.
Appending $this->getOptionalDistinctClause() to the dispatched arguments is valid, and the spacing is properly handled by the trailing space in the return string.

---

34-49:

✅ Verification successful

Enhanced parse method for DISTINCT and ORDER BY clauses.
Using parseDistinctClause() and parseOrderByClause() ensures robust handling of advanced SQL syntax. The usage of $parser->match() for both Doctrine version scenarios is correct, and calling $this->customizeFunction() is consistent with the BaseFunction pattern.

To ensure comprehensive coverage of these new features, please verify that you have test scenarios exercising the DISTINCT usage in array_agg.

---

🏁 Script executed:

#!/bin/bash
# Check if test coverage includes DISTINCT usage in array_agg
rg -A 3 --replace "" "ArrayAgg" tests

Length of output: 946

---

I'll run a command to search for "DISTINCT" usage in the ArrayAggTest.php file to verify that DISTINCT scenarios are indeed covered.

---

🏁 Script executed:

#!/bin/bash
rg -n "DISTINCT" tests/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/ArrayAggTest.php

Length of output: 1229

---

Verified DISTINCT test coverage for array_agg.
Your updated parse method correctly integrates DISTINCT and ORDER BY functionality, and the test cases in tests/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/ArrayAggTest.php confirm that DISTINCT scenarios are well exercised.

src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/StringAgg.php (6)

12-13: New import statements are valid.
The DistinctableTrait and OrderableTrait references integrate well with the updated approach for handling DISTINCT and ORDER BY in StringAgg.

---

24-24: Switch to BaseFunction aligns with the trait-based approach.
Replacing BaseOrderableFunction with BaseFunction keeps consistency with the newly introduced traits for distinct and ordering functionality.

---

26-27: OrderableTrait and DistinctableTrait usage.
Adopting these traits ensures modular and reusable logic for parsing and generating DISTINCT and ORDER BY clauses.

---

34-35: Updated function prototype supports optional DISTINCT.
string_agg(%s%s, %s%s) and mapping two StringPrimary nodes for the main expression and the delimiter is correct for this aggregator.

---

61-61: Distinct clause integrated into SQL generation.
Including $this->getOptionalDistinctClause() in the final dispatched array ensures DISTINCT is properly inserted in the resulting query when required.

---

38-55:

❓ Verification inconclusive

Enhanced parse method with DISTINCT and ORDER BY parsing.
The method correctly handles older Doctrine versions, calls parseDistinctClause() for optional DISTINCT, obtains the expression and delimiter, and then calls parseOrderByClause() for ordering. Matching parentheses and commas align with the aggregator syntax.

Please verify tests cover the DISTINCT usage with string_agg to ensure correctness for real-world queries.

---

🏁 Script executed:

#!/bin/bash
# Check if test coverage includes DISTINCT usage in string_agg
rg -A 3 --replace "" "StringAgg" tests

Length of output: 958

---

Attention: Verify DISTINCT Test Coverage in StringAgg Parsing

The updated parse method is well-structured—it correctly adapts to both older and newer Doctrine versions by conditionally using the appropriate lexer/token types and properly delegates to parseDistinctClause(), expression parsing, and parseOrderByClause(). However, while the test file (tests/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/StringAggTest.php) exists and covers basic STRING_AGG` registration, the grep results don’t clearly show explicit test scenarios for DISTINCT usage. Please verify (or add) tests that exercise the DISTINCT branch to ensure robust behavior in real-world queries.

✨ Finishing Touches

• 📝 Generate Docstrings

---

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

• X
• Mastodon
• Reddit
• LinkedIn

🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

• Review comments: Directly reply to a review comment made by CodeRabbit. Example:
  • I pushed a fix in commit <commit_id>, please review it.
  • Generate unit testing code for this file.
  • Open a follow-up GitHub issue for this discussion.
• Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
  • @coderabbitai generate unit testing code for this file.
  • @coderabbitai modularize this function.
• PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
  • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
  • @coderabbitai read src/utils.ts and generate unit testing code.
  • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
  • @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

• @coderabbitai pause to pause the reviews on a PR.
• @coderabbitai resume to resume the paused reviews.
• @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
• @coderabbitai full review to do a full review from scratch and review all the files again.
• @coderabbitai summary to regenerate the summary of the PR.
• @coderabbitai generate docstrings to generate docstrings for this PR.
• @coderabbitai resolve resolve all the CodeRabbit review comments.
• @coderabbitai plan to trigger planning for file edits and PR creation.
• @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
• @coderabbitai help to get help.

Other keywords and placeholders

• Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
• Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
• Add @coderabbitai anywhere in the PR title to generate the title automatically.

Documentation and Community

• Visit our Documentation for detailed information on how to use CodeRabbit.
• Join our Discord Community to get help, request features, and share feedback.
• Follow us on X/Twitter for updates and announcements.