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

Support Postgres Interval, Range, Bit, and BitVarying #2010

Merged
merged 3 commits into from
Sep 26, 2024

Conversation

alex-tan
Copy link
Contributor

@alex-tan alex-tan commented Sep 3, 2024

Motivation

Adds support for this column type.

Implementation

I followed the other implementations of the postgres column types.

Tests

No tests since there's not an established way of doing that in the repo for this type of feature.

@alex-tan alex-tan requested a review from a team as a code owner September 3, 2024 12:41
Copy link
Contributor

@amomchilov amomchilov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

Are there any other PG column types that we should add here?

@amomchilov amomchilov added the enhancement New feature or request label Sep 4, 2024
@alex-tan
Copy link
Contributor Author

alex-tan commented Sep 4, 2024

👍

I took a cursory look based on this and found that these were T.untyped in the rbis:

Postgres Type Rails Type
bit String
bit varying String
point ActiveRecord::Point

I can add them to this PR.

I didn't dig far enough to see if these would work already or not: hstore, legacy_point, vector

@alex-tan
Copy link
Contributor Author

alex-tan commented Sep 4, 2024

I tested these three new columns however ActiveRecord::Point doesn't resolve after the rbis are generated. Is there a place I should add a shim for that?

@alex-tan alex-tan changed the title Support PostgreSQL Interval Support Postgres Interval, Bit, BitVarying, and Point Sep 4, 2024
@amomchilov
Copy link
Contributor

@alex-tan It seems to fall under this category, so perhaps you can just declare it in activerecord_postgresql.rbi as well?

# These constants are dynamically loaded only when connecting to postgresql

class Point < Struct.new(:x, :y)
   sig { returns(FloatButImNotSure) } # TODO: Fill in correct types
   def x; end
   
   sig { params(x:FloatButImNotSure).returns(FloatButImNotSure) }
   def x=(x); end
   
   sig { returns(FloatButImNotSure) }
   def y; end

   sig { params(x:FloatButImNotSure).returns(FloatButImNotSure) }
   def y=(x); end
end

@alex-tan
Copy link
Contributor Author

alex-tan commented Sep 4, 2024

But that file doesn't end up in the shims for the project using tapioca right?

@amomchilov
Copy link
Contributor

Ahh, you're right, these are just shims for Tapioca's own internal use.

Hmmm... I'll ask for guidance internally. To get you unblocked quicker, would you like to roll this PR back to just the original support for Interval? I'll approve/merge that right away, then you can split the rest in their own new PR

@alex-tan
Copy link
Contributor Author

alex-tan commented Sep 4, 2024

Sounds good, thanks 👍

@alex-tan alex-tan changed the title Support Postgres Interval, Bit, BitVarying, and Point Support Postgres Interval Sep 4, 2024
auto-merge was automatically disabled September 4, 2024 17:24

Head branch was pushed to by a user without write access

@alex-tan
Copy link
Contributor Author

I went back and added Bit/BitVarying support again and also support for Range. It looks like there's a PR out to fix CI so I'll rebase when that gets merged.

@alex-tan alex-tan changed the title Support Postgres Interval Support Postgres Interval, Range, Bit, and BitVarying Sep 16, 2024
@egiurleo
Copy link
Contributor

Hi, @alextan! We just fixed CI on Ruby 3.3.5. Would you mind rebasing on main and force pushing your branch?

@alex-tan
Copy link
Contributor Author

@egiurleo sure thing, done 👍

@egiurleo egiurleo merged commit 44a27cb into Shopify:main Sep 26, 2024
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants