feat: add support for DATE_ADD(), DATE_SUBTRACT() and DATE_BIN()
#345
Conversation
WalkthroughThis pull request introduces three new DQL functions— Changes
Sequence Diagram(s)sequenceDiagram
participant C as Client
participant P as DQL Parser
participant F as Date Function (Add/Subtract)
participant T as Timezone Validator
participant DB as Database
C->>P: Submit DQL query with date function
P->>F: Parse and invoke custom date function
F->>F: Validate arguments (ensure count between 2 and 3)
alt Three arguments provided
F->>T: Validate timezone argument
T-->>F: Return valid result or throw InvalidTimezoneException
end
F->>DB: Build and execute SQL query
DB-->>F: Return SQL result
F-->>C: Return query result
Poem
Tip ⚡💬 Agentic Chat (Pro Plan, General Availability)
✨ Finishing Touches
🪧 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
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/DateBin.php (1)
1-28: Implementation matches PostgreSQL DATE_BIN() functionThe implementation correctly defines the DATE_BIN() function with the proper arguments and structure. The syntax matches PostgreSQL's date_bin(interval, timestamp, origin) function and the documentation is thorough, providing good context and example usage.
One minor suggestion: Consider updating the PostgreSQL documentation link to version 16 for consistency with the other date function implementations.
-@see https://www.postgresql.org/docs/14/functions-datetime.html +@see https://www.postgresql.org/docs/16/functions-datetime.htmlsrc/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/DateAdd.php (1)
33-44: Inconsistent function name casing in error messagesThere's an inconsistency in the function name casing used for error messages. For argument validation, lowercase
date_addis used, while for timezone validation, uppercaseDATE_ADDis used.- throw InvalidArgumentForVariadicFunctionException::between('date_add', 2, 3); + throw InvalidArgumentForVariadicFunctionException::between('DATE_ADD', 2, 3);Or alternatively:
- $this->validateTimezone($arguments[2], 'DATE_ADD'); + $this->validateTimezone($arguments[2], 'date_add');Choose one casing style and use it consistently.
src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/DateSubtract.php (1)
33-44: Inconsistent function name casing in error messagesSimilar to the DateAdd class, there's an inconsistency in the function name casing used for error messages. For argument validation, lowercase
date_subtractis used, while for timezone validation, uppercaseDATE_SUBTRACTis used.- throw InvalidArgumentForVariadicFunctionException::between('date_subtract', 2, 3); + throw InvalidArgumentForVariadicFunctionException::between('DATE_SUBTRACT', 2, 3);Or alternatively:
- $this->validateTimezone($arguments[2], 'DATE_SUBTRACT'); + $this->validateTimezone($arguments[2], 'date_subtract');Choose one casing style and use it consistently.
src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Traits/TimezoneValidationTrait.php (1)
38-47: Consider improving error handling in isValidTimezone method.The method catches all exceptions generically. While this works, it could be more specific.
Consider catching the specific
\Exceptiontype thatDateTimeZoneconstructor throws for invalid timezones (\Exceptionis currently used which is very broad):private function isValidTimezone(string $timezone): bool { try { new \DateTimeZone($timezone); return true; - } catch (\Exception) { + } catch (\Exception $e) { + // DateTimeZone constructor throws exception for invalid timezone identifiers return false; } }tests/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/DateAddTest.php (1)
20-36: Test cases provide good coverage for DATE_ADD function.The test covers multiple variations of the
DATE_ADD()function including different time intervals and with/without timezone specifications.Consider adding edge case tests to ensure robustness:
protected function getExpectedSqlStatements(): array { return [ 'adds 1 day with timezone' => "SELECT date_add(c0_.datetimetz1, '1 day', 'Europe/Sofia') AS sclr_0 FROM ContainsDates c0_", 'adds 2 hours with timezone' => "SELECT date_add(c0_.datetimetz1, '2 hours', 'UTC') AS sclr_0 FROM ContainsDates c0_", 'adds 3 days without timezone' => "SELECT date_add(c0_.datetimetz1, '3 days') AS sclr_0 FROM ContainsDates c0_", + 'adds complex interval' => "SELECT date_add(c0_.datetimetz1, '1 year 2 months 3 days 4 hours 5 minutes 6 seconds') AS sclr_0 FROM ContainsDates c0_", ]; } protected function getDqlStatements(): array { return [ 'adds 1 day with timezone' => \sprintf("SELECT DATE_ADD(e.datetimetz1, '1 day', 'Europe/Sofia') FROM %s e", ContainsDates::class), 'adds 2 hours with timezone' => \sprintf("SELECT DATE_ADD(e.datetimetz1, '2 hours', 'UTC') FROM %s e", ContainsDates::class), 'adds 3 days without timezone' => \sprintf("SELECT DATE_ADD(e.datetimetz1, '3 days') FROM %s e", ContainsDates::class), + 'adds complex interval' => \sprintf("SELECT DATE_ADD(e.datetimetz1, '1 year 2 months 3 days 4 hours 5 minutes 6 seconds') FROM %s e", ContainsDates::class), ]; }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/DateAdd.php(1 hunks)src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/DateBin.php(1 hunks)src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/DateSubtract.php(1 hunks)src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Exception/InvalidTimezoneException.php(1 hunks)src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Traits/TimezoneValidationTrait.php(1 hunks)tests/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/DateAddTest.php(1 hunks)tests/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/DateBinTest.php(1 hunks)tests/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/DateSubtractTest.php(1 hunks)tests/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/TestCase.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/TestCase.phptests/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/DateBinTest.phptests/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/DateSubtractTest.phptests/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/DateAddTest.php
🧬 Code Graph Analysis (7)
src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/DateBin.php (3)
src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/BaseFunction.php (3)
BaseFunction(20-105)setFunctionPrototype(57-60)addNodeMapping(62-65)src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/DateAdd.php (1)
customizeFunction(28-31)src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/DateSubtract.php (1)
customizeFunction(28-31)
tests/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/DateBinTest.php (3)
fixtures/MartinGeorgiev/Doctrine/Entity/Entity.php (1)
Entity(10-16)src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/DateBin.php (1)
DateBin(19-28)tests/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/TestCase.php (4)
TestCase(15-120)getStringFunctions(79-82)getExpectedSqlStatements(87-87)getDqlStatements(92-92)
src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/DateSubtract.php (4)
src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Exception/InvalidArgumentForVariadicFunctionException.php (2)
InvalidArgumentForVariadicFunctionException(7-46)between(29-37)src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/BaseVariadicFunction.php (1)
BaseVariadicFunction(20-73)src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/BaseFunction.php (1)
setFunctionPrototype(57-60)src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Traits/TimezoneValidationTrait.php (1)
validateTimezone(25-36)
src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/DateAdd.php (4)
src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Exception/InvalidArgumentForVariadicFunctionException.php (2)
InvalidArgumentForVariadicFunctionException(7-46)between(29-37)src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/BaseVariadicFunction.php (1)
BaseVariadicFunction(20-73)src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/BaseFunction.php (1)
setFunctionPrototype(57-60)src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Traits/TimezoneValidationTrait.php (1)
validateTimezone(25-36)
src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Traits/TimezoneValidationTrait.php (1)
src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Exception/InvalidTimezoneException.php (3)
InvalidTimezoneException(14-33)forNonLiteralNode(16-23)forInvalidTimezone(25-32)
tests/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/DateSubtractTest.php (4)
fixtures/MartinGeorgiev/Doctrine/Entity/Entity.php (1)
Entity(10-16)src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/DateSubtract.php (1)
DateSubtract(24-45)src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Exception/InvalidTimezoneException.php (1)
InvalidTimezoneException(14-33)tests/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/TestCase.php (4)
TestCase(15-120)getStringFunctions(79-82)getExpectedSqlStatements(87-87)getDqlStatements(92-92)
tests/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/DateAddTest.php (4)
fixtures/MartinGeorgiev/Doctrine/Entity/Entity.php (1)
Entity(10-16)src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/DateAdd.php (1)
DateAdd(24-45)src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Exception/InvalidTimezoneException.php (1)
InvalidTimezoneException(14-33)tests/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/TestCase.php (5)
TestCase(15-120)getStringFunctions(79-82)getExpectedSqlStatements(87-87)getDqlStatements(92-92)buildEntityManager(116-119)
⏰ Context from checks skipped due to timeout of 90000ms (14)
- GitHub Check: PHP 8.4 + Doctrine ORM 2.14 + Doctrine Lexer latest
- GitHub Check: PHP 8.4 + Doctrine ORM latest + Doctrine Lexer latest
- GitHub Check: PHP 8.4 + Doctrine ORM latest + Doctrine Lexer 3.0
- GitHub Check: PHP 8.1 + Doctrine ORM 2.14 + Doctrine Lexer 1.2
- GitHub Check: PHP 8.4 + Doctrine ORM 3.0 + Doctrine Lexer 3.0
- GitHub Check: PHP 8.3 + Doctrine ORM 2.18 + 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.3 + Doctrine ORM 3.0 + Doctrine Lexer latest
- GitHub Check: PHP 8.2 + Doctrine ORM latest + Doctrine Lexer 3.0
- GitHub Check: PHP 8.2 + Doctrine ORM 3.0 + Doctrine Lexer 3.0
- GitHub Check: PHP 8.1 + Doctrine ORM 3.0 + Doctrine Lexer latest
- GitHub Check: PHP 8.1 + Doctrine ORM 2.18 + Doctrine Lexer 2.1
- GitHub Check: sloth
🔇 Additional comments (12)
tests/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/TestCase.php (1)
116-116: LGTM: Appropriate visibility changeChanging the visibility from
privatetoprotectedallows test subclasses to access thebuildEntityManagermethod, which is necessary for the new date function test classes.src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/DateAdd.php (2)
1-27: Implementation matches PostgreSQL DATE_ADD() functionThe implementation correctly defines the DATE_ADD() function and includes the TimezoneValidationTrait to validate the optional timezone parameter. The documentation is thorough with appropriate reference to PostgreSQL docs and a clear usage example.
28-31: Function prototype is correctThe function prototype is properly set to correctly format the SQL output.
src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/DateSubtract.php (3)
1-27: Implementation matches PostgreSQL DATE_SUBTRACT() functionThe implementation correctly defines the DATE_SUBTRACT() function and includes the TimezoneValidationTrait to validate the optional timezone parameter. The documentation is thorough with appropriate reference to PostgreSQL docs and a clear usage example.
28-31: Function prototype is correctThe function prototype is properly set to correctly format the SQL output.
1-44:✅ Verification successful
Missing test cases verification
According to the coding guidelines, test files for SQL functions should include comprehensive use cases and example SQL queries matching PostgreSQL documentation. As the test files aren't included in this review, I can't verify this requirement.
🏁 Script executed:
#!/bin/bash # Check test files for these functions to ensure they include comprehensive test cases echo "Checking test files for date functions..." echo "1. DateBinTest.php:" fd "DateBinTest.php" tests/ --exec cat echo "2. DateAddTest.php:" fd "DateAddTest.php" tests/ --exec cat echo "3. DateSubtractTest.php:" fd "DateSubtractTest.php" tests/ --exec catLength of output: 5409
Test Cases Verified for DateSubtract Function
I confirmed that the test cases for SQL functions are comprehensive. The
tests/DateSubtractTest.phpfile includes valid examples for subtracting intervals both with and without a timezone, as well as negative tests for invalid timezones. This aligns with the PostgreSQL documentation requirements and our coding guidelines.No further changes are required.
tests/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/DateSubtractTest.php (3)
38-45: Test case for invalid timezone properly validates error handling.The test correctly validates that an invalid timezone throws the expected exception with the appropriate error message.
20-36: Test coverage includes good examples aligned with PostgreSQL documentation.The test cases for
DATE_SUBTRACT()cover important scenarios with varying time intervals (days, hours) and both with and without timezone specifications.
11-18: Function registration looks good.The function is correctly registered with the appropriate class mapping.
src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Traits/TimezoneValidationTrait.php (1)
25-36: Timezone validation implementation properly handles edge cases.The validation logic correctly checks for literal nodes and validates timezone strings by trimming quotes and using PHP's native timezone validation.
tests/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/DateAddTest.php (1)
38-45: Test for invalid timezone correctly validates exception behavior.The test properly checks that an invalid timezone throws the expected exception with the right error message.
src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Exception/InvalidTimezoneException.php (1)
1-33: Well-structured exception class with clear factory methods.This is a well-organized exception class that follows good PHP practices. The static factory methods provide contextual error messages that will help developers understand what went wrong with timezone validation.
A few small observations:
- Good use of extending the appropriate Doctrine
ConversionExceptionbase class- Factory methods follow a clean pattern with descriptive names
- Error messages are clear and include the relevant context
| <?php | ||
|
|
||
| declare(strict_types=1); | ||
|
|
||
| namespace Tests\MartinGeorgiev\Doctrine\ORM\Query\AST\Functions; | ||
|
|
||
| use Fixtures\MartinGeorgiev\Doctrine\Entity\ContainsDates; | ||
| use MartinGeorgiev\Doctrine\ORM\Query\AST\Functions\DateBin; | ||
|
|
||
| class DateBinTest extends TestCase | ||
| { | ||
| protected function getStringFunctions(): array | ||
| { | ||
| return [ | ||
| 'DATE_BIN' => DateBin::class, | ||
| ]; | ||
| } | ||
|
|
||
| protected function getExpectedSqlStatements(): array | ||
| { | ||
| return [ | ||
| 'bins by 15 minutes' => "SELECT date_bin('15 minutes', c0_.datetime1, '2001-02-16 20:05:00') AS sclr_0 FROM ContainsDates c0_", | ||
| 'bins by 1 hour' => "SELECT date_bin('1 hour', c0_.datetime1, '2001-02-16 00:00:00') AS sclr_0 FROM ContainsDates c0_", | ||
| ]; | ||
| } | ||
|
|
||
| protected function getDqlStatements(): array | ||
| { | ||
| return [ | ||
| 'bins by 15 minutes' => \sprintf("SELECT DATE_BIN('15 minutes', e.datetime1, '2001-02-16 20:05:00') FROM %s e", ContainsDates::class), | ||
| 'bins by 1 hour' => \sprintf("SELECT DATE_BIN('1 hour', e.datetime1, '2001-02-16 00:00:00') FROM %s e", ContainsDates::class), | ||
| ]; | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Test coverage should include additional use cases based on PostgreSQL documentation.
The test covers basic functionality for the DATE_BIN() function with two test cases (15 minutes and 1 hour binning), but would benefit from more comprehensive coverage based on PostgreSQL documentation.
Consider adding the following test cases to ensure comprehensive coverage:
- Binning with a timestamp literal instead of a field
- Edge cases like binning with small intervals (e.g., seconds)
- Test with different origin values and their effects on binning
- Different interval formats accepted by PostgreSQL
According to the PostgreSQL documentation, date_bin() has specific behavior when changing the origin parameter, which affects where the bins are anchored. Adding these test cases would ensure the function behaves as expected across various scenarios.
protected function getExpectedSqlStatements(): array
{
return [
'bins by 15 minutes' => "SELECT date_bin('15 minutes', c0_.datetime1, '2001-02-16 20:05:00') AS sclr_0 FROM ContainsDates c0_",
'bins by 1 hour' => "SELECT date_bin('1 hour', c0_.datetime1, '2001-02-16 00:00:00') AS sclr_0 FROM ContainsDates c0_",
+ 'bins by 30 seconds with timestamp literal' => "SELECT date_bin('30 seconds', '2023-03-10 12:34:56', '2000-01-01 00:00:00') AS sclr_0 FROM ContainsDates c0_",
+ 'bins with different origin' => "SELECT date_bin('1 day', c0_.datetime1, '2023-01-01 12:00:00') AS sclr_0 FROM ContainsDates c0_",
];
}
protected function getDqlStatements(): array
{
return [
'bins by 15 minutes' => \sprintf("SELECT DATE_BIN('15 minutes', e.datetime1, '2001-02-16 20:05:00') FROM %s e", ContainsDates::class),
'bins by 1 hour' => \sprintf("SELECT DATE_BIN('1 hour', e.datetime1, '2001-02-16 00:00:00') FROM %s e", ContainsDates::class),
+ 'bins by 30 seconds with timestamp literal' => \sprintf("SELECT DATE_BIN('30 seconds', '2023-03-10 12:34:56', '2000-01-01 00:00:00') FROM %s e", ContainsDates::class),
+ 'bins with different origin' => \sprintf("SELECT DATE_BIN('1 day', e.datetime1, '2023-01-01 12:00:00') FROM %s e", ContainsDates::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.
| <?php | |
| declare(strict_types=1); | |
| namespace Tests\MartinGeorgiev\Doctrine\ORM\Query\AST\Functions; | |
| use Fixtures\MartinGeorgiev\Doctrine\Entity\ContainsDates; | |
| use MartinGeorgiev\Doctrine\ORM\Query\AST\Functions\DateBin; | |
| class DateBinTest extends TestCase | |
| { | |
| protected function getStringFunctions(): array | |
| { | |
| return [ | |
| 'DATE_BIN' => DateBin::class, | |
| ]; | |
| } | |
| protected function getExpectedSqlStatements(): array | |
| { | |
| return [ | |
| 'bins by 15 minutes' => "SELECT date_bin('15 minutes', c0_.datetime1, '2001-02-16 20:05:00') AS sclr_0 FROM ContainsDates c0_", | |
| 'bins by 1 hour' => "SELECT date_bin('1 hour', c0_.datetime1, '2001-02-16 00:00:00') AS sclr_0 FROM ContainsDates c0_", | |
| ]; | |
| } | |
| protected function getDqlStatements(): array | |
| { | |
| return [ | |
| 'bins by 15 minutes' => \sprintf("SELECT DATE_BIN('15 minutes', e.datetime1, '2001-02-16 20:05:00') FROM %s e", ContainsDates::class), | |
| 'bins by 1 hour' => \sprintf("SELECT DATE_BIN('1 hour', e.datetime1, '2001-02-16 00:00:00') FROM %s e", ContainsDates::class), | |
| ]; | |
| } | |
| } | |
| <?php | |
| declare(strict_types=1); | |
| namespace Tests\MartinGeorgiev\Doctrine\ORM\Query\AST\Functions; | |
| use Fixtures\MartinGeorgiev\Doctrine\Entity\ContainsDates; | |
| use MartinGeorgiev\Doctrine\ORM\Query\AST\Functions\DateBin; | |
| class DateBinTest extends TestCase | |
| { | |
| protected function getStringFunctions(): array | |
| { | |
| return [ | |
| 'DATE_BIN' => DateBin::class, | |
| ]; | |
| } | |
| protected function getExpectedSqlStatements(): array | |
| { | |
| return [ | |
| 'bins by 15 minutes' => "SELECT date_bin('15 minutes', c0_.datetime1, '2001-02-16 20:05:00') AS sclr_0 FROM ContainsDates c0_", | |
| 'bins by 1 hour' => "SELECT date_bin('1 hour', c0_.datetime1, '2001-02-16 00:00:00') AS sclr_0 FROM ContainsDates c0_", | |
| 'bins by 30 seconds with timestamp literal' => "SELECT date_bin('30 seconds', '2023-03-10 12:34:56', '2000-01-01 00:00:00') AS sclr_0 FROM ContainsDates c0_", | |
| 'bins with different origin' => "SELECT date_bin('1 day', c0_.datetime1, '2023-01-01 12:00:00') AS sclr_0 FROM ContainsDates c0_", | |
| ]; | |
| } | |
| protected function getDqlStatements(): array | |
| { | |
| return [ | |
| 'bins by 15 minutes' => \sprintf("SELECT DATE_BIN('15 minutes', e.datetime1, '2001-02-16 20:05:00') FROM %s e", ContainsDates::class), | |
| 'bins by 1 hour' => \sprintf("SELECT DATE_BIN('1 hour', e.datetime1, '2001-02-16 00:00:00') FROM %s e", ContainsDates::class), | |
| 'bins by 30 seconds with timestamp literal' => \sprintf("SELECT DATE_BIN('30 seconds', '2023-03-10 12:34:56', '2000-01-01 00:00:00') FROM %s e", ContainsDates::class), | |
| 'bins with different origin' => \sprintf("SELECT DATE_BIN('1 day', e.datetime1, '2023-01-01 12:00:00') FROM %s e", ContainsDates::class), | |
| ]; | |
| } | |
| } |
There was a problem hiding this comment.
Are these really relevant test scenarios?
There was a problem hiding this comment.
@martin-georgiev Looking at the PostgreSQL documentation more carefully, I think the most relevant additional test case would be the one with a different origin value, as the origin parameter significantly affects where bin boundaries are created.
The timestamp literal test is less critical since the function is primarily translating DQL to SQL correctly, which is already demonstrated with field references. The smaller interval test (30 seconds) doesn't really test anything fundamentally different from the existing 15-minute test.
The existing tests already cover the basic functionality of the DATE_BIN() implementation. If you want to keep the test focused on the most essential scenarios, just adding a test with a different origin value would be the most valuable addition.
There was a problem hiding this comment.
How is "bins by 1 hour" any different from "bins with different origin"?
There was a problem hiding this comment.
@martin-georgiev You're absolutely right. Looking more closely at the existing test cases, there's no fundamental difference in what's being tested. Both cases already test the function's ability to handle different interval values and different origin timestamps:
- "bins by 1 hour": Uses '1 hour' interval with origin '2001-02-16 00:00:00'
- "bins with different origin" (suggested): Uses '1 day' interval with origin '2023-01-01 12:00:00'
The existing test cases already adequately cover the functionality by testing different intervals ('15 minutes' and '1 hour') and different origins. My suggested addition doesn't add any meaningful test coverage that isn't already present in your implementation.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
tests/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/DateAddTest.php (1)
21-29: Consider adding more test cases for different interval formats.While the current test cases cover the basic functionality, PostgreSQL supports various interval formats that could be tested:
- Fractional intervals (e.g., '1.5 days')
- Compound intervals (e.g., '1 day 2 hours')
- Negative intervals (e.g., '-1 day')
protected function getExpectedSqlStatements(): array { return [ 'adds 1 day with timezone' => "SELECT date_add(c0_.datetimetz1, '1 day', 'Europe/Sofia') AS sclr_0 FROM ContainsDates c0_", 'adds 2 hours with timezone' => "SELECT date_add(c0_.datetimetz1, '2 hours', 'UTC') AS sclr_0 FROM ContainsDates c0_", 'adds 3 days without timezone' => "SELECT date_add(c0_.datetimetz1, '3 days') AS sclr_0 FROM ContainsDates c0_", 'adds with WHERE clause' => "SELECT c0_.datetimetz1 AS datetimetz1_0 FROM ContainsDates c0_ WHERE date_add(c0_.datetimetz1, '1 day') = '2023-01-02 00:00:00'", + 'adds fractional interval' => "SELECT date_add(c0_.datetimetz1, '1.5 days') AS sclr_0 FROM ContainsDates c0_", + 'adds compound interval' => "SELECT date_add(c0_.datetimetz1, '1 day 2 hours') AS sclr_0 FROM ContainsDates c0_", + 'adds negative interval' => "SELECT date_add(c0_.datetimetz1, '-1 day') AS sclr_0 FROM ContainsDates c0_", ]; }Corresponding DQL changes would also be needed in the
getDqlStatements()method.tests/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/DateSubtractTest.php (1)
21-28: Consider adding more test cases for different interval formats.Similar to the suggestion for DateAddTest, consider adding test cases for:
- Fractional intervals (e.g., '1.5 days')
- Compound intervals (e.g., '1 day 2 hours')
- Negative intervals (e.g., '-1 day')
protected function getExpectedSqlStatements(): array { return [ 'subtracts 1 day with timezone' => "SELECT date_subtract(c0_.datetimetz1, '1 day', 'Europe/Sofia') AS sclr_0 FROM ContainsDates c0_", 'subtracts 2 hours with timezone' => "SELECT date_subtract(c0_.datetimetz1, '2 hours', 'UTC') AS sclr_0 FROM ContainsDates c0_", 'subtracts 3 days without timezone' => "SELECT date_subtract(c0_.datetimetz1, '3 days') AS sclr_0 FROM ContainsDates c0_", + 'subtracts fractional interval' => "SELECT date_subtract(c0_.datetimetz1, '1.5 days') AS sclr_0 FROM ContainsDates c0_", + 'subtracts compound interval' => "SELECT date_subtract(c0_.datetimetz1, '1 day 2 hours') AS sclr_0 FROM ContainsDates c0_", + 'subtracts negative interval' => "SELECT date_subtract(c0_.datetimetz1, '-1 day') AS sclr_0 FROM ContainsDates c0_", ]; }Corresponding DQL changes would also be needed in the
getDqlStatements()method.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
tests/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/DateAddTest.php(1 hunks)tests/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/DateBinTest.php(1 hunks)tests/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/DateSubtractTest.php(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/DateBinTest.php
🧰 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/DateAddTest.phptests/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/DateSubtractTest.php
🧬 Code Graph Analysis (2)
tests/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/DateAddTest.php (4)
src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/DateAdd.php (1)
DateAdd(24-45)src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Exception/InvalidArgumentForVariadicFunctionException.php (1)
InvalidArgumentForVariadicFunctionException(7-46)src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Exception/InvalidTimezoneException.php (1)
InvalidTimezoneException(14-33)tests/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/TestCase.php (5)
TestCase(15-120)getStringFunctions(79-82)getExpectedSqlStatements(87-87)getDqlStatements(92-92)buildEntityManager(116-119)
tests/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/DateSubtractTest.php (4)
src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/DateSubtract.php (1)
DateSubtract(24-45)src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Exception/InvalidArgumentForVariadicFunctionException.php (1)
InvalidArgumentForVariadicFunctionException(7-46)src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Exception/InvalidTimezoneException.php (1)
InvalidTimezoneException(14-33)tests/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/TestCase.php (5)
TestCase(15-120)getStringFunctions(79-82)getExpectedSqlStatements(87-87)getDqlStatements(92-92)buildEntityManager(116-119)
⏰ Context from checks skipped due to timeout of 90000ms (20)
- GitHub Check: sloth
- GitHub Check: PHP 8.4 + Doctrine ORM 3.0 + Doctrine Lexer latest
- GitHub Check: PHP 8.1 + Doctrine ORM 2.14 + Doctrine Lexer 1.2
- GitHub Check: PHP 8.4 + Doctrine ORM latest + Doctrine Lexer 3.0
- GitHub Check: PHP 8.4 + Doctrine ORM 2.18 + 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 latest
- GitHub Check: PHP 8.4 + Doctrine ORM latest + Doctrine Lexer 2.1
- GitHub Check: PHP 8.4 + Doctrine ORM 3.0 + Doctrine Lexer 3.0
- GitHub Check: PHP 8.3 + Doctrine ORM 3.0 + Doctrine Lexer 3.0
- GitHub Check: PHP 8.3 + Doctrine ORM 2.14 + Doctrine Lexer latest
- GitHub Check: PHP 8.3 + Doctrine ORM 2.14 + Doctrine Lexer 2.1
- GitHub Check: PHP 8.2 + Doctrine ORM 3.0 + Doctrine Lexer latest
- GitHub Check: PHP 8.2 + Doctrine ORM 2.14 + Doctrine Lexer latest
- GitHub Check: PHP 8.2 + Doctrine ORM latest + Doctrine Lexer 3.0
- GitHub Check: PHP 8.1 + Doctrine ORM 2.18 + Doctrine Lexer latest
- GitHub Check: PHP 8.2 + Doctrine ORM 2.18 + Doctrine Lexer 2.1
- GitHub Check: PHP 8.1 + Doctrine ORM 2.14 + Doctrine Lexer latest
- GitHub Check: PHP 8.1 + Doctrine ORM 3.0 + Doctrine Lexer 3.0
- GitHub Check: PHP 8.1 + Doctrine ORM 2.18 + Doctrine Lexer 3.0
🔇 Additional comments (4)
tests/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/DateAddTest.php (2)
12-67: Tests look comprehensive with good coverage of the DATE_ADD function.The test class provides thorough coverage for the DATE_ADD function implementation including:
- Basic functionality with various time intervals (days, hours)
- Support for timezone handling
- Usage in WHERE clauses
- Error handling for invalid arguments
The test cases align well with how PostgreSQL handles date/time operations with intervals, though implemented as a custom function.
41-48:✅ Verification successful
Verify test against PostgreSQL documentation for timezone validation.
This test correctly verifies the function throws an exception for invalid timezones. According to PostgreSQL documentation, timezone identifiers should be valid IANA timezone names, which aligns with the implementation here.
🌐 Web query:
What are the valid timezone formats in PostgreSQL?💡 Result:
PostgreSQL supports three primary formats for specifying time zones, each with distinct characteristics and use cases:
1. IANA Time Zone Names
These use the format
Area/Location(e.g.,America/New_York,Europe/Paris) and provide comprehensive historical and daylight savings time (DST) rules. They are stored in thepg_timezone_namessystem catalog and are the most reliable for accurate time zone conversions[1][2].
Example:SET TimeZone = 'Europe/Vienna'; SELECT TIMESTAMPTZ '2022-04-01 12:00:00 Europe/Vienna'; -- Accounts for DST shifts[1]2. Time Zone Abbreviations
Abbreviations like
CET(Central European Time) orPST(Pacific Standard Time) represent fixed offsets from UTC. However, they lack DST awareness and can be ambiguous (e.g.,ISTcould mean Irish, Israeli, or Indian time). Valid abbreviations are listed inpg_timezone_abbrevs[1][2][6].
Example:SELECT TIMESTAMP '2023-03-15 18:00:00-11' AT TIME ZONE 'CET'; -- Converts using abbreviation[2]3. POSIX-Style Specifications
These use formats like
UTC±Hor±HH:MM(e.g.,UTC+3,-04:30). Notably, POSIX defines offsets as hours west of GMT, soUTC+3becomes-03in PostgreSQL[4][6].
Example:SET TIME ZONE 'UTC+3'; -- Stored as UTC-03:00[4]Key Considerations:
- Ambiguity: Abbreviations like
CST(Central Standard Time) can conflict between regions (e.g., Australia vs. North America)[5][6].- Session Settings: Use
SET TimeZoneto define the session’s time zone, or override it withAT TIME ZONEin queries[1][4][7].- System Catalogs: Query
pg_timezone_namesfor IANA zones andpg_timezone_abbrevsfor abbreviations[1][3].For most applications, IANA time zone names are recommended due to their DST awareness and global consistency[1][2][7].
Citations:
- 1: https://www.cybertec-postgresql.com/en/time-zone-management-in-postgresql/
- 2: https://www.postgresql.org/docs/current/datatype-datetime.html
- 3: https://www.commandprompt.com/education/postgresql-timezone-explained-with-examples/
- 4: https://www.cockroachlabs.com/blog/time-data-types-postgresql/
- 5: https://www.postgresql.org/docs/7.2/timezones.html
- 6: https://www.postgresql.org/docs/8.1/datetime-keywords.html
- 7: https://www.dbvis.com/thetable/date-formatting-in-postgres-a-comprehensive-guide/
- 8: https://neon.tech/postgresql/postgresql-tutorial/postgresql-time
Verified: Test for Invalid Timezone Exception is Correct
The
test_invalid_timezone_throws_exception()test intests/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/DateAddTest.phpcorrectly expects anInvalidTimezoneExceptionwith the precise message when an invalid timezone string is provided. Our verification against PostgreSQL’s documentation confirms that valid time zone identifiers must follow the IANA naming convention (e.g.,Europe/Vienna), which means that"Invalid/Timezone"is indeed invalid. This behavior matches the intended implementation in the test. No changes are required at this time.tests/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/DateSubtractTest.php (2)
39-46: Error handling test looks good.The test for invalid timezone exception is properly implemented and validates the expected behavior according to PostgreSQL's timezone requirements.
48-64: Argument validation tests are thorough.The tests for argument validation ensure that the function correctly enforces its expected interface, requiring between 2 and 3 arguments. This is good practice for robust API design.
Summary by CodeRabbit
New Features
Tests
DATE_ADD,DATE_SUBTRACT, andDATE_BIN.