Nested Grouping (multiple column grouping) - Fully Working #522

ghiscoding · 2013-01-24T00:56:57Z

I would like to contribute my code to Slickgrid, after working intensely on this project I finally made a fully working Nested Grouping (multiple column grouping).

Notes / things to consider:

Almost everything had to be coded in recursive way.
The working example had to be drop to 5k rows instead of 50k, because of performance issue at 3 levels of grouping, going deeper in the amount of grouping is a killer since everytime you're going with an extra level this is going exponentially and it's a killer to refresh all the data with multiple groups. Since even collapsing/expanding need a data refresh, 50k is just way too much rows.
Collapsing a group had to be changed, the property of ".value()" is not more valid since that same value could exist (and does exist in the example), and so was replaced by a new property ".groupby()" this property holds the trace of group, that is the only way I found to properly collapse/expand a certain group at the proper group level. Example, inside the .html I had to replace this dataView.collapseGroup(0); with this dataView.collapseGroup("duration:0");
Since this is a huge change, 3 separates files were created, in case some bugs were to be found and emerge. If found fully stable, no more problems found on my side, it might be nice to rename the files and merge them into your original files.

Hopefully my code will be implemented with yours, I spent a lot time on time.
Thanks
Ghislain B.

Brief description of the project status

fixes of indentation problems

fixind indentation problems

mleibman · 2013-01-24T02:04:25Z

Something's off here. It shouldn't be that slow.

mleibman · 2013-01-24T02:09:59Z

I think the problem is that you're doing needless exponential work in extractGroups().

Looking at your core loop:

  for (var i = 0, l = rows.length; i < l; i++) {
    r = rows[i];
    val = (groupingGetterIsAFn[groupLevel]) ? groupingGetter[groupLevel](r) : r[groupingGetter[groupLevel]];
    val = val || 0;
    group = groupsByVal[val];
    if (!group) {
      group = new Slick.Group();
      group.count = 0;
      group.value = val;
      group.groupby = dataset.__group ? dataset.groupby+"-->"+groupingColumn+":"+val : groupingColumn+":"+val;
      group.level = groupLevel;
      group.rows = [];
      groupsLcl[groupsLcl.length] = group;
      groupsByVal[val] = group;
    }

    group.rows[group.count++] = r;

    // do we have more level of grouping? if yes go deeper by recursion until we resurface back to parent level 0
    if(groupLevel < groupingGetter.length-1) {
      groupLevel++;
      var grp = extractGroups(group, groupingGetter[groupLevel], groupLevel);
      group.groups = grp;   
      groupLevel--;
    }
  }

On every iteration, you're rescanning the group rows from scratch ignoring and overwriting the previous groups ("group.groups = grp"). Instead, you should do this sequentially. First, break up the passed in rows into groups. Next, if there's another grouping level, iterate over found groups and call extractGroups(group.rows) on its rows recursively.

ghiscoding · 2013-01-24T02:12:51Z

I know the performance is a killer and I mentioned it, I did it recursively, I couldn't find any other way of implementing it but I have the impression it's also exponential each time you pass a level in terms of performance. And I'm almost sure the problem is in the function "extractGroups()", it's most probably exponential.

That was the only way I could find without losing the reference to previous level of group, if you find another way, feel free to share because I passed a couple weeks on this issue. I seriously don't know how else to do it.

ghiscoding · 2013-01-24T02:13:32Z

Sorry I clicked by mistake on the close button... I've reopened it..

mleibman · 2013-01-24T02:14:24Z

I'm not sure if you responded after reading my first comment or both of them. I think my second comment explains where the problem is.

ghiscoding · 2013-01-24T02:16:30Z

Yes I saw while I was writing, though I seriously don't know how to tackle it. I did know the problem from the beginning, I mainly wanted it to work at first. If you could fix the code, I'll be more then happy... I did most of it already.

mleibman · 2013-01-24T02:23:11Z

What about the solution I proposed? Doing it sequentially?

function extractGroups() {

extract groups into "gropus" like we did before
if there's another level, iterate over "groups" and call extractGroups() on each "group[i].rows".
}

