feat: add support for formatting functions to_char, to_date, to_number, to_timestamp
#386
Conversation
…number', 'to_timestamp'
WalkthroughThis update introduces support for PostgreSQL data type formatting functions ( Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant DoctrineORM
participant DQLParser
participant ToChar/ToDate/ToNumber/ToTimestamp
participant PostgreSQL
User->>DoctrineORM: Write DQL query using TO_CHAR/TO_DATE/TO_NUMBER/TO_TIMESTAMP
DoctrineORM->>DQLParser: Parse DQL query
DQLParser->>ToChar/ToDate/ToNumber/ToTimestamp: Map DQL function to SQL function
ToChar/ToDate/ToNumber/ToTimestamp->>PostgreSQL: Execute SQL function (to_char, to_date, etc.)
PostgreSQL-->>ToChar/ToDate/ToNumber/ToTimestamp: Return formatted result
ToChar/ToDate/ToNumber/ToTimestamp-->>DoctrineORM: Return result
DoctrineORM-->>User: Provide query result
Suggested labels
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (5)
✨ Finishing Touches
🧪 Generate Unit Tests
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:
SupportNeed 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)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
|
@coderabbitai, what test scenarios (integration or unit) may be missing here? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain@martin-georgiev Let me analyze the current test coverage to identify potential gaps. Based on my analysis of the current test coverage, here are the potentially missing test scenarios: Missing Unit Test Scenarios:
Missing Integration Test Scenarios:
Recommended Additional Tests:// Example missing test scenarios:
// Error handling
'SELECT TO_DATE(c.text1, \'\') FROM ContainsTexts c' // Empty format
'SELECT TO_CHAR(null, \'DD/MM/YYYY\') FROM ContainsDates c' // Null input
// Complex patterns
'SELECT TO_CHAR(123456.789, \'$999G999D00\') FROM ContainsTexts c' // Currency
'SELECT TO_DATE(\'2023-12-31\', \'YYYY-MM-DD\') FROM ContainsTexts c' // ISO format
// Multiple functions
'SELECT TO_CHAR(TO_DATE(c.text1, \'DD Mon YYYY\'), \'YYYY-MM-DD\') FROM ContainsTexts c'The current test coverage provides a solid foundation, but adding these scenarios would significantly improve robustness and real-world applicability. 🏁 Script executed: Length of output: 12944 🏁 Script executed: Length of output: 4125 🏁 Script executed: Length of output: 17415 🏁 Script executed: Length of output: 1438 Based on my analysis of the current test coverage, here are the missing test scenarios that would significantly improve robustness: Critical Missing Unit Test Scenarios:1. Error Handling & Edge Cases:// Currently missing - these should be added to unit tests
'SELECT TO_DATE(e.text1, \'\') FROM ContainsTexts e' // Empty format string
'SELECT TO_CHAR(null, \'DD/MM/YYYY\') FROM ContainsDates e' // Null input values
'SELECT TO_NUMBER(e.text1, \'invalid\') FROM ContainsTexts e' // Invalid format patterns2. Complex Format Patterns:// Advanced TO_CHAR patterns not tested
'SELECT TO_CHAR(e.decimal1, \'$999G999D00\') FROM ContainsDecimals e' // Currency symbols
'SELECT TO_CHAR(e.datetime1, \'Day, DD Month YYYY\') FROM ContainsDates e' // Full names
'SELECT TO_CHAR(123, \'RN\') FROM ContainsDates e' // Roman numerals
// Advanced TO_DATE/TO_TIMESTAMP patterns
'SELECT TO_DATE(e.text1, \'YYYY-MM-DD HH24:MI:SS\') FROM ContainsTexts e' // ISO formats
'SELECT TO_TIMESTAMP(e.text1, \'DD/MM/YYYY HH24:MI:SS TZ\') FROM ContainsTexts e' // Timezone
// Advanced TO_NUMBER patterns
'SELECT TO_NUMBER(e.text1, \'9.99EEEE\') FROM ContainsTexts e' // Scientific notation3. Function Composition (Missing Entirely):// Nested function calls - not tested anywhere
'SELECT TO_CHAR(TO_DATE(e.text1, \'DD Mon YYYY\'), \'YYYY-MM-DD\') FROM ContainsTexts e'
'SELECT TO_NUMBER(TO_CHAR(e.decimal1, \'999D99\'), \'999D99\') FROM ContainsDecimals e'
'SELECT TO_CHAR(UPPER(e.text1), \'DD Mon YYYY\') FROM ContainsTexts e'Critical Missing Integration Test Scenarios:1. Real-world Edge Cases:// Date boundaries and special cases
$dql = "SELECT to_date('29 Feb 2024', 'DD Mon YYYY') as result FROM ..."; // Leap year
$dql = "SELECT to_timestamp('31 Dec 2023 23:59:59', 'DD Mon YYYY HH24:MI:SS') as result FROM ..."; // Year boundary
// Large numbers and precision
$dql = "SELECT to_number('999999999.999999', '999999999D999999') as result FROM ..."; // Precision limits
$dql = "SELECT to_char(-999999.99, 'S999G999D99') as result FROM ..."; // Negative large numbers2. Error Handling Integration Tests:// These should test actual PostgreSQL error behavior
public function test_todate_with_invalid_format(): void
{
$this->expectException(\Doctrine\DBAL\Exception::class);
$dql = "SELECT to_date('invalid', 'invalid_format') FROM ContainsJsons t WHERE t.id = 1";
$this->executeDqlQuery($dql);
}3. Null Handling:// Currently not tested - PostgreSQL behavior with nulls
$dql = "SELECT to_char(null, 'DD/MM/YYYY') as result FROM ContainsJsons t WHERE t.id = 1";
// Should expect null resultPattern Comparison with Existing Functions:Looking at
The new formatting functions should follow this same comprehensive pattern. Recommended Priority:
The current coverage provides a solid foundation, but these additions would bring the new functions up to the same quality standard as the existing well-tested functions in the codebase. |
|
@karpilin CodeRabbit suggested some more test scenarios. Some of them are rather rubbish, but there are also valid points that can improve the quality :) |
martin-georgiev
changed the title
to_char, to_date, to_number, to_timestamp
Adds more comprehensive tests for the `to_char` function, covering various data types and formats. This ensures the function correctly handles different scenarios, including numeric, timestamp, and interval types, and validates its behavior with valid and invalid inputs. Also adds tests for to_timestamp, to_date and to_number to check invalid inputs and formats
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/Unit/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/ToCharTest.php (1)
39-45: LGTM! Essential error handling validation.The exception test properly validates that the
TO_CHARfunction requires a format argument, which is critical for function correctness.Based on the PR objectives discussion about test coverage gaps, you might consider adding optional tests for:
- Invalid format strings (e.g., unsupported format characters)
- Complex format patterns (e.g., currency symbols, Roman numerals)
- Null input handling
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
fixtures/MartinGeorgiev/Doctrine/Entity/ContainsDates.php(1 hunks)src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/ToChar.php(1 hunks)tests/Integration/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/DateTestCase.php(2 hunks)tests/Integration/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/ToCharTest.php(1 hunks)tests/Integration/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/ToDateTest.php(1 hunks)tests/Integration/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/ToNumberTest.php(1 hunks)tests/Integration/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/ToTimestampTest.php(1 hunks)tests/Unit/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/ToCharTest.php(1 hunks)tests/Unit/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/ToDateTest.php(1 hunks)tests/Unit/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/ToNumberTest.php(1 hunks)tests/Unit/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/ToTimestampTest.php(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/ToChar.php
- tests/Unit/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/ToDateTest.php
- tests/Integration/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/ToCharTest.php
- tests/Unit/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/ToNumberTest.php
🧰 Additional context used
🧠 Learnings (4)
tests/Integration/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/ToTimestampTest.php (3)
Learnt from: martin-georgiev
PR: martin-georgiev/postgresql-for-doctrine#383
File: tests/Unit/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/RadiansTest.php:1-9
Timestamp: 2025-05-23T11:11:57.926Z
Learning: Tests in the `Tests\Unit\MartinGeorgiev\Doctrine\ORM\Query\AST\Functions` namespace extend a custom `TestCase` class from the same namespace (`Tests\Unit\MartinGeorgiev\Doctrine\ORM\Query\AST\Functions\TestCase`), rather than PHPUnit's TestCase directly, and therefore don't need an explicit import.
Learnt from: martin-georgiev
PR: martin-georgiev/postgresql-for-doctrine#318
File: tests/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/XmlAggTest.php:1-9
Timestamp: 2025-03-29T03:31:17.114Z
Learning: Tests in the `Tests\MartinGeorgiev\Doctrine\ORM\Query\AST\Functions` namespace extend a custom `TestCase` class from the same namespace (`Tests\MartinGeorgiev\Doctrine\ORM\Query\AST\Functions\TestCase`), rather than PHPUnit's TestCase, and therefore don't need an explicit import.
Learnt from: martin-georgiev
PR: martin-georgiev/postgresql-for-doctrine#318
File: tests/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/XmlAggTest.php:1-9
Timestamp: 2025-03-29T03:31:17.114Z
Learning: Tests in the `Tests\MartinGeorgiev\Doctrine\ORM\Query\AST\Functions` namespace extend a custom `TestCase` class from the same namespace (`Tests\MartinGeorgiev\Doctrine\ORM\Query\AST\Functions\TestCase`), not PHPUnit's TestCase, and therefore don't need an explicit import statement.
tests/Unit/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/ToCharTest.php (3)
Learnt from: martin-georgiev
PR: martin-georgiev/postgresql-for-doctrine#383
File: tests/Unit/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/RadiansTest.php:1-9
Timestamp: 2025-05-23T11:11:57.926Z
Learning: Tests in the `Tests\Unit\MartinGeorgiev\Doctrine\ORM\Query\AST\Functions` namespace extend a custom `TestCase` class from the same namespace (`Tests\Unit\MartinGeorgiev\Doctrine\ORM\Query\AST\Functions\TestCase`), rather than PHPUnit's TestCase directly, and therefore don't need an explicit import.
Learnt from: martin-georgiev
PR: martin-georgiev/postgresql-for-doctrine#318
File: tests/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/XmlAggTest.php:1-9
Timestamp: 2025-03-29T03:31:17.114Z
Learning: Tests in the `Tests\MartinGeorgiev\Doctrine\ORM\Query\AST\Functions` namespace extend a custom `TestCase` class from the same namespace (`Tests\MartinGeorgiev\Doctrine\ORM\Query\AST\Functions\TestCase`), rather than PHPUnit's TestCase, and therefore don't need an explicit import.
Learnt from: martin-georgiev
PR: martin-georgiev/postgresql-for-doctrine#318
File: tests/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/XmlAggTest.php:1-9
Timestamp: 2025-03-29T03:31:17.114Z
Learning: Tests in the `Tests\MartinGeorgiev\Doctrine\ORM\Query\AST\Functions` namespace extend a custom `TestCase` class from the same namespace (`Tests\MartinGeorgiev\Doctrine\ORM\Query\AST\Functions\TestCase`), not PHPUnit's TestCase, and therefore don't need an explicit import statement.
tests/Integration/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/ToDateTest.php (1)
Learnt from: martin-georgiev
PR: martin-georgiev/postgresql-for-doctrine#383
File: tests/Unit/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/RadiansTest.php:1-9
Timestamp: 2025-05-23T11:11:57.926Z
Learning: Tests in the `Tests\Unit\MartinGeorgiev\Doctrine\ORM\Query\AST\Functions` namespace extend a custom `TestCase` class from the same namespace (`Tests\Unit\MartinGeorgiev\Doctrine\ORM\Query\AST\Functions\TestCase`), rather than PHPUnit's TestCase directly, and therefore don't need an explicit import.
tests/Unit/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/ToTimestampTest.php (3)
Learnt from: martin-georgiev
PR: martin-georgiev/postgresql-for-doctrine#383
File: tests/Unit/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/RadiansTest.php:1-9
Timestamp: 2025-05-23T11:11:57.926Z
Learning: Tests in the `Tests\Unit\MartinGeorgiev\Doctrine\ORM\Query\AST\Functions` namespace extend a custom `TestCase` class from the same namespace (`Tests\Unit\MartinGeorgiev\Doctrine\ORM\Query\AST\Functions\TestCase`), rather than PHPUnit's TestCase directly, and therefore don't need an explicit import.
Learnt from: martin-georgiev
PR: martin-georgiev/postgresql-for-doctrine#318
File: tests/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/XmlAggTest.php:1-9
Timestamp: 2025-03-29T03:31:17.114Z
Learning: Tests in the `Tests\MartinGeorgiev\Doctrine\ORM\Query\AST\Functions` namespace extend a custom `TestCase` class from the same namespace (`Tests\MartinGeorgiev\Doctrine\ORM\Query\AST\Functions\TestCase`), rather than PHPUnit's TestCase, and therefore don't need an explicit import.
Learnt from: martin-georgiev
PR: martin-georgiev/postgresql-for-doctrine#318
File: tests/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/XmlAggTest.php:1-9
Timestamp: 2025-03-29T03:31:17.114Z
Learning: Tests in the `Tests\MartinGeorgiev\Doctrine\ORM\Query\AST\Functions` namespace extend a custom `TestCase` class from the same namespace (`Tests\MartinGeorgiev\Doctrine\ORM\Query\AST\Functions\TestCase`), not PHPUnit's TestCase, and therefore don't need an explicit import statement.
🧬 Code Graph Analysis (5)
fixtures/MartinGeorgiev/Doctrine/Entity/ContainsDates.php (1)
fixtures/MartinGeorgiev/Doctrine/Entity/Entity.php (1)
ORM(10-17)
tests/Integration/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/ToNumberTest.php (4)
src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/ToNumber.php (1)
ToNumber(12-20)tests/Unit/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/ToNumberTest.php (2)
ToNumberTest(11-41)getStringFunctions(13-18)tests/Integration/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/TextTestCase.php (1)
TextTestCase(9-48)tests/Integration/MartinGeorgiev/TestCase.php (1)
executeDqlQuery(236-250)
tests/Integration/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/ToTimestampTest.php (4)
src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/ToTimestamp.php (1)
ToTimestamp(12-20)tests/Unit/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/ToTimestampTest.php (2)
ToTimestampTest(11-41)getStringFunctions(13-18)tests/Integration/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/TextTestCase.php (1)
TextTestCase(9-48)tests/Integration/MartinGeorgiev/TestCase.php (1)
executeDqlQuery(236-250)
tests/Unit/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/ToCharTest.php (3)
fixtures/MartinGeorgiev/Doctrine/Entity/ContainsDates.php (1)
ORM(10-33)src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/ToChar.php (1)
ToChar(12-20)tests/Unit/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/TestCase.php (1)
buildEntityManager(115-118)
tests/Integration/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/ToDateTest.php (4)
src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/ToDate.php (1)
ToDate(12-20)tests/Unit/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/ToDateTest.php (2)
ToDateTest(11-41)getStringFunctions(13-18)tests/Integration/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/TextTestCase.php (1)
TextTestCase(9-48)tests/Integration/MartinGeorgiev/TestCase.php (1)
executeDqlQuery(236-250)
🔇 Additional comments (22)
fixtures/MartinGeorgiev/Doctrine/Entity/ContainsDates.php (1)
31-32:❓ Verification inconclusive
Verify the property type mapping for INTERVAL columns.
The
dateinterval1property is typed as\DateTimeImmutablebut annotated withTypes::DATEINTERVAL. PostgreSQLINTERVALtype represents duration/period (like '15h 2m 12s'), which semantically should map to PHP's\DateIntervalclass, not\DateTimeImmutable(which represents a point in time).Please verify this type mapping is correct and functional:
🏁 Script executed:
#!/bin/bash # Verify how Types::DATEINTERVAL is handled in Doctrine DBAL rg -A 10 -B 5 "DATEINTERVAL.*=.*" --type php rg -A 10 -B 5 "class.*DateIntervalType" --type phpLength of output: 99
🏁 Script executed:
#!/bin/bash # Search for the DATEINTERVAL constant definition in Doctrine DBAL rg "const\s\+DATEINTERVAL" -n # Search for the DateIntervalType class in the repository (including vendor) rg "class\s\+DateIntervalType" -nLength of output: 67
Verify Doctrine DBAL’s DATEINTERVAL mapping
I wasn’t able to locate any
Types::DATEINTERVALconstant orDateIntervalTypeclass in the checked codebase—likely because these live in your installed Doctrine DBAL dependency. Please confirm that in your DBAL version:•
Types::DATEINTERVALis defined in
vendor/doctrine/dbal/src/Types/Types.php
• It maps to aDateIntervalType(in
vendor/doctrine/dbal/src/Types/DateIntervalType.php)
whose PHP class handling uses\DateIntervaland not\DateTimeImmutable.If the mapping indeed uses
\DateInterval, you’ll need to change your entity property to:#[ORM\Column(type: Types::DATEINTERVAL)] public \DateInterval $dateinterval1;Otherwise, you’re risking a semantic mismatch between DB durations and PHP point-in-time objects.
tests/Integration/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/DateTestCase.php (2)
36-37: LGTM! Proper PostgreSQL INTERVAL column addition.The schema update correctly adds the
dateinterval1column with PostgreSQL'sINTERVALtype, which aligns with the entity property addition.
47-48: LGTM! Valid interval test data.The INSERT statement correctly includes the interval value
'15h 2m 12s'which is valid PostgreSQL interval syntax and provides good test data for the formatting functions.tests/Unit/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/ToCharTest.php (3)
1-11: LGTM! Proper imports and namespace setup.The file header, imports, and namespace declaration follow the established patterns for unit tests in this codebase.
12-19: LGTM! Correct function registration pattern.The
getStringFunctions()method properly registers theTO_CHARfunction following the established pattern used by other function tests in this codebase.
21-37: LGTM! Comprehensive DQL-to-SQL mapping tests.The test cases cover different data types (datetime, interval, decimal) with appropriate PostgreSQL
to_charformat strings. The DQL statements correctly map to the expected SQL output.tests/Integration/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/ToNumberTest.php (4)
11-18: LGTM! Function registration follows established pattern.The function registration correctly maps the DQL function name to the implementation class, consistent with other formatting function tests.
20-25: LGTM! Basic functionality test is well-structured.The test correctly validates the PostgreSQL
to_numberfunction with a properly formatted input string and verifies the expected numeric conversion result.
34-46: Error handling tests are comprehensive for type validation.Both tests correctly validate that the function properly handles wrong argument types (null format and numeric input) by expecting
QueryException, which is appropriate for DQL validation errors.
27-32:❓ Verification inconclusive
Inconsistent error handling compared to other formatting functions.
This test expects a generic
Exceptionfor invalid format strings, whileToDateTestandToTimestampTestdon't expect exceptions for invalid formats at all. This inconsistency suggests different PostgreSQL behavior or testing approaches that should be aligned.Verify PostgreSQL's actual behavior with invalid format strings across all formatting functions:
🏁 Script executed:
#!/bin/bash # Test PostgreSQL behavior with invalid format strings for all formatting functions # Create a temporary SQL file to test each function cat > test_invalid_formats.sql << 'EOF' \echo 'Testing TO_NUMBER with invalid format:' SELECT to_number('12,454.8-', 'invalid_format'); \echo 'Testing TO_DATE with invalid format:' SELECT to_date('05 Dec 2000', 'invalid_format'); \echo 'Testing TO_TIMESTAMP with invalid format:' SELECT to_timestamp('05 Dec 2000', 'invalid_format'); EOF echo "Execute this SQL against a PostgreSQL database to verify consistent error behavior:" cat test_invalid_formats.sqlLength of output: 509
🌐 Web query:
PostgreSQL behavior of to_number, to_date, and to_timestamp functions when provided an invalid format string💡 Result:
PostgreSQL's formatting functions to_number, to_date, and to_timestamp exhibit specific behaviors when facing invalid format strings or inputs that don't match the expected format. Understanding these behaviors is crucial for effective data conversion and error handling.
to_number Function Behavior
The to_number function converts string inputs to numeric values based on a specified format template. When encountering invalid formats or mismatches:
Format Mismatches
When the input string doesn't match the format template exactly, PostgreSQL may:
Skip digits if separators in the format aren't present in the input. For example,
to_number('34,50','999,99')returns3450because PostgreSQL ignores the separators[8].Return an error when the input string contains characters that can't be parsed according to the format. For instance,
to_numbercannot convert spaces or alphabets that aren't part of a recognized format pattern[9].Skip non-data characters in the input string when non-data template patterns (like
LorTH) are used. For example, theTHpattern would skip two non-data characters in the input string[1].Format String Restrictions
The format string in to_number has several constraints:
- It cannot be an empty string (
'')[4].- The input string must be shorter than or equal to the format string. For example,
to_number('123','99')will return an error because the input has three digits but the format specifies only two[4].- Special format patterns like
EEEEmust be the last pattern used in the format string[3].to_date and to_timestamp Functions Behavior
These functions convert strings to date/time values based on specified formats.
Whitespace Handling
By default, these functions are flexible with whitespace:
- They skip multiple blank spaces at the beginning of the input string and around date/time values[1].
- For example,
to_timestamp(' 2000 JUN', 'YYYY MON')works despite the leading space[1].- However, when the
FXoption is used as the first item in the template, exact matching is enforced. For instance,to_timestamp('2000 JUN', 'FXYYYY MON')returns an error because it expects only a single space[1].Separator Handling
The functions have specific rules for handling separators:
- A separator in the template matches any single separator in the input or can be skipped[1].
- For example, both
to_timestamp('2000JUN', 'YYYY///MON')andto_timestamp('2000/JUN', 'YYYY MON')work correctly[1].- If the number of separators in the input exceeds those in the template, an error occurs. For example,
to_timestamp('2000//JUN', 'YYYY/MON')returns an error[1].- With the
FXoption, a separator in the template must match exactly one character in the input, though it doesn't need to be the same character[1].Format String Issues
Specific issues can occur with format strings:
- When the format string has more whitespaces compared to the input string, unexpected results can occur[7].
- For example,
SELECT TO_TIMESTAMP('2016-06-13 15:43:36', 'YYYY/MM/DD HH24:MI:SS')can produce incorrect time values[7].- Time zone formats (
TZ,tz, orOF) are not supported as input in the to_timestamp function[10].Out-of-Range Values
For to_timestamp, there's an optional parameter to control error handling:
- The
is_strictparameter (when set to TRUE) causes the function to return an error if there are out-of-range values[10].- By default (
is_strictis FALSE), overflow values are accepted, potentially producing unexpected results[10].- For example,
TO_TIMESTAMP('2011-12-18 24:38:15', 'YYYY-MM-DD HH24:MI:SS')returns a timestamp for the next day due to hours exceeding 24, but withis_strictset to TRUE, it would return an error[10].Understanding these behaviors helps in writing robust code that handles data conversion edge cases appropriately in PostgreSQL applications.
Citations:
- 1: https://www.postgresql.org/docs/current/functions-formatting.html
- 2: https://www.postgresql.org/message-id/CAGMVOdvpbMqPf9XWNzOwBpzJfErkydr_fEGhmuDGa015z97mwg@mail.gmail.com
- 3: https://postgrespro.com/list/thread-id/2711978
- 4: https://docs.yellowbrick.com/6.7.1/ybd_sqlref/to_number.html
- 5: https://neon.tech/postgresql/postgresql-string-functions/postgresql-to_number
- 6: https://www.youtube.com/watch?v=CAmcUgvcPxc
- 7: https://postgrespro.com/list/thread-id/1894700
- 8: https://www.postgresql.org/message-id/CAEepm=2KXZ+8BdJsopK0Bq=GuJRsYnDbwqf0U_HsocN46xE05w@mail.gmail.com
- 9: https://forums.oracle.com/ords/apexds/post/to-number-function-and-the-invalid-number-error-0654
- 10: https://docs.aws.amazon.com/redshift/latest/dg/r_TO_TIMESTAMP.html
Ensure consistent exception expectations for invalid format strings
Based on PostgreSQL’s formatting-function behavior, an invalid format template will result in a runtime error for all three conversion functions. We should therefore align the tests so that
ToDateTestandToTimestampTestalso expect an exception when given an invalid format.• In
tests/Integration/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/ToDateTest.php, wrap the invalid‐format case with:$this->expectException(Exception::class);• In
tests/Integration/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/ToTimestampTest.php, do the same for its invalid‐format scenario.Please confirm by running these queries in your PostgreSQL environment and then update the two tests accordingly:
SELECT to_number('12,454.8-','invalid_format'); SELECT to_date('05 Dec 2000','invalid_format'); SELECT to_timestamp('05 Dec 2000','invalid_format');tests/Integration/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/ToDateTest.php (5)
11-18: LGTM! Function registration follows established pattern.The function registration correctly maps the DQL function name to the implementation class.
20-25: LGTM! Basic functionality test validates correct date conversion.The test correctly validates PostgreSQL's
to_datefunction converting a formatted date string to ISO date format.
27-32: Appropriate exception handling for invalid date input.Correctly expects
DriverExceptionwhen PostgreSQL cannot parse the date string, which is the appropriate exception type for database-level errors.
34-39: Verify PostgreSQL's behavior with invalid format strings.This test doesn't expect an exception for invalid format strings and asserts a specific fallback result ('2005-01-01'). This differs from
ToNumberTestwhich expects an exception. This inconsistency needs verification against actual PostgreSQL behavior.The behavior differs from
ToNumberTest.phpline 29 which expects an exception for invalid formats. Please verify if this reflects actual PostgreSQL behavior or if the tests should be consistent.
41-53: Error handling tests are comprehensive for type validation.Both tests correctly validate that the function handles wrong argument types by expecting
QueryException, which is appropriate for DQL validation errors.tests/Integration/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/ToTimestampTest.php (4)
11-18: LGTM! Function registration follows established pattern.The function registration correctly maps the DQL function name to the implementation class.
20-25: Excellent test with complex format pattern.The test uses a comprehensive format string that includes various PostgreSQL format elements (
DD Mon YYYY tt HH24:MI ttt SS ttttttt), providing good coverage of the function's formatting capabilities.
27-32: Appropriate exception handling for invalid timestamp input.Correctly expects
DriverExceptionwhen PostgreSQL cannot parse the timestamp string, consistent withToDateTest.
41-53: Error handling tests are comprehensive for type validation.Both tests correctly validate wrong argument types by expecting
QueryException, consistent with other formatting function tests.tests/Unit/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/ToTimestampTest.php (3)
11-18: LGTM! Function registration follows established pattern.The unit test correctly registers the
TO_TIMESTAMPfunction for DQL-to-SQL conversion testing.
20-32: LGTM! SQL generation tests are properly structured.The methods correctly define the expected SQL output and corresponding DQL input for validating the function's SQL generation, following the established unit test pattern in this codebase.
34-40: LGTM! Essential validation test for required parameters.The test correctly validates that missing the required format parameter throws a
QueryException, ensuring proper DQL validation. This aligns with the corresponding tests in other formatting function unit tests.
tests/Integration/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/ToTimestampTest.php
Show resolved
Hide resolved
|
@martin-georgiev Let me know if there are more tests that you want me to add |
There was a problem hiding this comment.
Generally, if a test scenario is expected to throw, it is more readable and maintainable to reflect that in the method name. For example, JsonbBuildObjectTest::throws_exception_when_odd_number_of_arguments_given. Can you update this in the proposed test method names?
src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/ToNumber.php
Outdated
Show resolved
Hide resolved
src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/ToTimestamp.php
Outdated
Show resolved
Hide resolved
| $this->executeDqlQuery($dql); | ||
| } | ||
|
|
||
| public function test_tochar_invalid_input(): void |
There was a problem hiding this comment.
| public function test_tochar_invalid_input(): void | |
| public function test_tochar_with_unsupported_null_input(): void |
| use MartinGeorgiev\Doctrine\ORM\Query\AST\Functions\ToChar; | ||
| use MartinGeorgiev\Doctrine\ORM\Query\AST\Functions\ToTimestamp; | ||
|
|
||
| class ToCharTest extends DateTestCase |
There was a problem hiding this comment.
This is not a pure DateTestCase; It's better when you extend from Tests\Integration\MartinGeorgiev\TestCase and add entirely new data fixtures population for the date and numerical values that you propose in this test. Otherwise, this seems out of place and dirty.
| $this->executeDqlQuery($dql); | ||
| } | ||
|
|
||
| public function test_todate_with_wrong_type_input(): void |
There was a problem hiding this comment.
Similar comment about the null value and naming as in the other place.
Updates the documentation URL to reflect the latest PostgreSQL documentation (version 17). Adds a `@since` tag to indicate the version in which these functions were introduced. Minor corrections to tests
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
tests/Integration/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/ToCharTest.php (1)
86-86: Rename the method to better reflect what it tests.The method name includes "unsupported" which might be misleading. Consider simplifying to
test_tochar_with_null_input.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/ToChar.php(1 hunks)src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/ToDate.php(1 hunks)src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/ToNumber.php(1 hunks)src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/ToTimestamp.php(1 hunks)tests/Integration/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/ToCharTest.php(1 hunks)tests/Integration/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/ToDateTest.php(1 hunks)tests/Integration/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/ToNumberTest.php(1 hunks)tests/Integration/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/ToTimestampTest.php(1 hunks)tests/Unit/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/ToCharTest.php(1 hunks)tests/Unit/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/ToDateTest.php(1 hunks)tests/Unit/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/ToNumberTest.php(1 hunks)tests/Unit/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/ToTimestampTest.php(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (11)
- src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/ToTimestamp.php
- src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/ToNumber.php
- src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/ToDate.php
- tests/Unit/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/ToCharTest.php
- tests/Unit/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/ToTimestampTest.php
- src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/ToChar.php
- tests/Unit/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/ToNumberTest.php
- tests/Unit/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/ToDateTest.php
- tests/Integration/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/ToNumberTest.php
- tests/Integration/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/ToTimestampTest.php
- tests/Integration/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/ToDateTest.php
🧰 Additional context used
🧠 Learnings (1)
tests/Integration/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/ToCharTest.php (2)
Learnt from: martin-georgiev
PR: martin-georgiev/postgresql-for-doctrine#383
File: tests/Unit/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/RadiansTest.php:1-9
Timestamp: 2025-05-23T11:11:57.951Z
Learning: Tests in the `Tests\Unit\MartinGeorgiev\Doctrine\ORM\Query\AST\Functions` namespace extend a custom `TestCase` class from the same namespace (`Tests\Unit\MartinGeorgiev\Doctrine\ORM\Query\AST\Functions\TestCase`), rather than PHPUnit's TestCase directly, and therefore don't need an explicit import.
Learnt from: martin-georgiev
PR: martin-georgiev/postgresql-for-doctrine#318
File: tests/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/XmlAggTest.php:1-9
Timestamp: 2025-03-29T03:31:17.114Z
Learning: Tests in the `Tests\MartinGeorgiev\Doctrine\ORM\Query\AST\Functions` namespace extend a custom `TestCase` class from the same namespace (`Tests\MartinGeorgiev\Doctrine\ORM\Query\AST\Functions\TestCase`), rather than PHPUnit's TestCase, and therefore don't need an explicit import.
🧬 Code Graph Analysis (1)
tests/Integration/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/ToCharTest.php (5)
fixtures/MartinGeorgiev/Doctrine/Entity/ContainsDates.php (1)
ORM(10-33)src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/ToChar.php (1)
ToChar(13-21)src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/ToTimestamp.php (1)
ToTimestamp(13-21)tests/Unit/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/ToCharTest.php (2)
ToCharTest(13-48)getStringFunctions(15-20)tests/Integration/MartinGeorgiev/TestCase.php (3)
executeDqlQuery(236-250)createTestSchema(149-154)dropTestTableIfItExists(204-211)
🔇 Additional comments (7)
tests/Integration/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/ToCharTest.php (7)
15-28: LGTM!The setup method properly initializes test data and the function registration is correct for the tests.
30-35: LGTM!The test correctly validates timestamp formatting with the 12-hour format pattern.
37-42: LGTM!The test properly validates interval formatting, correctly converting '15h 2m 12s' to '15:02:12'.
44-49: LGTM!The test correctly validates numeric formatting with the sign indicator ('S') placing the minus sign at the end for negative values.
51-63: LGTM!Both tests correctly validate numeric literal formatting, demonstrating how the 'S' format indicator displays '+' for positive and '-' for negative values at the end.
65-70: LGTM!Excellent test case demonstrating function composition with
to_char(to_timestamp(...)). This validates that the functions work correctly when nested.
93-123: LGTM!The method properly creates a standalone test table with appropriate date/time columns and test data, following the integration test pattern.
tests/Integration/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/ToCharTest.php
Outdated
Show resolved
Hide resolved
tests/Integration/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/ToCharTest.php
Outdated
Show resolved
Hide resolved
tests/Integration/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/ToCharTest.php
Outdated
Show resolved
Hide resolved
|
@martin-georgiev I think I addressed all of the points here. Please let me know if I missed any or further tweaks are necessary. |
tests/Integration/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/ToCharTest.php
Outdated
Show resolved
Hide resolved
tests/Integration/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/ToCharTest.php
Outdated
Show resolved
Hide resolved
tests/Integration/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/ToCharTest.php
Outdated
Show resolved
Hide resolved
tests/Unit/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/ToDateTest.php
Outdated
Show resolved
Hide resolved
tests/Unit/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/ToNumberTest.php
Outdated
Show resolved
Hide resolved
tests/Unit/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/ToTimestampTest.php
Outdated
Show resolved
Hide resolved
tests/Unit/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/ToCharTest.php
Outdated
Show resolved
Hide resolved
This pull request adds support for PostgreSQL data type formatting functions (
to_char,to_date,to_number, andto_timestamp) in the Doctrine ORM library. The changes include new function implementations, updates to documentation, and corresponding unit and integration tests.New Functionality: Data Type Formatting Functions
to_char,to_date,to_number, andto_timestampin thesrc/MartinGeorgiev/Doctrine/ORM/Query/AST/Functionsdirectory. These classes extendBaseFunctionand define the function prototypes and node mappings.Documentation Updates
README.mdto include "Data Type Formatting Functions" as a new category of supported features.docs/AVAILABLE-FUNCTIONS-AND-OPERATORS.md.Testing
Summary by CodeRabbit
New Features
Documentation
Tests