Skip to content
This repository has been archived by the owner on Jul 30, 2024. It is now read-only.
/ NuGet.Jobs Public archive

Fix for statistics pipline to display proper last 6 weeks data around the new year #712

Merged
merged 5 commits into from
Feb 1, 2019

Conversation

agr
Copy link
Contributor

@agr agr commented Jan 23, 2019

Addresses NuGet/NuGetGallery#6836

Fix for the DownloadReportLast6Weeks sproc

Changed the way [dbo].[Fact_Download] is joined to [dbo].[Dimension_Date]: instead of direct join, a WeekLookup CTE is introduced that limits the date range by itself and corrects the week number for dates around new year.

In dev new sproc produces the following output:

Year WeekOfYear Downloads
2018 51 762
2018 52 5036
2018 53 3720
2019 2 3437
2019 3 9691
2019 4 2995

Old sproc output:

Year WeekOfYear Downloads
2018 51 762
2018 52 5036
2018 53 514
2019 1 3206
2019 2 3437
2019 3 9691

Code fix

Not needed anymore since all is handled in SQL.

Replaced the ugly DATETIME to DATE conversion with the pretty one.
@scottbommarito
Copy link

Would be possible to

  1. Fetch last 7 weeks
  2. Combine any rows in the result that represent < 7 days
  3. Return the most recent 6 weeks

We should only have multiple rows in the result with < 7 days if it's the end of the year.

I think this would be a less drastic code change.

@agr
Copy link
Contributor Author

agr commented Jan 23, 2019

We would be trading "drastic code change in C#" with "drastic code change in SQL" here. I'll take a look into it though.

@agr agr changed the title [WIP] Fix for statistics pipline to display proper last 6 weeks data around the new year Fix for statistics pipline to display proper last 6 weeks data around the new year Jan 31, 2019
@agr
Copy link
Contributor Author

agr commented Jan 31, 2019

OK, it ended up not that hard.

Copy link

@scottbommarito scottbommarito left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome fix!

@joelverhagen
Copy link
Member

Should this really be against master?

Copy link
Member

@joelverhagen joelverhagen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice one.

@agr agr changed the base branch from master to dev February 1, 2019 18:53
@agr agr merged commit 604eb51 into dev Feb 1, 2019
@agr agr deleted the agr-last6weeks branch February 1, 2019 23:05
joelverhagen pushed a commit that referenced this pull request Jul 31, 2020
joelverhagen pushed a commit that referenced this pull request Oct 26, 2020
… the new year (#712)

* Replaced the `top 6` with `@MaxDate`
Replaced the ugly DATETIME to DATE conversion with the pretty one.

* Rewrote query to produce required data right away.
joelverhagen pushed a commit that referenced this pull request Oct 26, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants