Skip to content

Conversation

@janklan
Copy link
Contributor

@janklan janklan commented May 7, 2025

I'm really missing the ability to create a range with non-default bounds, so here is a PR that should add it.

See https://www.postgresql.org/docs/17/rangetypes.html#RANGETYPES-CONSTRUCT for PostgreSQL details.

TL;DR: We can now pass the third argument to the range functions.

Summary by CodeRabbit

  • Documentation

    • Updated documentation to include details about supported PostgreSQL range functions and types.
  • New Features

    • Enhanced support for PostgreSQL range functions, including arithmetic and date/time range types, with improved handling of optional range bounds.
  • Tests

    • Expanded test coverage for all supported range functions to verify correct behavior with and without optional bounds parameters.

@coderabbitai
Copy link

coderabbitai bot commented May 7, 2025

Walkthrough

A new abstract class, BaseRangeFunction, was introduced to standardize the implementation of PostgreSQL range functions in Doctrine ORM. Existing range function classes were refactored to inherit from this base class, simplifying their structure. Corresponding tests and documentation were updated to reflect the new structure and to cover additional range function argument scenarios.

Changes

File(s) Change Summary
README.md Added a "Range functions" subsection under "Functions" listing supported PostgreSQL range types.
src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/BaseRangeFunction.php Introduced new abstract class BaseRangeFunction extending BaseVariadicFunction for range-type AST functions.
src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Daterange.php
src/.../Int4range.php
src/.../Int8range.php
src/.../Numrange.php
src/.../Tsrange.php
src/.../Tstzrange.php
Refactored each range function to extend BaseRangeFunction instead of BaseFunction. Replaced customizeFunction() with getFunctionName() and, where needed, getNodeMappingPattern().
tests/Unit/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Int4rangeTest.php
tests/.../Int8rangeTest.php
tests/.../NumrangeTest.php
tests/.../TsrangeTest.php
tests/.../TstzrangeTest.php
Extended test cases to include scenarios with a third argument specifying range bounds for each range function.

Sequence Diagram(s)