ghiscoding · 2013-01-24T02:28:05Z

That was my first approach though when I tried it that way, I was losing reference to the "group" object in which I want to add another "groups" into. I wanted to do it with 2 loops like you seem to mention, but the big thing is that I don't know which object I'm suppose to attach it anymore, I thought passing that reference into a string at first but I decided to stop since this was ugly code and error pron.

mleibman · 2013-01-24T02:37:56Z

I really don't get what you're saying.
What's broken if I do it this way? Seems functionally the same and works with 50k rows easily.

function extractGroups(dataset, groupingColumn, groupLevel) {
  var group;
  var val;
  var groupsLcl = [];
  var groupsByVal = [];
  var r;
  var rows = dataset.__group ? dataset.rows : dataset;

  for (var i = 0, l = rows.length; i < l; i++) {
    r = rows[i];
    val = (groupingGetterIsAFn[groupLevel]) ? groupingGetter[groupLevel](r) : r[groupingGetter[groupLevel]];
    val = val || 0;
    group = groupsByVal[val];
    if (!group) {
      group = new Slick.Group();
      group.count = 0;
      group.value = val;
      group.groupby = dataset.__group ? dataset.groupby+"-->"+groupingColumn+":"+val : groupingColumn+":"+val;
      group.level = groupLevel;
      group.rows = [];
      groupsLcl[groupsLcl.length] = group;
      groupsByVal[val] = group;
    }

    group.rows[group.count++] = r;       
  }

  // do we have more level of grouping? if yes go deeper by recursion until we resurface back to parent level 0
  if(groupLevel < groupingGetter.length-1) {
    for (var i = 0; i < groupsLcl.length; i++) {
      group = groupsLcl[i];
      group.groups = extractGroups(group, groupingGetter[groupLevel], groupLevel+1);
    }
  }

  return groupsLcl;
}

ghiscoding · 2013-01-24T02:51:17Z

Oh boy you made my day here, your code does seem to fix the exponential... Thanks for the optimization!!!
Could you merge your code fix with my code? oh by the way, I rename them into 3 separate files, now that you fixed it, did you want to rename them to original files (without the "file...-multigrouping" naming)?

I did try it with 50k and it's smooth like it's been since the beginning.. wow awesome.
If you do merge your fix, could you change the file to display 50k instead of 5k like I did... line 67 & 306 of the html file. Everything else is good

mleibman · 2013-01-24T03:01:55Z

To be honest, I don't feel confident merging this code. Since it's in all new files and there's no diff in the pull request, I can't even review it easily. As a first step, please clean up the code a bit by using the same formatting and spaces instead of tabs. Make the fix I suggested and clean up unnecessary code. Also, please merge the changes into the same files so that GitHub can display a diff.

ghiscoding · 2013-01-24T03:23:40Z

Ok I see, I'll rename the files though I'm not sure about the spaces instead of tabs as you mentioned... You want me to do 2 spaces instead of a tab in each line there is, is that it?
I can't see the difference in my editor, guess I'll have to restart from your original file to do that, I might end up finishing that all tomorrow

Thanks a lot for the code fix!!!

ghiscoding · 2013-01-24T04:04:14Z

There's something that will break a portion of your code, I don't know if you will accept it or not but before I go ahead with the changes, I need some feedback on this!!!

You handle your group by a property on the group object of ".value", in which you keep the name of the collapsed/expanded groups inside the "collapsedGroups" variable, that is working fine in 1 level like you always had but isn't working with multiple levels because of collision of possible names as for an example the duration is "0" and you save 0 inside the variable but then I add a second grouping and the % complete is also "0", you can see that it ain't working anymore..

So the way I fixed this issue is by creating a new property of ".grouping" and keeping a trace of the level I'm at, so instead of just keeping "0" in the "collapsedGroups" variable, I keep this instead: "duration:0-->percentComplete:0", this says that at that place the group is collapsed, so there's no collusion.

Example inside the html code:
Here the string is different...
BEFORE: dataView.collapseGroup(0); AFTER: dataView.collapseGroup("duration:0"); // closes the 1st group

