-
-
Notifications
You must be signed in to change notification settings - Fork 56
feat: add support for distance operator <@>
#361
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: add support for distance operator <@>
#361
Conversation
7da4f07 to
ff82710
Compare
WalkthroughThis update introduces a new custom DQL function, Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Doctrine
participant DistanceFunction
participant PostgreSQL
User->>Doctrine: Write DQL using DISTANCE()
Doctrine->>DistanceFunction: Parse and translate DISTANCE()
DistanceFunction->>PostgreSQL: Generate SQL with <@> operator
PostgreSQL-->>Doctrine: Return query results
Doctrine-->>User: Return processed results
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms (20)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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)
Other keywords and placeholders
Documentation and Community
|
b4edfb6 to
2440a02
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 5
🧹 Nitpick comments (2)
tests/Unit/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/DistanceTest.php (2)
28-35: Fix code style issues and consider adding test cases for error conditions.
- The CI pipeline flagged the use of double quotes which should be replaced with single quotes.
- Consider adding test cases that verify error handling, such as invalid point formats.
protected function getDqlStatements(): array { return [ - \sprintf("SELECT DISTANCE(e.point, '(2.320041, 48.858889)') FROM %s e", ContainsPoints::class), - \sprintf("SELECT DISTANCE(e.point, e.point2) FROM %s e", ContainsPoints::class), - \sprintf("SELECT DISTANCE('(1.0, 1.0)', '(2.0, 2.0)') FROM %s e", ContainsPoints::class), + \sprintf('SELECT DISTANCE(e.point, \'(2.320041, 48.858889)\') FROM %s e', ContainsPoints::class), + \sprintf('SELECT DISTANCE(e.point, e.point2) FROM %s e', ContainsPoints::class), + \sprintf('SELECT DISTANCE(\'(1.0, 1.0)\', \'(2.0, 2.0)\') FROM %s e', ContainsPoints::class), ]; }
1-36: Consider adding PHPDoc comments for better documentation.Adding PHPDoc comments to the class and its methods would improve the documentation and better describe the purpose of this test class and what each method is testing.
Example:
/** * Test case for the DISTANCE DQL function that wraps PostgreSQL's <@> earth distance operator. */ class DistanceTest extends TestCase { /** * Returns a mapping of DQL function names to their implementing classes. * * @return array<string, string> */ protected function getStringFunctions(): array { // ... } // Similar PHPDoc for other methods }🧰 Tools
🪛 GitHub Actions: CI
[error] 20-29: PHP CS Fixer: Use single quotes instead of double quotes (single_quote) and missing newline at end of file (single_blank_line_at_eof).
🛑 Comments failed to post (5)
docs/INTEGRATING-WITH-LARAVEL.md (1)
221-221:
⚠️ Potential issueIncorrect class reference for DISTANCE function.
TheDISTANCEmapping points toDistanceTest::classinstead of the actualDistancefunction class. This will prevent Doctrine from loading the function correctly.Apply this diff:
- 'DISTANCE' => MartinGeorgiev\Doctrine\ORM\Query\AST\Functions\DistanceTest::class, + 'DISTANCE' => MartinGeorgiev\Doctrine\ORM\Query\AST\Functions\Distance::class,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.'DISTANCE' => MartinGeorgiev\Doctrine\ORM\Query\AST\Functions\Distance::class,docs/INTEGRATING-WITH-DOCTRINE.md (1)
166-166:
⚠️ Potential issueFix class name for DISTANCE custom function.
The example registersDISTANCEwith theDistanceTestclass, but it should referenceDistance::classto correctly map the DQL function.Proposed change:
-$configuration->addCustomStringFunction('DISTANCE', MartinGeorgiev\Doctrine\ORM\Query\AST\Functions\DistanceTest::class); +$configuration->addCustomStringFunction('DISTANCE', MartinGeorgiev\Doctrine\ORM\Query\AST\Functions\Distance::class);src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Distance.php (1)
23-23:
⚠️ Potential issueAdd missing newline at end of file.
PHP CS Fixer is failing due to a missing newline at EOF. Please ensure the file ends with a single blank line.Patch:
} +📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.}tests/Unit/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/DistanceTest.php (2)
36-36:
⚠️ Potential issueAdd newline at the end of file.
The CI pipeline flagged that there's a missing newline at the end of the file.
} +📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.}
19-26:
⚠️ Potential issueFix code style issues as flagged by the pipeline.
The CI pipeline flagged code style issues with the use of double quotes. According to the project's coding standards, single quotes should be used instead.
protected function getExpectedSqlStatements(): array { return [ - "SELECT (c0_.point <@> '(2.320041, 48.858889)') AS sclr_0 FROM ContainsPoints c0_", - "SELECT (c0_.point <@> c0_.point2) AS sclr_0 FROM ContainsPoints c0_", - "SELECT ('(1.0, 1.0)' <@> '(2.0, 2.0)') AS sclr_0 FROM ContainsPoints c0_", + 'SELECT (c0_.point <@> \'(2.320041, 48.858889)\') AS sclr_0 FROM ContainsPoints c0_', + 'SELECT (c0_.point <@> c0_.point2) AS sclr_0 FROM ContainsPoints c0_', + 'SELECT (\'(1.0, 1.0)\' <@> \'(2.0, 2.0)\') AS sclr_0 FROM ContainsPoints c0_', ]; }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.protected function getExpectedSqlStatements(): array { return [ 'SELECT (c0_.point <@> \'(2.320041, 48.858889)\') AS sclr_0 FROM ContainsPoints c0_', 'SELECT (c0_.point <@> c0_.point2) AS sclr_0 FROM ContainsPoints c0_', 'SELECT (\'(1.0, 1.0)\' <@> \'(2.0, 2.0)\') AS sclr_0 FROM ContainsPoints c0_', ]; }
8a8df52 to
ad7f2d5
Compare
355b5d2 to
d6332bb
Compare
d6332bb to
b922291
Compare
|
@martin-georgiev I have an error with scrutinizer: |
Oopsie... My bad when I introduced integration tests with a real PostgreSQL server earlier today. It's ok to ignore Scrutinizer failure in this particular instance. |
<@>
Summary by CodeRabbit
New Features
Documentation
Tests
Chores