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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Distinct modifier support for aggregate functions #1046

Merged
merged 1 commit into from Nov 9, 2015

Conversation

Projects
None yet
5 participants
@ErisDS
Contributor

ErisDS commented Nov 4, 2015

This adds support for count(distinct *) as discussed in #1028.

I have also added support for avg(distinct *) and sum(distinct *) as both of these seemed valuable, I have not added distinct support for min and max as I don't think that adding distinct to those aggregates can impact the outcome. If it is felt that these should be added, I'm more than happy to update the PR.

There may be some confusion here between the standard distinct expression, and the distinct expression that goes inside of an aggregate function. I'm not sure whether to refer to it as 'inner distinct' or perhaps 'aggregate distinct'. When building the statement, I used aggregateDistinct to differ from the standard distinct.

I should probably also rename both _distinct() and _distinctFlag to also clarify the distinction between the distincts (see what I did there? 馃槢) but wanted to ask if there were thoughts on the name - naming things is always the hardest part.

I have tested this locally against sqlite3 only.

More than happy to provide more tests or other updates as necessary.


closes #1028

  • add support for count(distinct *), avg(distinct *) and sum(distinct *)
  • min and max don't really make sense with distinct, so didn't add those
@elhigu

This comment has been minimized.

Collaborator

elhigu commented Nov 7, 2015

Looks perfect to me. Only thing I would have changed would have been to add changed code and autogenerated files in separate commits :) @rhys-vdw any comments to this one? It had good tests and implementation was pretty much the same with e.g. not modifier.

EDIT: Actually now I noticed that documentation of the new APIs is still missing from index.html

@ErisDS

This comment has been minimized.

Contributor

ErisDS commented Nov 8, 2015

I can always pull changes out into separate commits if that is desirable, however I think having separate commits for src and built code would be undesirable - it is not useful history & creates a point at which things will not work as you expect.

I've tried to implement this in an idiomatic way so it fits the library - I felt the naming was definitely confusing so I've updated that. These names are long, but other than calling it aggDistinct I wasn't sure where to go with it 馃榿 happy to change it.

Also updated the docs, good shout @elhigu. Also happy to modify / improve them if anyone has a better idea.

