Flowframe / laravel-trend

Generate trends for your models. Easily generate charts or reports.
MIT License
671 stars 70 forks source link

Issue with PgSQL driver with hourly aggregates #10

Closed ZacharyDuBois closed 1 year ago

ZacharyDuBois commented 2 years ago

Apparently my original assumption was entirely incorrect. This is an issue specifically with the PgsqlAdapter specifically line 13 for the hourly aggregate. You have seconds included when it should only be scoped to the hours hence the duplication issue when you pad the returned aggregate for missing intervals. I have made an override class for my project. I will create a PR this weekend :)

Original thought below (with lack of caffeine): Hi,

When using Trend to get aggregate data using a date column that contains milliseconds in pgsql, it will return TrendValues with and without seconds for the same interval. See example code and output below.

Migration:

Schema::create('examples', function (Blueprint $table) {
    $table->timestamp('created_at', 3); // Time stamp with a precision of 3 (ie: ms)
});

Model:

class Example extends Model
{
    protected $dateFormat = 'Y-m-d H:i:s.v';

    protected $casts = [
        'created_at' => 'datetime',
    ];
}

Trend call:

Trend::model(Example::class)
    ->between(now()->subDay(), now())
    ->perHour()
    ->count();

Output:

Look at the pointers 5434 and 5395, they are for the same interval but one was returned with seconds. This happens for every period where data exists. So all the periods with no data, it does not happen.

Illuminate\Support\Collection {#5444
     all: [
       Flowframe\Trend\TrendValue {#5434
         +date: "2022-03-08 20:00",
         +aggregate: 0,
       },
       Flowframe\Trend\TrendValue {#5395
         +date: "2022-03-08 20:00:00",
         +aggregate: 14,
       },
       Flowframe\Trend\TrendValue {#5367
         +date: "2022-03-08 21:00",
         +aggregate: 0,
       },
       Flowframe\Trend\TrendValue {#5373
         +date: "2022-03-08 21:00:00",
         +aggregate: 93,
       },
       Flowframe\Trend\TrendValue {#5406
         +date: "2022-03-08 22:00",
         +aggregate: 0,
       },
       Flowframe\Trend\TrendValue {#1639
         +date: "2022-03-08 22:00:00",
         +aggregate: 180,
       },
       Flowframe\Trend\TrendValue {#5431
         +date: "2022-03-08 23:00",
         +aggregate: 0,
       },
       Flowframe\Trend\TrendValue {#5409
         +date: "2022-03-09 00:00",
         +aggregate: 0,
       },
       Flowframe\Trend\TrendValue {#5375
         +date: "2022-03-09 01:00",
         +aggregate: 0,
       },
       Flowframe\Trend\TrendValue {#5376
         +date: "2022-03-09 02:00",
         +aggregate: 0,
       },
       Flowframe\Trend\TrendValue {#5404
         +date: "2022-03-09 03:00",
         +aggregate: 0,
       },
       Flowframe\Trend\TrendValue {#5407
         +date: "2022-03-09 04:00",
         +aggregate: 0,
       },
       Flowframe\Trend\TrendValue {#5396
         +date: "2022-03-09 05:00",
         +aggregate: 0,
       },
       Flowframe\Trend\TrendValue {#5368
         +date: "2022-03-09 06:00",
         +aggregate: 0,
       },
       Flowframe\Trend\TrendValue {#5369
         +date: "2022-03-09 07:00",
         +aggregate: 0,
       },
       Flowframe\Trend\TrendValue {#5372
         +date: "2022-03-09 08:00",
         +aggregate: 0,
       },
       Flowframe\Trend\TrendValue {#5355
         +date: "2022-03-09 09:00",
         +aggregate: 0,
       },
       Flowframe\Trend\TrendValue {#5403
         +date: "2022-03-09 10:00",
         +aggregate: 0,
       },
       Flowframe\Trend\TrendValue {#5413
         +date: "2022-03-09 11:00",
         +aggregate: 0,
       },
       Flowframe\Trend\TrendValue {#5400
         +date: "2022-03-09 12:00",
         +aggregate: 0,
       },
       Flowframe\Trend\TrendValue {#5397
         +date: "2022-03-09 13:00",
         +aggregate: 0,
       },
       Flowframe\Trend\TrendValue {#5392
         +date: "2022-03-09 14:00",
         +aggregate: 0,
       },
       Flowframe\Trend\TrendValue {#5371
         +date: "2022-03-09 15:00",
         +aggregate: 0,
       },
       Flowframe\Trend\TrendValue {#5399
         +date: "2022-03-09 16:00",
         +aggregate: 0,
       },
       Flowframe\Trend\TrendValue {#5391
         +date: "2022-03-09 17:00",
         +aggregate: 0,
       },
       Flowframe\Trend\TrendValue {#5421
         +date: "2022-03-09 18:00",
         +aggregate: 0,
       },
       Flowframe\Trend\TrendValue {#5470
         +date: "2022-03-09 19:00",
         +aggregate: 0,
       },
       Flowframe\Trend\TrendValue {#5471
         +date: "2022-03-09 20:00",
         +aggregate: 0,
       },
     ],
   }
ZacharyDuBois commented 2 years ago

Updated my issue title and body after I got some caffeine to realize it was an entirely simpler issue. Like above, I will submit a PR over the weekend, thanks for this library!

Larsklopstra commented 2 years ago

@ZacharyDuBois have you had time to work on the PR?

Larsklopstra commented 1 year ago

Closing stale issue