and here the .value is replaced by .groupby
BEFORE with .value
function collapseAllGroups() {
dataView.beginUpdate();
for (var i = 0; i < dataView.getGroups().length; i++) {
dataView.collapseGroup(dataView.getGroups()[i].value);
}
dataView.endUpdate();
}

AFTER with .groupby
function collapseAllGroups() {
dataView.beginUpdate();
for (var i = 0; i < dataView.getGroups().length; i++) {
dataView.collapseGroup(dataView.getGroups()[i].groupby);
}
dataView.endUpdate();
}

This include the code fix with 50k rows

mleibman · 2013-01-24T21:37:48Z

That's one of the things that I think could use some work. The string concatenation-based ids for groups look less than ideal to me (if only for the fact that group by values may contain ":"). What about making .expandGroup() take in a variable list of parameters, one for each group level? I.e. .expandGroup(0) would expand the first level group with value 0 while .expandGroup(0, 'high') would expand the 'high' subgroup of the 0 group.

ghiscoding · 2013-01-25T03:53:06Z

Hmm I tried implementing it and was close to something, just to find out that the proposition you give does not holds enough information as for the full path of levels. When you say .expandGroup(0, 'high'), that could work but would have the effect of closing that same value in every other subgroup of the same level number, that's the main problem I had at the beginning, closing it in 1 group was reflected in other groups as well... That's why I did it with the string, like so "duration:0-->percentComplete:0" there is only 1 in the all dataset, even if there's ":" like you said it doesn't matter at all, I look for the complete string for comparison, I'm not parsing it or anything, so even if you get something like this "dura:tion:0-->percent:complete:0", you would still have only 1 possibility in the dataset. I seriously don't see a problem with the ":", that doesn't bother my string compare at all.

By the way, when you put in your code .expandGroup(0), you consider this a the value right? Or is it the index position of the first group? From the code, I think it's the value but just want to make sure... if it's the index, I can do that in my code too.

Any more suggestions?

This reverts commit 1efc84f.

This is working with a .groupby String to expand/collapse. Ex.: dataView.expandGroup("duration:10-->percentComplete:55");

ghiscoding · 2013-01-25T05:07:11Z

I just made a commit of my latest changes and cleanup you asked for, though it still has my own implementation with a string instead of value even if you are still a little concerned about it, for now that is my latest code which is fully working with 50k rows.

I'll wait for your suggestions if you have any...
Thanks
Ghislain

ghiscoding · 2013-01-29T23:06:19Z

Hey Michael, did you had a chance to look into the code change?
I would like to have your feedback on this if possible

mleibman · 2013-01-29T23:09:42Z

Sorry, been really busy.

On Tue, Jan 29, 2013 at 3:06 PM, Ghislain B. notifications@github.comwrote:

Hey Michael, did you had a chance to look into the code change?
I would like to have your feedback on this if possible

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/522#issuecomment-12864098.

Without this change, the demo was only expanding/collapsing the first level and not all levels of grouping.

mleibman · 2013-02-05T20:47:08Z

I'm very interested in adding this functionality, but we got a release at
work this week and I'm still swamped.

On Tue, Jan 29, 2013 at 3:09 PM, Michael Leibman
michael.leibman@gmail.comwrote:

Sorry, been really busy.

On Tue, Jan 29, 2013 at 3:06 PM, Ghislain B. notifications@github.comwrote:

Hey Michael, did you had a chance to look into the code change?
I would like to have your feedback on this if possible

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/522#issuecomment-12864098.

ghiscoding · 2013-02-05T20:57:57Z

No problem take the time it takes to review it and see if we need to change anything. No rush on my side, I already started using it as is on couple reports at work, it's working as expected and fast like it has always been. Even if there is changes at this point, it shouldn't be major anyway.

Your work is more important ;)

mleibman · 2013-02-27T04:54:10Z

There were too many changes I had to make, but also add a lot, so I started with your code and extended it - 90964ce.

ghiscoding · 2013-02-27T05:45:12Z

Ok well at least I made a big big contribution on adding that nice missing feature :)