@@ -26,6 +26,7 @@ function Builder(client) {
this._joinFlag = 'inner';
this._boolFlag = 'and';
this._notFlag = false;
this._aggregateDistinctFlag = false;

This comment has been minimized.

@rhys-vdw

rhys-vdw Nov 9, 2015

Collaborator

No need to store this on QueryBuilder instance.

@@ -565,6 +566,21 @@ assign(Builder.prototype, {
return this._aggregate('avg', column);
},
// Retrieve the "count" of the distinct results of the query.
countDistinct: function countDistinct() {
return this._aggregateDistinct(true).count.apply(this, arguments);

This comment has been minimized.

@rhys-vdw

rhys-vdw Nov 9, 2015

Collaborator

Instead call return this._aggregate('sum', column, true), and have _aggregate take a distinct flag.

@@ -742,6 +758,17 @@ assign(Builder.prototype, {
return ret;
},
// Helper to get or set the "distinctFlag" value.
_aggregateDistinct: function _aggregateDistinct(val) {

This comment has been minimized.

@rhys-vdw

rhys-vdw Nov 9, 2015

Collaborator

No longer necessary.

@rhys-vdw

This comment has been minimized.

Collaborator

rhys-vdw commented Nov 9, 2015

@ErisDS @elhigu: Having generated code in the repository at all is undesirable. Don't worry about it. The solution is to add it to the .gitignore, but there will have to be some tweaks to the release script first.

Sorry for the wait, @ErisDS. I approve of the API change.

One thing to change in the implementation before I can merge:

Instead of adding an _aggregateDistinct(), just pass an extra argument directly to _aggregate.

  // Retrieve the sum of the distinct values of a given column.
  sumDistinct: function(column) {
    return this._aggregate('sum', column, true);
  },
Distinct modifier support for aggregate functions
closes #1028

- add support for count(distinct *), avg(distinct *) and sum(distinct *)
- min and max don't really make sense with distinct, so didn't add those
@ErisDS

This comment has been minimized.

Contributor

ErisDS commented Nov 9, 2015

I have updated this PR with the suggested changes. Reduced surface area 馃憤

I think whether or not generated code should be checked in depends a lot on the project and how it works - there are pros and cons from a git perspective. In this case, as the documentation in the repo depends on the generated code, I don't think it's so bad to have it checked in. I didn't realise how the docs worked at first - but actually, writing docs is a great extra check that your code does what you expect - in fact, the docs almost work like a REPL, very handy.

May I (in a separate PR) update the contribution guidelines with what I learned about the documentation, and that an update there is needed when making api changes?

rhys-vdw added a commit that referenced this pull request Nov 9, 2015

Merge pull request #1046 from ErisDS/issue-1028
Distinct modifier support for aggregate functions

@rhys-vdw rhys-vdw merged commit bd97610 into tgriesser:master Nov 9, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@rhys-vdw

This comment has been minimized.

Collaborator

rhys-vdw commented Nov 9, 2015

I have updated this PR with the suggested changes.

Great, thanks for that.

I think whether or not generated code should be checked in depends a lot on the project and how it works - there are pros and cons from a git perspective.

The included knex.min.js script matters in the gh-pages branch, so that can be worked around. Docs, as you are obviously aware, are not generated content. I think the docs would be the same other than missing the script outside of gh-pages.

May I (in a separate PR) update the contribution guidelines with what I learned about the documentation, and that an update there is needed when making api changes?

Yes, please. 馃憤

@ErisDS ErisDS deleted the ErisDS:issue-1028 branch Nov 9, 2015

@ErisDS

This comment has been minimized.

Contributor

ErisDS commented Nov 9, 2015

Another reason why it may be desirable to keep the generated files in git - I just changed my package.json file to "knex": "git://github.com/tgriesser/knex.git#bd97610" to check if this fixes an issue I'm encountering in my project (it doesn't... that's another story).

I know there are both pros and cons to keeping built files in the repo, just wanted to suggest that if it isn't causing major headaches (I don't know if it is or isn't ), changing it may result in some you aren't expecting 馃槈

@rhys-vdw

This comment has been minimized.

Collaborator

rhys-vdw commented Nov 9, 2015

@ErisDS That case can be addressed with an install script in package.json.

@rhys-vdw

This comment has been minimized.

Collaborator

rhys-vdw commented Nov 9, 2015

I've done a bit of research on the matter recently as I just made this change to Bookshelf. I'm fairly convinced it's the right way. Note that the generated code is not necessarily correct at each commit - so keeping lib/ under version control could be more problematic than you realize. Also nobody wants to be solving merge conflicts in compiled output.

@tgriesser

This comment has been minimized.

Owner

tgriesser commented Nov 10, 2015

I think that having /src with a postinstall script as react-router does it will be the way to go here, https://github.com/rackt/react-router/blob/620d2703b6314a03b8a54a1fccaac097d436567a/npm-scripts/postinstall.js that way if you have a copy off of github it should also work so long as you have babel installed.

@rhys-vdw

This comment has been minimized.

Collaborator

rhys-vdw commented Nov 10, 2015

@tgriesser Problem here is that the global version of babel may be incorrect. IMO it's totally valid to call npm install babel in the postinstall script and use the devDependencies version of Babel. Babel has just undergone some massive changes so you can't rely on configuration being compatible across the board.

I've been advised that the publish script is technically the correct place for this. It is also run on install, but will run before a release.

@rhys-vdw

This comment has been minimized.

Collaborator

rhys-vdw commented Nov 10, 2015

Here is how I handled it in Bookshelf. Still requires some work.

@ricardogama

This comment has been minimized.

ricardogama commented Nov 19, 2015

@tgriesser @rhys-vdw This feature is awesome, when can we expect a new release?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment