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

Simplify count() behavior #201

Closed
benallfree opened this issue Oct 29, 2017 · 13 comments
Closed

Simplify count() behavior #201

benallfree opened this issue Oct 29, 2017 · 13 comments
Assignees

Comments

@benallfree
Copy link
Contributor

benallfree commented Oct 29, 2017

I wonder if the default behavior of count() can be simplified. Right now, we need to do

let count = (await User.query().where('is_active', '=', true).count('* as total'))[0].total

What if count(expression = null) means to use * as total and return [0].total from the call?

let count = await User.query().where('is_active', '=', true).count()

Seems like the vast majority of cases would use this simple form.

@benallfree benallfree changed the title Can we make count() behave better? Simplify count() behavior Oct 29, 2017
@thetutlage
Copy link
Member

thetutlage commented Oct 31, 2017

The array is required since you can have multiple values returned from a count query too. For example.

await User.query().count('* as total').avg('age as avgAge')

Returns

[ { total: 51, avgAge: 26.019607843137255 } ]

Assuming you are 51 rows in the database and also each user has an age.

@benallfree
Copy link
Contributor Author

benallfree commented Oct 31, 2017

Coming from Laravel https://laravel.com/docs/5.5/queries#aggregates

They would solve it as (rough syntax):

// Get multiple aggregates
await UserRatings.query().select('user_id, avg(rating)').groupBy('user_id').fetch()

// Get single aggregate
await UserRating.query().avg('rating')
await UserRating.query().where('user_id', '=', 42).avg('rating')

So here, all aggregates do an auto-fetch and return the single row result.

@thetutlage
Copy link
Member

Yeah but if you need 2 aggregates in the same query?

@benallfree
Copy link
Contributor Author

Is this coming from Knex?

@thetutlage
Copy link
Member

Yes it is

@benallfree
Copy link
Contributor Author

benallfree commented Oct 31, 2017

Ok then I think this goes too deep to modify count() if it's coming from knex. Also I see your point for more advanced use cases. Mainly it seems like a common case to want the straight row count from a query, just trying to figure out a clean way to offer it.

What about adding a countRows method that wraps the current query and returns a row count?

I don't know knex enough, but something like...

KnexQueryBuilder.prototype.countRows = async function () {
  return (await knex().from(this).count('* as total'))[0].total
}

@thetutlage
Copy link
Member

thetutlage commented Oct 31, 2017

Yeah we can do this, maybe rowsCount and it takes a parameter too, to tell us what to count.

KnexQueryBuilder.prototype.rowsCount = async function (field) {
  field = field || '*'
  return (await knex().from(this).count(`${field} as total`))[0].total
}

@benallfree
Copy link
Contributor Author

Assign to me I’ll do

@RomainLanz
Copy link
Member

Done 😄

@benallfree
Copy link
Contributor Author

#205

@RomainLanz
Copy link
Member

Closing since a PR is opened for this feature.

@OrkhanAlikhanov
Copy link

It seems Adonisjs 5 is missing this feature. There is no getCount on model queries. Is this planned to be added @thetutlage?

@vikrambiwal
Copy link

@thetutlage can you suggest for this?

sum(price*quantity)/sum(quantity) as average_price

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

No branches or pull requests

5 participants