Fix Snowflake migrations for #2113#2224
Conversation
scooby359
commented
Jan 20, 2026
scooby359
commented
- Reverts changes made in Fix Snowflake CurrentDateTime to return TIMESTAMP_NTZ compatible value #2149, restoring mapping for SystemMethods.CurrentDateTime back to CURRENT_TIMESTAMP()
- Map SystemMethods.CurrentUTCDateTime to SYSDATE() in the Snowflake generator.
- Updated all related unit tests.
Changed mapping for SystemMethods.CurrentDateTime to CURRENT_TIMESTAMP() and SystemMethods.CurrentUTCDateTime to SYSDATE() in the Snowflake generator. Updated all related unit tests.
|
Fix for #2113 Haven't contributed to this project before and was looking for contribution guidelines for the project, but all links I could find are broken. I've reverted the previous changes made in error, applied the fix as identified, and updated tests. |
There was a problem hiding this comment.
Pull request overview
This pull request reverts changes from PR #2149 to fix Snowflake migrations related to issue #2113. The changes restore the mapping of SystemMethods.CurrentDateTime to CURRENT_TIMESTAMP() and remap SystemMethods.CurrentUTCDateTime to SYSDATE(). However, this introduces a critical semantic issue with UTC datetime handling.
Changes:
- Restored
SystemMethods.CurrentDateTimemapping fromSYSDATE()back toCURRENT_TIMESTAMP()in the Snowflake quoter - Changed
SystemMethods.CurrentUTCDateTimemapping fromCONVERT_TIMEZONE('UTC',CURRENT_TIMESTAMP())toSYSDATE() - Updated all related unit tests to match the new function mappings
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/FluentMigrator.Runner.Snowflake/Generators/Snowflake/SnowflakeQuoter.cs | Reverted CurrentDateTime to CURRENT_TIMESTAMP() and mapped CurrentUTCDateTime to SYSDATE(), but this mapping is semantically incorrect as SYSDATE() returns local session time, not UTC |
| test/FluentMigrator.Tests/Unit/Generators/Snowflake/SnowflakeQuoterTests.cs | Updated test expectations to match new mappings, but now validates incorrect behavior for CurrentUTCDateTime |
| test/FluentMigrator.Tests/Unit/Generators/Snowflake/SnowflakeGeneratorTests.cs | Correctly updated test expectations for CurrentDateTime to use CURRENT_TIMESTAMP() |
| test/FluentMigrator.Tests/Unit/Generators/Snowflake/SnowflakeColumnTests.cs | Correctly updated test expectations for CurrentDateTime to use CURRENT_TIMESTAMP() |
|
@copilot In your review, did you consider the following facts:
The change from @scooby359 appears to be correct in light of the fact AppliedOn does not preserve the timezone information. While I agree that it should ideally capture the TimeZone, the next best alternative is that the application always writes the value in UTC time. Do you agree? |
|
@scooby359 It looks like I can't interact with Copilot on other people's PRs. But I think your analysis is correct, and my rebuttal to Copilot will merely serve as a log as to what I thought in case somehow we missed something else. Will publish 8.1 today. |
