Skip to content
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

Fix BC year handling in postgres #15521

Merged
merged 1 commit into from Jun 5, 2014

Conversation

nanaya
Copy link
Contributor

@nanaya nanaya commented Jun 5, 2014

Current method is broken: it simply converts astronomical year (what ruby uses) to BC era year (what postgres uses) even though they're off by one, starts from 1, and year 0 AD does not exist which causes postgres to return error.

  • 0000-01-01 => 0001-01-01 BC
  • 0000-02-29 => 0001-02-29 BC

For example, just try searching any model's created_at with Date.new(0) (0001-01-01 BC) or Date.new(-4,2,29) (0005-02-29 BC).

@nanaya
Copy link
Contributor Author

nanaya commented Jun 5, 2014

Sorry, I made several mistakes and triggered many CI builds.

@matthewd
Copy link
Member

matthewd commented Jun 5, 2014

LGTM; can you add some tests to ensure it stays fixed? 😄

BC era year is (astronomical year + 1) and starts from 1 BC.
@nanaya
Copy link
Contributor Author

nanaya commented Jun 5, 2014

Test added. The fix feels really ugly though.

Ideally:

  • assignment using something like TO_TIMESTAMP($date, 'YYYY-MM-DD')
  • and type casting done using something that actually understands BC date string format (or alternatively somehow tell postgres to use astronomical year)

@matthewd
Copy link
Member

matthewd commented Jun 5, 2014

I think it's fine; casting & quoting is the right place to translate between two different calendar systems.

matthewd added a commit that referenced this pull request Jun 5, 2014
@matthewd matthewd merged commit e496a25 into rails:master Jun 5, 2014
@nanaya nanaya deleted the fix_bc_postgres_master branch June 5, 2014 14:09
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.

None yet

2 participants