After testing it, I see something that might be a little hiccup... You've added the total on last line of each row, it might be a little confusing at first, I figured it out after it was summing even previous groups.
The hiccup is not there though, if you choose 50k rows and click on the 3 level group, look on the last line (Group 14), the total doesn't match, though all the other group until Group 13 are ok...

As for the rest, it looks nice this way :)
Do you get trigger when the pull is close? I'm not sure you'll see my message or not..

mleibman · 2013-02-27T09:09:29Z

Indeed you did :)

Could you clarify what the problem you see is? Each group level can
independently specified totals, and some of the groupings demonstrate that.
I've added a default of 50 rows (with 50k and 500k as options) so it's
easier to see the groupings.

On Tue, Feb 26, 2013 at 9:45 PM, Ghislain B. notifications@github.comwrote:

Ok well at least I made a big big contribution on adding that nice missing
feature :)

After testing it, I see something that might be a little hiccup... You've
added the total on last line of each row, it might be a little confusing at
first, I figured it out after it was summing even previous groups.
The hiccup is not there though, if you choose 50k rows and click on the 3
level group, look on the last line (Group 14), the total doesn't match,
though all the other group until Group 13 are ok...

As for the rest, it looks nice this way :)
Do you get trigger when the pull is close? I'm not sure you'll see my
message or not..

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/522#issuecomment-14158215
.

ghiscoding · 2013-02-27T15:36:45Z

My bad, I got confused and didn't inspect properly, I thought the bottom totals were in fact a Grand Total incremented after each group, but in fact it just happen that the last group had a lot less in it's total...

I would recommend you update your README.md file as well to mention that new multi-level grouping with independent aggregators functionality.

As for the rest..Awesome!!!
The way you expended on my base brings a lot more flexibility and functionality, a lot more independent the way you've done... sweet
I believe you brought CSS styling per levels, and independent aggregators, is there something else I didn't see? :)

Thanks for implementing it Michael

mleibman · 2013-02-27T19:32:55Z

Thanks! I'll add a wiki page on it later when I have some more free time.

What's changed:

Added independent aggregators per level.
Added a setting to control how subgroups get aggregated
(aggregateChildGroups).
Added a setting to control expand/collapse on a level basis, plus
optimized memory usage for how expand/collapse state is tracked
(toggledGroups instead of collapsedGroups, XOR'ed with the
groupingInfo.collapsed). This also solves the problem when you collapse
all groups and change the filter to include more rows - if that adds a new
group, it won't be collapsed like it should.
Added collapseAllGroups(opt_level) and expandAllGroups(opt_level).
Changed the collapseGroup()/expandGroup() API to not rely on the user
being aware of the groupingKey logic.
Added a way to set CSS styling per level.
A bunch of code clean-ups, fixes and optimizations.

On Wed, Feb 27, 2013 at 7:36 AM, Ghislain B. notifications@github.comwrote:

My bad, I got confused and didn't inspect properly, I thought the bottom
totals were in fact a Grand Total incremented after each group, but in fact
it just happen that the last group had a lot less in it's total...

I would recommend you update your README.md file as well to mention that
new multi-level grouping with independent aggregators functionality.

As for the rest..Awesome!!!
The way you expended on my base brings a lot more flexibility and
functionality, a lot more independent the way you've done... sweet
I believe you brought CSS styling per levels, and independent aggregators,
is there something else I didn't see? :)

Thanks for implementing it Michael

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/522#issuecomment-14180014
.

Based on the original pull request (mleibman#522) by ghiscoding. Deprecated DataVIew APIs (will continue to work): - .groupBy() - .setAggregators() New DataView APIs: - .getGrouping() - .setGrouping(groupingInfo) - .setGrouping([groupingInfo1, groupingInfo2, ...]) - .collapseAllGroups() - .collapseAllGroups(level) - .expandAllGroups() - .expandAllGroups(level) - .collapseGroup(groupingKey) - .collapseGroup(level1value, level2value, ...) - .expandGroup(groupingKey) - .expandGroup(level1value, level2value, ...) Grouping info options (for use in .setGrouping() calls): - getter - formatter - comparer - aggregators - aggregateCollapsed - aggregateChildGroups - collapsed New Group fields: - level - groups - groupingKey Also fixed 0-handling in default aggregators.

When frozen columns is enabled the height of the header row is missing in the height calculations. This change calculates the values also in this specific case so they are available later.

ghiscoding added 11 commits December 21, 2012 17:48

Create example-multigrouping.html

3ec3489

Create slick.dataview-multigrouping.js

963a43a

Create slick.groupitemmetadataprovider-multigrouping.js

c6e7199

Create README-NestedGrouping.md

d7553cb

Brief description of the project status

Update README-NestedGrouping.md

708fe90

Update README-NestedGrouping.md

f5b9015

Expanding/Collapsing is partly working

cfd8181

Fully working Nested Grouping

dad493f

Fully working Nested Grouping

6faba02

Fully working Nested Grouping

dacb165

fixes of indentation problems

Fully working Nested Grouping

fde4ff4

fixind indentation problems

ghiscoding closed this Jan 24, 2013

ghiscoding reopened this Jan 24, 2013

Nested Grouping

1efc84f

This include the code fix with 50k rows

Revert "Nested Grouping"

5193d29

This reverts commit 1efc84f.

Nested Grouping with 50k rows

4742ecf

This is working with a .groupby String to expand/collapse. Ex.: dataView.expandGroup("duration:10-->percentComplete:55");

Nested Grouping - Fixed expand/collapse on all levels

c282e9e

Without this change, the demo was only expanding/collapsing the first level and not all levels of grouping.

mleibman closed this Feb 27, 2013

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Nested Grouping (multiple column grouping) - Fully Working #522

Nested Grouping (multiple column grouping) - Fully Working #522

ghiscoding commented Jan 24, 2013

mleibman commented Jan 24, 2013

mleibman commented Jan 24, 2013

ghiscoding commented Jan 24, 2013

ghiscoding commented Jan 24, 2013

mleibman commented Jan 24, 2013

ghiscoding commented Jan 24, 2013

mleibman commented Jan 24, 2013

ghiscoding commented Jan 24, 2013

mleibman commented Jan 24, 2013

ghiscoding commented Jan 24, 2013

mleibman commented Jan 24, 2013

ghiscoding commented Jan 24, 2013

ghiscoding commented Jan 24, 2013

mleibman commented Jan 24, 2013

ghiscoding commented Jan 25, 2013

ghiscoding commented Jan 25, 2013

ghiscoding commented Jan 29, 2013

mleibman commented Jan 29, 2013

mleibman commented Feb 5, 2013

ghiscoding commented Feb 5, 2013

mleibman commented Feb 27, 2013

ghiscoding commented Feb 27, 2013

mleibman commented Feb 27, 2013

ghiscoding commented Feb 27, 2013

mleibman commented Feb 27, 2013

Nested Grouping (multiple column grouping) - Fully Working #522

Nested Grouping (multiple column grouping) - Fully Working #522

Conversation

ghiscoding commented Jan 24, 2013

mleibman commented Jan 24, 2013

mleibman commented Jan 24, 2013

ghiscoding commented Jan 24, 2013

ghiscoding commented Jan 24, 2013

mleibman commented Jan 24, 2013

ghiscoding commented Jan 24, 2013

mleibman commented Jan 24, 2013

ghiscoding commented Jan 24, 2013

mleibman commented Jan 24, 2013

ghiscoding commented Jan 24, 2013

mleibman commented Jan 24, 2013

ghiscoding commented Jan 24, 2013

ghiscoding commented Jan 24, 2013

mleibman commented Jan 24, 2013

ghiscoding commented Jan 25, 2013

ghiscoding commented Jan 25, 2013

ghiscoding commented Jan 29, 2013

mleibman commented Jan 29, 2013

mleibman commented Feb 5, 2013

ghiscoding commented Feb 5, 2013

mleibman commented Feb 27, 2013

ghiscoding commented Feb 27, 2013

mleibman commented Feb 27, 2013

ghiscoding commented Feb 27, 2013

mleibman commented Feb 27, 2013