[BUG] aggregation day function incorrect output about redisearch HOT 5 CLOSED

Hi @GuyAv46 great work!! I have also tested your implementation on my instances, and it works like a charm.

from redisearch.

Yes, just looking at the fast_timegm() function, the issue might be here:

tdays = tdays + (tyears * 365) + leaps;

Because 2024 is a leap year, and leap year day happens on the 29th Feb. However, the algorithm is very crude and just adds a day for the year, which means the output will be incorrect from Jan 1 to Feb 28, then presumably correct again after Mar 1.

from redisearch.

As a quick test, I made this patch:

...
  tdays += ltm->tm_mday - 1;  // days of month passed.

  if ((tyears + 2) % 4 == 0) {
    if (tdays <= (31 + 29)) {
      tdays -= 1;
    }
  }

  tdays = tdays + (tyears * 365) + leaps;
...

Ie, it will only add the leap year's day from Mar 1 onwards. Seems to fix the issue for my case above, but needs further testing for Feb 29 and Mar 1.

from redisearch.

Nice catch! Thanks for reporting.
I think a faster and simpler solution would be not to include the current year if it is a leap year, and to use tm_yday which counts the number of days from the beginning of the year including a leap day if there was one.
here is a correction check for my solution, compared to the existing implementation. Notice that I use leaps = (tyears + 1) / 4; to not include the current year, instead of leaps = (tyears + 2) / 4;
And here is a performance comparison of the fix, and here compared to the standard function

from redisearch.

Fixed. Available on master and version branches.
Will be part of v2.6.16 and v2.8.12

from redisearch.