Feature: quarterly interval in cohort #4269

[zjsun]

trafficstars

What type of PR is this? (check all applicable)

• [x] Feature

Description

Support quarterly interval in cohort #4269

Related Tickets & Documents

#4269

Mobile & Desktop Screenshots/Recordings (if there are UI changes)

[zjsun]

I'd never changed the source structure, only added some 'interval' related options like "quarterly" etc.

How could I pass the build and e2e-tests?

[kravets-levko]

How could I pass the build and e2e-tests?

You passed! 😅

[rafawendel]

Hi @zjsun thanks for your work here.

I took a look at the implementation of this feature and despite the categories showing up properly in the UI, the logic behind them seems to not work properly. That is, the quarters do not match with the monthly results. Please tell me if otherwise.

It is basically defined here (line 40 of prepareData.ts):

_.each(grouped, group => {
    if (previousDate !== null) {
      // @ts-expect-error ts-migrate(2571) FIXME: Object is of type 'unknown'.
      let diff = Math.abs(previousDate.diff(group.date, momentInterval[timeInterval]));
      while (diff > 1) {
        const row = [0];
        for (let stage = firstStage; stage <= lastStage; stage += 1) {
          // @ts-expect-error ts-migrate(2571) FIXME: Object is of type 'unknown'.
          row.push(group.values[stage] || 0);
        }
        data.push(row);
        // It should be diagonal, so decrease count of stages for each next row
        lastStage -= 1;
        diff -= 1;
      }
    }

I couldn't grasp immediately why it didn't work, so I can better investigate it soon, if you want to.

[zjsun]

I took a look at the implementation of this feature and despite the categories showing up properly in the UI, the logic behind them seems to not work properly. That is, the quarters do not match with the monthly results. Please tell me if otherwise.

Thanks for your reviewing. @rafawendel

When using quarterly interval option, The cohort date in the query result is based on the rule which is based on the first day of the quarter(which is mentioned here ), eg. 2019-01-01, 2019-04-01, 2019-07-01 ... etc.

FYI.

Here is my query result: result.json.zip

[Avey777]

It is already difficult to merge the code here. You have another option available: https://github.com/RedashCommunity/redash

[guidopetri]

@zjsun , thanks for the PR! We've updated a lot of things now that we're Community-driven so - if you're still interested in getting this merged - would you mind rebasing off master to re-run the CI?

We're trying to clean up our PR todo list, so if you're not interested, that's fine - we'll close the PR in about a week if we don't hear back. If you're interested in reopening the PR afterwards, we would also very much welcome that.

[codecov[bot]]

Codecov Report

Merging #5342 (5d39fcc) into master (4107265) will not change coverage. The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #5342   +/-   ##
=======================================
  Coverage   60.70%   60.70%           
=======================================
  Files         154      154           
  Lines       12624    12624           
  Branches     1716     1716           
=======================================
  Hits         7663     7663           
  Misses       4735     4735           
  Partials      226      226