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.

Sign up for GitHub

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

Jump to bottom

Add #count #137

Merged
merged 1 commit into from
Feb 9, 2018
Merged

Add #count #137

merged 1 commit into from
Feb 9, 2018

Conversation

maiha
Copy link
Contributor

@maiha maiha commented Feb 7, 2018

I need count anyway 😄

class User < Granite::ORM::Base
  adapter mysql
  primary id : Int32
end
% amber exec User.count
491_i64
Exec

Best regards,

robacarp
robacarp approved these changes Feb 8, 2018
View reviewed changes
Copy link
Member

@robacarp robacarp left a comment

Choose a reason for hiding this comment

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

thank you @maiha

spec/granite_orm/querying/count_spec.cr
count = {{ model_constant }}.count
count.should eq 2
end
end
Copy link
Member

Choose a reason for hiding this comment

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

❤️ this spec, thanks @maiha

src/granite_orm/querying.cr Outdated
@@ -92,6 +92,11 @@ module Granite::ORM::Querying
end
end

# count returns a count of all the records
def count : Int64
Copy link
Member

Choose a reason for hiding this comment

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

No objection here but why did you choose Int64?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was thinking about handling large tables like 10 billion data.

However, in this case, special queries such as CAST AS BIGINT may be required. In fact, there are no rare cases beyond Int32 at this moment.

So I'd like to modify it to use Int32. Thought?

Copy link
Member

Choose a reason for hiding this comment

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

I have no preference. I think Postgres might return a different type depending on how much data comes back, so that may be an issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, I will choose Int32 and fix it immediately when there is a problem! 😄

@drujensen drujensen merged commit 40b13eb into amberframework:master Feb 9, 2018
@robacarp robacarp added this to the v0.8.6 milestone Feb 22, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@robacarp robacarp robacarp approved these changes

@faustinoaq faustinoaq faustinoaq approved these changes

@drujensen drujensen drujensen approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v0.8.6
Development

Successfully merging this pull request may close these issues.
4 participants
@maiha @drujensen @robacarp @faustinoaq