sequenceDiagram
    participant DQL_User
    participant Doctrine
    participant BaseRangeFunction
    participant PostgreSQL

    DQL_User->>Doctrine: Write DQL using range function (e.g., INT4RANGE(a, b, '[)')
    Doctrine->>BaseRangeFunction: Parse and map function call
    BaseRangeFunction->>PostgreSQL: Generate SQL (e.g., int4range(a, b, '[)'))
    PostgreSQL-->>BaseRangeFunction: Return query result
    BaseRangeFunction-->>Doctrine: Pass result
    Doctrine-->>DQL_User: Return final result
Loading

Possibly related PRs

Poem

In the fields of code where ranges grow,
A rabbit hops where functions flow.
With a base for all, the bounds are set,
From dates to numbers, none forget.
Tests abound, the docs now sing—
Range functions unified, let queries spring!
🐇✨

Note

⚡️ AI Code Reviews for VS Code, Cursor, Windsurf

CodeRabbit now has a plugin for VS Code, Cursor and Windsurf. This brings AI code reviews directly in the code editor. Each commit is reviewed immediately, finding bugs before the PR is raised. Seamless context handoff to your AI code agent ensures that you can easily incorporate review feedback.
Learn more here.


Note

⚡️ Faster reviews with caching

CodeRabbit now supports caching for code and dependencies, helping speed up reviews. This means quicker feedback, reduced wait times, and a smoother review experience overall. Cached data is encrypted and stored securely. This feature will be automatically enabled for all accounts on May 16th. To opt out, configure Review - Disable Cache at either the organization or repository level. If you prefer to disable all data retention across your organization, simply turn off the Data Retention setting under your Organization Settings.
Enjoy the performance boost—your workflow just got faster.


📜 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 e34f60c and 532f94d.

📒 Files selected for processing (13)
  • README.md (1 hunks)
  • src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/BaseRangeFunction.php (1 hunks)
  • src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Daterange.php (1 hunks)
  • src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Int4range.php (1 hunks)
  • src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Int8range.php (1 hunks)
  • src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Numrange.php (1 hunks)
  • src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Tsrange.php (1 hunks)
  • src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Tstzrange.php (1 hunks)
  • tests/Unit/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Int4rangeTest.php (1 hunks)
  • tests/Unit/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Int8rangeTest.php (1 hunks)
  • tests/Unit/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/NumrangeTest.php (1 hunks)
  • tests/Unit/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/TsrangeTest.php (1 hunks)
  • tests/Unit/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/TstzrangeTest.php (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (13)
  • README.md
  • tests/Unit/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Int8rangeTest.php
  • tests/Unit/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/NumrangeTest.php
  • tests/Unit/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Int4rangeTest.php
  • src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Tsrange.php
  • tests/Unit/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/TstzrangeTest.php
  • src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Daterange.php
  • src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Int4range.php
  • src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/BaseRangeFunction.php
  • tests/Unit/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/TsrangeTest.php
  • src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Numrange.php
  • src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Int8range.php
  • src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Tstzrange.php
⏰ Context from checks skipped due to timeout of 90000ms (20)
  • GitHub Check: PHP 8.4 + Doctrine ORM 3.0 + Doctrine Lexer 3.0
  • GitHub Check: PHP 8.4 + Doctrine ORM latest + Doctrine Lexer latest
  • GitHub Check: PHP 8.4 + Doctrine ORM 2.18 + Doctrine Lexer latest
  • GitHub Check: PHP 8.4 + Doctrine ORM 2.18 + Doctrine Lexer 2.1
  • GitHub Check: PHP 8.1 + Doctrine ORM 2.14 + Doctrine Lexer 1.2
  • GitHub Check: PHP 8.4 + Doctrine ORM 3.0 + Doctrine Lexer latest
  • GitHub Check: PHP 8.3 + Doctrine ORM 2.18 + Doctrine Lexer 3.0
  • GitHub Check: PHP 8.3 + Doctrine ORM latest + Doctrine Lexer 2.1
  • GitHub Check: PHP 8.3 + Doctrine ORM 2.18 + Doctrine Lexer latest
  • GitHub Check: PHP 8.3 + Doctrine ORM 2.14 + Doctrine Lexer latest
  • GitHub Check: PHP 8.3 + Doctrine ORM 3.0 + Doctrine Lexer 3.0
  • GitHub Check: PHP 8.3 + Doctrine ORM latest + Doctrine Lexer 3.0
  • GitHub Check: PHP 8.2 + Doctrine ORM latest + Doctrine Lexer 2.1
  • GitHub Check: PHP 8.3 + Doctrine ORM 2.14 + Doctrine Lexer 2.1
  • GitHub Check: PHP 8.2 + Doctrine ORM 2.18 + Doctrine Lexer 3.0
  • GitHub Check: PHP 8.2 + Doctrine ORM 2.14 + Doctrine Lexer 2.1
  • GitHub Check: PHP 8.1 + Doctrine ORM 2.14 + Doctrine Lexer latest
  • GitHub Check: PHP 8.1 + Doctrine ORM latest + Doctrine Lexer latest
  • GitHub Check: PHP 8.1 + Doctrine ORM 2.18 + Doctrine Lexer 3.0
  • GitHub Check: PHP 8.1 + Doctrine ORM latest + Doctrine Lexer 2.1
✨ Finishing Touches
  • 📝 Generate Docstrings
🧪 Generate Unit Tests
  • Create PR with Unit Tests
  • Commit Unit Tests in branch feat/range-bounds
  • Post Copyable Unit Tests in Comment

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
🪧 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.
    • Explain this complex logic.
    • 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 explain this code block.
    • @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 explain its main purpose.
    • @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.

Support

Need help? Create a ticket on our support page for assistance with any issues or questions.

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 generate sequence diagram to generate a sequence diagram of the changes in this PR.
  • @coderabbitai generate unit tests to generate unit tests for this PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @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.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (6)
tests/Unit/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/TsrangeTest.php (1)

21-32: Use a non-default bounds literal to exercise the new feature.

'[)' is PostgreSQL’s default bounds, so the two‐argument and three-argument variants currently generate identical SQL semantics.
To genuinely verify that the third argument is parsed and propagated, use a different bounds literal such as '()', '[]', or '(]'.

-            'SELECT tsrange(c0_.datetime1, c0_.datetime2, \'[)\') AS sclr_0 FROM ContainsDates c0_',
+            'SELECT tsrange(c0_.datetime1, c0_.datetime2, \'()\' ) AS sclr_0 FROM ContainsDates c0_',
...
-            \sprintf('SELECT TSRANGE(e.datetime1, e.datetime2, \'[)\') FROM %s e', ContainsDates::class),
+            \sprintf('SELECT TSRANGE(e.datetime1, e.datetime2, \'()\' ) FROM %s e', ContainsDates::class),

This will fail if the bounds argument is not wired correctly, giving the test real discriminative power.

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

15-20: Bounds-string inputs like 'infinity' won’t match the default node pattern

BaseRangeFunction expects the first two arguments to be ArithmeticPrimary.
PostgreSQL allows numeric ranges to use textual literals such as 'infinity'/'-infinity'.
Those literals are parsed by Doctrine as StringPrimary, which means a DQL such as:

NUMRANGE('infinity', 10)

would throw a QueryException because the node mapping pattern rejects the first argument.

If supporting these literals is desirable (they are frequently used when the upper or lower
bound is open-ended), consider overriding getNodeMappingPattern() here so that the first
two positions accept either ArithmeticPrimary or StringPrimary:

protected function getNodeMappingPattern(): array
{
-    return parent::getNodeMappingPattern();
+    return [
+        'ArithmeticPrimary|StringPrimary',
+        'ArithmeticPrimary|StringPrimary',
+        'StringPrimary',
+    ];
}

No change is needed if the project explicitly forbids the textual form, but please double-check
the intended coverage.

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

21-24: Nice coverage – consider parametrising more bound variants

Adding the '[)' case strengthens the test suite 🎉.
Because PostgreSQL accepts four bound specifications ('[)', '[ ]', '( ]', '( )'),
you may squeeze even more value from this test by parametrising the bounds argument and
asserting the translation for all four possibilities. That keeps behaviour tight if a
future refactor accidentally hard-codes a single default.

Example outline:

/** @dataProvider boundsProvider */
public function testTstzrangeWithBounds(string $bounds)
{
    $dql = sprintf(
        'SELECT TSTZRANGE(e.datetimetz1, e.datetimetz2, %s) FROM %s e',
        $bounds,
        ContainsDates::class
    );
    $this->assertDqlProducesSql($dql);
}

public static function boundsProvider(): array
{
    return ["'[)'", "'[]'", "'(]'", "'()'"];
}

Also applies to: 29-32

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

22-28: Consider reducing duplication in node-mapping overrides

Every range function whose elements are expressed as strings ( tsrange, tstzrange, most use–cases for daterange ) now has to fully override getNodeMappingPattern() just to swap ArithmeticPrimaryStringPrimary for the first two arguments. This is a bit of boiler-plate and risks drift if the default pattern ever changes.

Two possible follow-ups:

-abstract class BaseRangeFunction extends BaseVariadicFunction
+abstract class BaseRangeFunction extends BaseVariadicFunction
+{
+    /** Override in children when the first two args are not ArithmeticPrimary */
+    protected function getElementNodeType(): string
+    {
+        return 'ArithmeticPrimary';
+    }
+
+    protected function getNodeMappingPattern(): array
+    {
+        $element = $this->getElementNodeType();
+        return [$element, $element, 'StringPrimary'];
+    }

Then Tsrange only needs:

protected function getElementNodeType(): string
{
    return 'StringPrimary';
}

This keeps the per-function classes minimal and centralises the mapping logic.

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

24-32: Bounds argument is fixed to StringPrimary – allow parameters?

At present the third argument can only be a literal node; a bound parameter such as :bounds would be rejected. If you foresee dynamic bounds (e.g. user-selectable inclusivity), consider adding 'StringPrimary' | 'InputParameter' to the mapping or documenting the constraint.

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

17-20: Minor nit – final newline

The file ends without a newline (POSIX convention). Adding it avoids diff noise in future edits.

📜 Review details

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

📥 Commits

Reviewing files that changed from the base of the PR and between bb6a6b6 and e34f60c.

📒 Files selected for processing (13)
  • README.md (1 hunks)
  • src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/BaseRangeFunction.php (1 hunks)
  • src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Daterange.php (1 hunks)
  • src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Int4range.php (1 hunks)
  • src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Int8range.php (1 hunks)
  • src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Numrange.php (1 hunks)
  • src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Tsrange.php (1 hunks)
  • src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Tstzrange.php (1 hunks)
  • tests/Unit/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Int4rangeTest.php (1 hunks)
  • tests/Unit/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Int8rangeTest.php (1 hunks)
  • tests/Unit/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/NumrangeTest.php (1 hunks)
  • tests/Unit/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/TsrangeTest.php (1 hunks)
  • tests/Unit/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/TstzrangeTest.php (1 hunks)
🧰 Additional context used
🧠 Learnings (6)
📓 Common learnings
Learnt from: martin-georgiev
PR: martin-georgiev/postgresql-for-doctrine#263
File: src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Numrange.php:19-21
Timestamp: 2025-03-11T12:32:10.726Z
Learning: In the postgresql-for-doctrine repository, PostgreSQL range functions have distinct implementations for different data types. The `Numrange` function works with numeric/decimal values and is tested using the `ContainsDecimals` fixture with properties typed as `float`. In contrast, the `Int4range` function works with 32-bit integers and is tested using the `ContainsIntegers` fixture with properties typed as `int`. While the PHP implementations share a similar structure (extending `BaseFunction`), they are semantically different as they handle different PostgreSQL data types.
tests/Unit/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Int8rangeTest.php (1)
Learnt from: martin-georgiev
PR: martin-georgiev/postgresql-for-doctrine#263
File: src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Numrange.php:19-21
Timestamp: 2025-03-11T12:32:10.726Z
Learning: In the postgresql-for-doctrine repository, PostgreSQL range functions have distinct implementations for different data types. The `Numrange` function works with numeric/decimal values and is tested using the `ContainsDecimals` fixture with properties typed as `float`. In contrast, the `Int4range` function works with 32-bit integers and is tested using the `ContainsIntegers` fixture with properties typed as `int`. While the PHP implementations share a similar structure (extending `BaseFunction`), they are semantically different as they handle different PostgreSQL data types.
src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/BaseRangeFunction.php (1)
Learnt from: martin-georgiev
PR: martin-georgiev/postgresql-for-doctrine#263
File: src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Numrange.php:19-21
Timestamp: 2025-03-11T12:32:10.726Z
Learning: In the postgresql-for-doctrine repository, PostgreSQL range functions have distinct implementations for different data types. The `Numrange` function works with numeric/decimal values and is tested using the `ContainsDecimals` fixture with properties typed as `float`. In contrast, the `Int4range` function works with 32-bit integers and is tested using the `ContainsIntegers` fixture with properties typed as `int`. While the PHP implementations share a similar structure (extending `BaseFunction`), they are semantically different as they handle different PostgreSQL data types.
tests/Unit/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Int4rangeTest.php (1)
Learnt from: martin-georgiev
PR: martin-georgiev/postgresql-for-doctrine#263
File: src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Numrange.php:19-21
Timestamp: 2025-03-11T12:32:10.726Z
Learning: In the postgresql-for-doctrine repository, PostgreSQL range functions have distinct implementations for different data types. The `Numrange` function works with numeric/decimal values and is tested using the `ContainsDecimals` fixture with properties typed as `float`. In contrast, the `Int4range` function works with 32-bit integers and is tested using the `ContainsIntegers` fixture with properties typed as `int`. While the PHP implementations share a similar structure (extending `BaseFunction`), they are semantically different as they handle different PostgreSQL data types.
src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Numrange.php (1)
Learnt from: martin-georgiev
PR: martin-georgiev/postgresql-for-doctrine#263
File: src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Numrange.php:19-21
Timestamp: 2025-03-11T12:32:10.726Z
Learning: In the postgresql-for-doctrine repository, PostgreSQL range functions have distinct implementations for different data types. The `Numrange` function works with numeric/decimal values and is tested using the `ContainsDecimals` fixture with properties typed as `float`. In contrast, the `Int4range` function works with 32-bit integers and is tested using the `ContainsIntegers` fixture with properties typed as `int`. While the PHP implementations share a similar structure (extending `BaseFunction`), they are semantically different as they handle different PostgreSQL data types.
tests/Unit/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/NumrangeTest.php (1)
Learnt from: martin-georgiev
PR: martin-georgiev/postgresql-for-doctrine#263
File: src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Numrange.php:19-21
Timestamp: 2025-03-11T12:32:10.726Z
Learning: In the postgresql-for-doctrine repository, PostgreSQL range functions have distinct implementations for different data types. The `Numrange` function works with numeric/decimal values and is tested using the `ContainsDecimals` fixture with properties typed as `float`. In contrast, the `Int4range` function works with 32-bit integers and is tested using the `ContainsIntegers` fixture with properties typed as `int`. While the PHP implementations share a similar structure (extending `BaseFunction`), they are semantically different as they handle different PostgreSQL data types.
🧬 Code Graph Analysis (10)
tests/Unit/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Int8rangeTest.php (4)
tests/Unit/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Int4rangeTest.php (1)
  • getDqlStatements (27-33)
tests/Unit/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/NumrangeTest.php (1)
  • getDqlStatements (27-33)
tests/Unit/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/TsrangeTest.php (1)
  • getDqlStatements (27-33)
tests/Unit/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/TstzrangeTest.php (1)
  • getDqlStatements (27-33)
tests/Unit/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Int4rangeTest.php (4)
tests/Unit/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Int8rangeTest.php (1)
  • getDqlStatements (27-33)
tests/Unit/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/NumrangeTest.php (1)
  • getDqlStatements (27-33)
tests/Unit/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/TsrangeTest.php (1)
  • getDqlStatements (27-33)
tests/Unit/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/TstzrangeTest.php (1)
  • getDqlStatements (27-33)
src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Daterange.php (6)
src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/BaseRangeFunction.php (1)
  • BaseRangeFunction (13-33)
src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Int8range.php (1)
  • getFunctionName (17-20)
src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Int4range.php (1)
  • getFunctionName (17-20)
src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Numrange.php (1)
  • getFunctionName (17-20)
src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Tsrange.php (1)
  • getFunctionName (17-20)
src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Tstzrange.php (1)
  • getFunctionName (17-20)
src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Int4range.php (6)
src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/BaseRangeFunction.php (1)
  • BaseRangeFunction (13-33)
src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Int8range.php (1)
  • getFunctionName (17-20)
src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Daterange.php (1)
  • getFunctionName (17-20)
src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Numrange.php (1)
  • getFunctionName (17-20)
src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Tsrange.php (1)
  • getFunctionName (17-20)
src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Tstzrange.php (1)
  • getFunctionName (17-20)
tests/Unit/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/TstzrangeTest.php (4)
tests/Unit/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Int8rangeTest.php (1)
  • getDqlStatements (27-33)
tests/Unit/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Int4rangeTest.php (1)
  • getDqlStatements (27-33)
tests/Unit/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/NumrangeTest.php (1)
  • getDqlStatements (27-33)
tests/Unit/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/TsrangeTest.php (1)
  • getDqlStatements (27-33)
tests/Unit/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/TsrangeTest.php (4)
tests/Unit/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Int8rangeTest.php (1)
  • getDqlStatements (27-33)
tests/Unit/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Int4rangeTest.php (1)
  • getDqlStatements (27-33)
tests/Unit/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/NumrangeTest.php (1)
  • getDqlStatements (27-33)
tests/Unit/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/TstzrangeTest.php (1)
  • getDqlStatements (27-33)
src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Numrange.php (6)
src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/BaseRangeFunction.php (1)
  • BaseRangeFunction (13-33)
src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Int8range.php (1)
  • getFunctionName (17-20)
src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Daterange.php (1)
  • getFunctionName (17-20)
src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Int4range.php (1)
  • getFunctionName (17-20)
src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Tsrange.php (1)
  • getFunctionName (17-20)
src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Tstzrange.php (1)
  • getFunctionName (17-20)
tests/Unit/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/NumrangeTest.php (4)
tests/Unit/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Int8rangeTest.php (1)
  • getDqlStatements (27-33)
tests/Unit/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Int4rangeTest.php (1)
  • getDqlStatements (27-33)
tests/Unit/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/TsrangeTest.php (1)
  • getDqlStatements (27-33)
tests/Unit/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/TstzrangeTest.php (1)
  • getDqlStatements (27-33)
src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Tstzrange.php (2)
src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/BaseRangeFunction.php (2)
  • BaseRangeFunction (13-33)
  • getNodeMappingPattern (15-22)
src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Tsrange.php (2)
  • getFunctionName (17-20)
  • getNodeMappingPattern (22-29)
src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Tsrange.php (6)
src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/BaseRangeFunction.php (2)
  • BaseRangeFunction (13-33)
  • getNodeMappingPattern (15-22)
src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Int8range.php (1)
  • getFunctionName (17-20)
src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Daterange.php (1)
  • getFunctionName (17-20)
src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Int4range.php (1)
  • getFunctionName (17-20)
src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Numrange.php (1)
  • getFunctionName (17-20)
src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Tstzrange.php (2)
  • getFunctionName (17-20)
  • getNodeMappingPattern (22-29)
⏰ Context from checks skipped due to timeout of 90000ms (16)
  • GitHub Check: PHP 8.4 + Doctrine ORM latest + Doctrine Lexer 2.1
  • GitHub Check: PHP 8.4 + Doctrine ORM 2.18 + Doctrine Lexer 3.0
  • GitHub Check: PHP 8.3 + Doctrine ORM latest + Doctrine Lexer 3.0
  • GitHub Check: PHP 8.3 + Doctrine ORM 2.18 + Doctrine Lexer 3.0
  • GitHub Check: PHP 8.4 + Doctrine ORM 3.0 + Doctrine Lexer 3.0
  • GitHub Check: PHP 8.4 + Doctrine ORM 2.18 + Doctrine Lexer 2.1
  • GitHub Check: PHP 8.3 + Doctrine ORM 3.0 + Doctrine Lexer 3.0
  • GitHub Check: PHP 8.3 + Doctrine ORM 2.14 + Doctrine Lexer 2.1
  • GitHub Check: PHP 8.2 + Doctrine ORM 2.18 + Doctrine Lexer latest
  • GitHub Check: PHP 8.2 + Doctrine ORM 3.0 + Doctrine Lexer 3.0
  • GitHub Check: PHP 8.2 + Doctrine ORM latest + Doctrine Lexer 2.1
  • GitHub Check: PHP 8.2 + Doctrine ORM 2.14 + Doctrine Lexer latest
  • GitHub Check: PHP 8.2 + Doctrine ORM 3.0 + Doctrine Lexer latest
  • GitHub Check: PHP 8.1 + Doctrine ORM 3.0 + Doctrine Lexer latest
  • GitHub Check: PHP 8.1 + Doctrine ORM 2.18 + Doctrine Lexer latest
  • GitHub Check: PHP 8.1 + Doctrine ORM 2.14 + Doctrine Lexer latest
🔇 Additional comments (11)
README.md (1)

73-75: Documentation addition for range functions looks good!

This addition nicely categorizes the PostgreSQL range functions supported by the library, making it clear to users which range types are available. The grouping into arithmetic ranges and date ranges is particularly helpful for navigation.

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

23-23: Implementation of range bound test looks good!

This test case correctly extends the coverage to include the optional third parameter for specifying range bounds as '[)' in the SQL output, which aligns with the PR objective of supporting custom range bounds.


31-31: DQL test with range bounds parameter is correctly implemented.

This test case properly demonstrates the usage of INT8RANGE with three parameters, where the third parameter '[)' specifies the bounds of the range. This matches the SQL expectation defined in the getExpectedSqlStatements method and ensures the function correctly handles the optional bounds parameter.

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

23-23: SQL test verification for NUMRANGE with bounds parameter looks good!

This test case correctly validates that the SQL output includes the third parameter for specifying range bounds as '[)', which is consistent with the implementation for other range functions.


31-31: DQL test with range bounds parameter is properly implemented.

This test case correctly demonstrates the usage of NUMRANGE with three parameters, with the third parameter specifying the bounds as '[)'. This is consistent with the approach used for other range functions and aligns with PostgreSQL's range type construction capabilities.

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

23-23: SQL test verification for INT4RANGE with bounds parameter looks good!

This test case correctly validates that the SQL output includes the third parameter for specifying range bounds as '[)', maintaining consistency with the implementation for other range functions.


31-31: DQL test with range bounds parameter is well implemented.

This test case properly demonstrates the usage of INT4RANGE with the optional third parameter for specifying bounds. The test uses the ContainsIntegers fixture appropriately, in line with the retrieved learning that Int4range works with 32-bit integers.

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

15-20: 👍 Refactor looks good.

Migrating to BaseRangeFunction removes boilerplate while keeping behaviour unchanged.

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

15-30: Implementation aligns perfectly with the new BaseRangeFunction abstraction

The class cleanly overrides only what differs from the default: the SQL name and the
StringPrimary mapping that timestamp-TZ expressions require. This keeps the surface
area minimal and consistent with sibling range functions – great work!

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

17-20: Function name override looks good

getFunctionName() cleanly delegates the literal to the parent class – concise and correct.

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

15-22: Validate that ArithmeticPrimary is always appropriate for the first two arguments

ArithmeticPrimary covers numeric literals/expressions, but not quoted date/time literals which Doctrine usually parses as StringPrimary. For daterange or when using bound parameters (:from, :to) this mapping might fail.

Because some child classes override the pattern (good), consider either:

  1. Documenting that children must override when their element type is not purely numeric, or
  2. Generalising the default pattern (see earlier suggestion in Tsrange.php).

This avoids subtle bugs where a developer assumes parameters will just work across all range functions.

@janklan janklan force-pushed the feat/range-bounds branch from e34f60c to 4099ba3 Compare May 7, 2025 02:17
coderabbitai[bot]
coderabbitai bot previously approved these changes May 7, 2025
Comment on lines +25 to +27
'StringPrimary',
'StringPrimary',
'StringPrimary',
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
'StringPrimary',
'StringPrimary',
'StringPrimary',
'StringPrimary',

Comment on lines +25 to +27
'StringPrimary',
'StringPrimary',
'StringPrimary',
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@coderabbitai - does this look alright to you? Analyse the usage of getNodeMappingPattern and cross-check with Postgresql's function signature. You may need to revise similar changes in the current PR.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧩 Analysis chain

@martin-georgiev I've analyzed the usage of getNodeMappingPattern and cross-checked with PostgreSQL's function signatures, and found a potential issue.

The code in Tsrange.php correctly overrides getNodeMappingPattern() to use three StringPrimary nodes, which is appropriate for timestamp ranges where all parameters (including bounds) are passed as strings.

However, I noticed an inconsistency:

  • BaseRangeFunction defines the default pattern as [ArithmeticPrimary, ArithmeticPrimary, StringPrimary]
  • Tsrange and Tstzrange override it to use three StringPrimary nodes
  • Daterange also uses string literals in the tests, but doesn't override the pattern

This means Daterange inherits the default pattern with ArithmeticPrimary nodes, but is actually used with string literals in tests. For consistency, Daterange should also override getNodeMappingPattern() similar to Tsrange and Tstzrange.

Would you like me to provide a code suggestion for the Daterange class?

(_/)
(•ㅅ•)
/ づ ♥ ~thinking...


🏁 Script executed:

#!/bin/bash
# First, let's examine the BaseRangeFunction class
cat src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/BaseRangeFunction.php

# Let's also see if there are any other range functions that have been modified
cat src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Tstzrange.php

# Let's check some of the tests to understand the usage
cat tests/Unit/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/TsrangeTest.php

Length of output: 2443


🏁 Script executed:

#!/bin/bash
# Let's check the other range functions to see if they properly override getNodeMappingPattern
cat src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Int4range.php
cat src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Int8range.php
cat src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Numrange.php
cat src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Daterange.php

Length of output: 2125


🏁 Script executed:

#!/bin/bash
# Let's check the Daterange tests to see how dates are passed
cat tests/Unit/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/DaterangeTest.php

Length of output: 853


🏁 Script executed:

#!/bin/bash
# Look at how these range functions are used in the tests, specifically in DQL statements
grep -A 3 "getDqlStatements" tests/Unit/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/DaterangeTest.php
grep -A 3 "getDqlStatements" tests/Unit/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Int4rangeTest.php
grep -A 3 "getDqlStatements" tests/Unit/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/TsrangeTest.php

Length of output: 822


🌐 Web query:

PostgreSQL tsrange function signature and parameters

💡 Result:

PostgreSQL's tsrange is a built-in range type that represents a range of timestamp values without time zone. This range type is useful for working with time periods in PostgreSQL databases.

Function Signature and Parameters

The tsrange constructor function can be used with two different parameter patterns:

  1. Two-parameter form: Creates a range with inclusive lower bound and exclusive upper bound (normal form)

    tsrange(lower_bound timestamp, upper_bound timestamp)
  2. Four-parameter form: Creates a range with explicitly specified bound inclusivity

    tsrange(lower_bound timestamp, upper_bound timestamp, lower_inclusive boolean, upper_inclusive boolean)

Where:

  • lower_bound: The starting timestamp of the range
  • upper_bound: The ending timestamp of the range
  • lower_inclusive: Boolean indicating whether the lower bound is inclusive (true) or exclusive (false)
  • upper_inclusive: Boolean indicating whether the upper bound is inclusive (true) or exclusive (false)

Usage Examples

The tsrange type is especially useful for scenarios involving time periods, such as reservations, events, or availability windows:

-- Create a reservation table using tsrange
CREATE TABLE reservation (
    room int, 
    during tsrange
);

-- Insert a reservation for a specific time period
INSERT INTO reservation VALUES (1108, '[2010-01-01 14:30, 2010-01-01 15:30)');

You can also query records that fall within a specific time range:

-- Select orders created within a specific time range
SELECT * FROM orders
WHERE created_at <@ tsrange('2023-03-14 04:00:00', '2023-03-14 10:00:00');

PostgreSQL also provides a corresponding multirange type called tsmultirange, which represents an ordered list of non-contiguous, non-empty, non-null timestamp ranges[1].

Citations:


@martin-georgiev I've analyzed the usage of getNodeMappingPattern in the Tsrange class and cross-checked with PostgreSQL's function signatures.

The implementation looks correct for the following reasons:

  1. Tsrange overrides getNodeMappingPattern() to use three StringPrimary nodes, which is appropriate since PostgreSQL timestamp ranges expect string parameters for both timestamps and bounds.

  2. Other date/time range functions follow the same pattern:

    • Tstzrange uses three StringPrimary nodes
    • Daterange also correctly overrides the pattern with three StringPrimary nodes
  3. The default pattern in BaseRangeFunction ([ArithmeticPrimary, ArithmeticPrimary, StringPrimary]) is suited for numeric ranges like int4range, int8range, and numrange, where the first two parameters are numbers.

  4. All the tests confirm this design, showing that:

    • Date/time ranges are used with string literals
    • Numeric ranges are used with numeric literals

This implementation correctly handles PostgreSQL's range function signatures, including the optional third parameter for specifying bounds. No revisions appear necessary for the current PR.


🧬 Code Graph Analysis Results

Relevant snippets for analyzing getNodeMappingPattern usage in Tsrange.php and cross-checking with PostgreSQL function signature:


File: src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Tsrange.php (lines 1-29)

<?php

declare(strict_types=1);

namespace MartinGeorgiev\Doctrine\ORM\Query\AST\Functions;

/**
 * Implementation of PostgreSQL TSRANGE().
 *
 * @see https://www.postgresql.org/docs/17/rangetypes.html
 * @since 2.9
 *
 * @author Martin Georgiev <martin.georgiev@gmail.com>
 */
class Tsrange extends BaseRangeFunction
{
    protected function getFunctionName(): string
    {
        return 'tsrange';
    }

    protected function getNodeMappingPattern(): array
    {
        return [
            'StringPrimary',
            'StringPrimary',
            'StringPrimary',
        ];
    }
}

File: src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/BaseRangeFunction.php (lines 13-33)

abstract class BaseRangeFunction extends BaseVariadicFunction
{
    protected function getNodeMappingPattern(): array
    {
        return [
            'ArithmeticPrimary',
            'ArithmeticPrimary',
            'StringPrimary',
        ];
    }

    protected function getMinArgumentCount(): int
    {
        return 2;
    }

    protected function getMaxArgumentCount(): int
    {
        return 3;
    }
}
  • This base class defines a default getNodeMappingPattern with two ArithmeticPrimary and one StringPrimary nodes.
  • It also defines minimum and maximum argument counts (2 and 3 respectively).

File: src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Daterange.php (lines 17-29)

protected function getFunctionName(): string
{
    return 'daterange';
}

protected function getNodeMappingPattern(): array
{
    return [
        'StringPrimary',
        'StringPrimary',
        'StringPrimary',
    ];
}
  • This function uses the same node mapping pattern as Tsrange.

File: src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Tstzrange.php (lines 17-29)

protected function getFunctionName(): string
{
    return 'tstzrange';
}

protected function getNodeMappingPattern(): array
{
    return [
        'StringPrimary',
        'StringPrimary',
        'StringPrimary',
    ];
}
  • This function also uses the same node mapping pattern as Tsrange.

Summary of relevant context:

  • Tsrange overrides getNodeMappingPattern to return three 'StringPrimary' elements.
  • The base class BaseRangeFunction returns a different pattern: two 'ArithmeticPrimary' and one 'StringPrimary'.
  • Other range functions like Daterange and Tstzrange also use the three 'StringPrimary' pattern.
  • PostgreSQL's tsrange function signature (per official docs) accepts:
    • Two timestamp values (lower and upper bounds),
    • An optional text parameter for range bounds (e.g., '[)', '()').

This matches the three parameters represented as strings in the node mapping pattern.


Additional snippet for comparison (RegexpReplace.php) showing complex node mapping patterns:

protected function getNodeMappingPattern(): array
{
    /*
     * PostgreSQL overloads the 4th argument depending on its type:
     *   - if the 4th arg is a string, it’s taken as flags.
     *   - if the 4th arg is an integer, it’s taken as start position. This can be extended with the Nth argument.
     */
    return [
        'StringPrimary,StringPrimary,StringPrimary,ArithmeticPrimary,ArithmeticPrimary,StringPrimary', // with start, N and flags: regexp_replace(string, pattern, replacement, 3, 2, 'i')
        'StringPrimary,StringPrimary,StringPrimary,ArithmeticPrimary,ArithmeticPrimary', // with start and N: regexp_replace(string, pattern, replacement, 3, 2)
        'StringPrimary,StringPrimary,StringPrimary,ArithmeticPrimary', // with start: regexp_replace(string, pattern, replacement, 3)
        'StringPrimary,StringPrimary,StringPrimary,StringPrimary', // with flags: regexp_replace(string, pattern, replacement, 'i')
        'StringPrimary,StringPrimary,StringPrimary', // basic replacement: regexp_replace(string, pattern, replacement)
    ];
}
  • Shows that getNodeMappingPattern can return multiple patterns to handle function overloading.
  • Tsrange returns a single pattern array with three 'StringPrimary' elements.

Summary for reviewer:

  • The Tsrange class overrides getNodeMappingPattern to expect three string-type nodes, consistent with the PostgreSQL tsrange function signature: two timestamps and an optional text parameter.
  • The base class BaseRangeFunction uses a different default pattern (two arithmetic and one string), but Tsrange and similar range functions (Daterange, Tstzrange) override it to three strings.
  • This pattern aligns with the PostgreSQL documentation for tsrange(lower timestamp, upper timestamp, text bounds).
  • Other range functions in the same namespace follow the same pattern, indicating consistency.
  • The getNodeMappingPattern method can return multiple patterns for overloaded functions, but Tsrange uses a single pattern.

This context should assist in verifying the correctness of getNodeMappingPattern usage in Tsrange.php and guide any necessary revisions in the current PR or similar functions.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you sure? Explain why RegexpCount and RegexpInstr look different then.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Let's check the implementations of RegexpCount and RegexpInstr
cat src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/RegexpCount.php
cat src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/RegexpInstr.php

# Let's also look at what base class they extend from
grep -A 1 "class RegexpCount" src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/RegexpCount.php
grep -A 1 "class RegexpInstr" src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/RegexpInstr.php

# Compare with Tsrange implementation again for reference
cat src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Tsrange.php

# Compare with BaseRangeFunction
cat src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/BaseRangeFunction.php

# Let's also check if there's a base class for regexp functions
ls -la src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/BaseRegexpFunction.php 2>/dev/null || echo "BaseRegexpFunction.php not found"

Length of output: 4544


🏁 Script executed:

#!/bin/bash
# Let's check the BaseVariadicFunction to understand how the node mapping pattern is used
cat src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/BaseVariadicFunction.php

# Also check the tests for RegexpCount and RegexpInstr to understand how they're used
cat tests/Unit/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/RegexpCountTest.php 2>/dev/null || echo "RegexpCountTest.php not found"
cat tests/Unit/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/RegexpInstrTest.php 2>/dev/null || echo "RegexpInstrTest.php not found"

Length of output: 12031


@martin-georgiev I understand your question now. The difference between RegexpCount/RegexpInstr and Tsrange implementations is intentional and relates to how they handle function overloads in PostgreSQL:

Why They Look Different

  1. Different Function Complexity:

    • RegexpCount/RegexpInstr: Support multiple function signatures with varying parameter counts (2-4 and 2-7 parameters respectively)
    • Tsrange: Supports a simpler function with 2-3 parameters of consistent types
  2. Node Mapping Format:

    • RegexpCount/RegexpInstr use comma-separated strings in an array:
      protected function getNodeMappingPattern(): array
      {
          return [
              'StringPrimary,StringPrimary,ArithmeticPrimary,StringPrimary', // 4 params
              'StringPrimary,StringPrimary,ArithmeticPrimary', // 3 params
              'StringPrimary,StringPrimary', // 2 params
          ];
      }
    • Tsrange uses individual array elements:
      protected function getNodeMappingPattern(): array
      {
          return [
              'StringPrimary',
              'StringPrimary',
              'StringPrimary',
          ];
      }
  3. How BaseVariadicFunction Processes Them:

    • For RegexpCount/RegexpInstr: It tries each pattern string, splitting it by commas and matching arguments until one succeeds
    • For Tsrange: It uses the single pattern with consistent parameter types

Both implementations are correct but serve different purposes based on the complexity of the PostgreSQL functions they represent. The regexp functions need to handle multiple overloads with varying parameter counts, while range functions have a more consistent signature.


🧬 Code Graph Analysis Results

Relevant snippets for understanding Tsrange.php and the user comment about RegexpCount and RegexpInstr differences:


File: src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Tsrange.php (lines 1-29)

<?php

declare(strict_types=1);

namespace MartinGeorgiev\Doctrine\ORM\Query\AST\Functions;

/**
 * Implementation of PostgreSQL TSRANGE().
 *
 * @see https://www.postgresql.org/docs/17/rangetypes.html
 * @since 2.9
 *
 * @author Martin Georgiev <martin.georgiev@gmail.com>
 */
class Tsrange extends BaseRangeFunction
{
    protected function getFunctionName(): string
    {
        return 'tsrange';
    }

    protected function getNodeMappingPattern(): array
    {
        return [
            'StringPrimary',
            'StringPrimary',
            'StringPrimary',
        ];
    }
}

File: src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/BaseRangeFunction.php (lines 13-33)

abstract class BaseRangeFunction extends BaseVariadicFunction
{
    protected function getNodeMappingPattern(): array
    {
        return [
            'ArithmeticPrimary',
            'ArithmeticPrimary',
            'StringPrimary',
        ];
    }

    protected function getMinArgumentCount(): int
    {
        return 2;
    }

    protected function getMaxArgumentCount(): int
    {
        return 3;
    }
}
  • Abstract base class for range functions.
  • Defines a default node mapping pattern expecting two ArithmeticPrimary and one StringPrimary arguments.
  • Defines minimum and maximum argument counts (2 to 3).

File: src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Numrange.php (lines 17-20)

protected function getFunctionName(): string
{
    return 'numrange';
}
  • Example of another range function overriding the function name.

File: src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Daterange.php (lines 17-29)

protected function getFunctionName(): string
{
    return 'daterange';
}

protected function getNodeMappingPattern(): array
{
    return [
        'StringPrimary',
        'StringPrimary',
        'StringPrimary',
    ];
}
  • Shows a range function overriding the node mapping pattern to three StringPrimary arguments, similar to Tsrange.

File: src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/RegexpReplace.php (lines 22-41)

protected function getNodeMappingPattern(): array
{
    /*
     * PostgreSQL overloads the 4th argument depending on its type:
     *   - if the 4th arg is a string, it’s taken as flags.
     *   - if the 4th arg is an integer, it’s taken as start position. This can be extended with the Nth argument.
     */
    return [
        'StringPrimary,StringPrimary,StringPrimary,ArithmeticPrimary,ArithmeticPrimary,StringPrimary', // with start, N and flags: regexp_replace(string, pattern, replacement, 3, 2, 'i')
        'StringPrimary,StringPrimary,StringPrimary,ArithmeticPrimary,ArithmeticPrimary', // with start and N: regexp_replace(string, pattern, replacement, 3, 2)
        'StringPrimary,StringPrimary,StringPrimary,ArithmeticPrimary', // with start: regexp_replace(string, pattern, replacement, 3)
        'StringPrimary,StringPrimary,StringPrimary,StringPrimary', // with flags: regexp_replace(string, pattern, replacement, 'i')
        'StringPrimary,StringPrimary,StringPrimary', // basic replacement: regexp_replace(string, pattern, replacement)
    ];
}

protected function getFunctionName(): string
{
    return 'regexp_replace';
}
  • Shows a complex node mapping pattern with multiple overloads depending on argument types.
  • Demonstrates why some functions like RegexpReplace have more complex patterns compared to simpler range functions.

Summary of relevance:

  • Tsrange.php overrides getNodeMappingPattern() to expect three StringPrimary arguments, differing from the base class BaseRangeFunction which expects two ArithmeticPrimary and one StringPrimary.
  • Other range functions like Daterange also override the node mapping pattern similarly.
  • The RegexpReplace function shows a more complex and varied node mapping pattern due to PostgreSQL's function overloading and argument type variations.
  • This contrast explains why RegexpCount and RegexpInstr might have different node mapping patterns compared to simpler range functions like Tsrange.

These snippets provide the factual context needed to understand the differences in node mapping patterns and function implementations.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@coderabbitai Now revise your approval of the PR and request changes where they are relevant!

Copy link

@coderabbitai coderabbitai bot May 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A summary of the edits CodeRabbit can apply:

Request changes to Tsrange.php so that getNodeMappingPattern() returns a single-element array ['StringPrimary'], fixing the mapping-bug in BaseVariadicFunction.

Request changes to Daterange.php so that getNodeMappingPattern() returns ['StringPrimary'] only, avoiding the three-entry bug.

Request changes to Tstzrange.php so that getNodeMappingPattern() returns ['StringPrimary'] only, correcting the pattern application bug.

Modify Daterange.php so getNodeMappingPattern returns a single 'StringPrimary' element.

Modify Tsrange.php so getNodeMappingPattern returns a single 'StringPrimary' element.

Modify Tstzrange.php so getNodeMappingPattern returns a single 'StringPrimary' element.

The Tstzrange.php file will be updated to use a single-element node mapping pattern ['StringPrimary'] so that all parameters are correctly treated as StringPrimary.

I will request changes in Tsrange.php to simplify getNodeMappingPattern() to a single ['StringPrimary'] element.

I will request changes in Daterange.php to simplify getNodeMappingPattern() to a single ['StringPrimary'] element.

I will request changes in Tstzrange.php to simplify getNodeMappingPattern() to a single ['StringPrimary'] element.

The getNodeMappingPattern in Tsrange.php will be simplified to a single 'StringPrimary'.

The getNodeMappingPattern in Tstzrange.php will be simplified to a single 'StringPrimary'.

The getNodeMappingPattern in Daterange.php will be simplified to a single 'StringPrimary'.

A plan of each step that CodeRabbit will take in order to execute the edits for each file (with inline rough draft snippets):

✏️ src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Tsrange.php

Remove the existing multi-line return array in getNodeMappingPattern():

return [
    'StringPrimary',
    'StringPrimary',
    'StringPrimary',
];

Replace it with a single-element return immediately after the method signature:

return ['StringPrimary'];

Save Tsrange.php and run the TsrangeTest suite to verify that both 2-argument and 3-argument forms of tsrange now parse without errors.

✏️ src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Daterange.php

Remove the existing multi-line return array in getNodeMappingPattern():

return [
    'StringPrimary',
    'StringPrimary',
    'StringPrimary',
];

Replace it with:

return ['StringPrimary'];

Save Daterange.php and run the DaterangeTest suite to confirm both 2-argument and 3-argument usages of daterange parse correctly.

✏️ src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Tstzrange.php

Remove the existing multi-line return array in getNodeMappingPattern():

return [
    'StringPrimary',
    'StringPrimary',
    'StringPrimary',
];

Replace it with:

return ['StringPrimary'];

Save Tstzrange.php and run the TstzrangeTest suite to ensure both 2-argument and 3-argument invocations of tstzrange parse correctly.

A summary of the context that CodeRabbit is considering across the codebase:

We need to locate the BaseVariadicFunction definition and see how it handles a single-element node mapping pattern to confirm that returning ['StringPrimary'] will correctly apply that type to all arguments.

Locate every subclass of BaseRangeFunction that overrides getNodeMappingPattern, so we know which files need updating.

Inspect Daterange.php to determine the exact lines to replace.

Inspect Tsrange.php to determine the exact lines to replace.

Inspect Tstzrange.php to determine the exact lines to replace.

Confirm that BaseRangeFunction defines a minimum of 2 and a maximum of 3 arguments, so a single‐element pattern will correctly apply to both two‐ and three‐parameter invocations.

Identify all date/time range function classes to ensure consistent updates across Tsrange, Daterange, and Tstzrange.

Locate the existing override of getNodeMappingPattern() in Tsrange.php to determine the exact lines to replace.

Locate the existing override of getNodeMappingPattern() in Daterange.php to determine the exact lines to replace.

Locate the existing override of getNodeMappingPattern() in Tstzrange.php to determine the exact lines to replace.

Identify all range‐function classes that override getNodeMappingPattern so we can update their patterns to a single 'StringPrimary'.

  • Created local commit - (🔄 Check again to try again)

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

CodeRabbit has failed to create a local commit with your edits.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

CodeRabbit has failed to create a local commit with your edits.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

CodeRabbit has failed to create a local commit with your edits.

@martin-georgiev martin-georgiev changed the title Add the ability to specify range bounds feat: add the ability to specify range bounds May 12, 2025
@martin-georgiev
Copy link
Owner

martin-georgiev commented May 13, 2025

@janklan, check out what decision Coderabbit finally came to. Follow the back-and-forth discussion for more details of why a change got suggested with a single return ['StringPrimary'];.


/**
* @see https://www.postgresql.org/docs/17/rangetypes.html
* @since 3.1
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* @since 3.1
* @since 3.2

Comment on lines +25 to +27
'StringPrimary',
'StringPrimary',
'StringPrimary',
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
'StringPrimary',
'StringPrimary',
'StringPrimary',
'StringPrimary',

Comment on lines +25 to +27
'StringPrimary',
'StringPrimary',
'StringPrimary',
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
'StringPrimary',
'StringPrimary',
'StringPrimary',
'StringPrimary',

Comment on lines +18 to +20
'ArithmeticPrimary',
'ArithmeticPrimary',
'StringPrimary',
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
'ArithmeticPrimary',
'ArithmeticPrimary',
'StringPrimary',
'ArithmeticPrimary',
'StringPrimary',

@martin-georgiev
Copy link
Owner

Superseded by #380

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants