Skip to content

Commit f3bb498

Browse files
danieljchuserclaude
andcommitted
fix: temporal series month/week wrap-around and add year to query pipeline
Fix month wrap-around in getTemporalCondition using += 12 instead of = 12, and week wrap-around using += 52/53 instead of = 52/53. Add year column to all SQL raw CTEs, query dimensions, and DTO output so cross-year temporal series data can be correctly sorted downstream. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
1 parent 39f656f commit f3bb498

File tree

7 files changed

+52
-43
lines changed

7 files changed

+52
-43
lines changed

packages/spacecat-shared-athena-client/src/traffic-analysis/queries.js

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -102,9 +102,9 @@ export function getTrafficAnalysisQueryPlaceholdersFilled({
102102
}
103103

104104
if (numTemporalSeries > 1 && week && week > 0) {
105-
dimensions.push('week');
105+
dimensions.push('year', 'week');
106106
} else if (numTemporalSeries > 1 && month && month > 0) {
107-
dimensions.push('month');
107+
dimensions.push('year', 'month');
108108
}
109109
const dimensionColumns = dimensions.join(', ');
110110
const dimensionColumnsPrefixed = dimensions.map((col) => `a.${col}`).join(', ');
@@ -179,6 +179,7 @@ WITH min_totals AS (
179179
),
180180
raw AS (
181181
SELECT
182+
year,
182183
week,
183184
month,
184185
path,
@@ -279,6 +280,7 @@ WITH min_totals AS (
279280
),
280281
raw AS (
281282
SELECT
283+
year,
282284
week,
283285
month,
284286
path,

packages/spacecat-shared-athena-client/src/traffic-analysis/traffic-analysis-template.js

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -48,6 +48,7 @@ WITH min_totals AS (
4848
),
4949
raw AS (
5050
SELECT
51+
year,
5152
week,
5253
month,
5354
path,

packages/spacecat-shared-athena-client/src/traffic-analysis/traffic-data-base-response.js

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@ export const TrafficDataResponseDto = {
1818
* Converts a traffic data object into a JSON object.
1919
* @param {object} data - traffic data object.
2020
* @returns {{
21+
* year: number,
2122
* type: string,
2223
* channel: string,
2324
* platform: string,
@@ -33,6 +34,7 @@ export const TrafficDataResponseDto = {
3334
* }} JSON object.
3435
*/
3536
toJSON: (data) => ({
37+
year: data.year,
3638
week: data.week,
3739
month: data.month,
3840
type: data.trf_type,

packages/spacecat-shared-athena-client/test/index.test.js

Lines changed: 21 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -824,9 +824,9 @@ describe('Traffic analysis query functions', () => {
824824

825825
const result = getTrafficAnalysisQueryPlaceholdersFilled(params);
826826

827-
expect(result.dimensionColumns).to.equal('device, week');
828-
expect(result.dimensionColumnsPrefixed).to.equal('a.device, a.week');
829-
expect(result.groupBy).to.equal('device, week');
827+
expect(result.dimensionColumns).to.equal('device, year, week');
828+
expect(result.dimensionColumnsPrefixed).to.equal('a.device, a.year, a.week');
829+
expect(result.groupBy).to.equal('device, year, week');
830830
});
831831

832832
it('should add month to dimensions when numTemporalSeries > 1 and month is provided', () => {
@@ -841,9 +841,9 @@ describe('Traffic analysis query functions', () => {
841841

842842
const result = getTrafficAnalysisQueryPlaceholdersFilled(params);
843843

844-
expect(result.dimensionColumns).to.equal('device, month');
845-
expect(result.dimensionColumnsPrefixed).to.equal('a.device, a.month');
846-
expect(result.groupBy).to.equal('device, month');
844+
expect(result.dimensionColumns).to.equal('device, year, month');
845+
expect(result.dimensionColumnsPrefixed).to.equal('a.device, a.year, a.month');
846+
expect(result.groupBy).to.equal('device, year, month');
847847
});
848848

849849
it('should prefer week over month when both are provided with numTemporalSeries > 1', () => {
@@ -859,9 +859,9 @@ describe('Traffic analysis query functions', () => {
859859

860860
const result = getTrafficAnalysisQueryPlaceholdersFilled(params);
861861

862-
expect(result.dimensionColumns).to.equal('device, week');
863-
expect(result.dimensionColumnsPrefixed).to.equal('a.device, a.week');
864-
expect(result.groupBy).to.equal('device, week');
862+
expect(result.dimensionColumns).to.equal('device, year, week');
863+
expect(result.dimensionColumnsPrefixed).to.equal('a.device, a.year, a.week');
864+
expect(result.groupBy).to.equal('device, year, week');
865865
expect(result.dimensionColumns).to.not.include('month');
866866
});
867867

@@ -927,9 +927,9 @@ describe('Traffic analysis query functions', () => {
927927

928928
const result = getTrafficAnalysisQueryPlaceholdersFilled(params);
929929

930-
expect(result.dimensionColumns).to.equal('device, utm_campaign, week');
931-
expect(result.dimensionColumnsPrefixed).to.equal('a.device, a.utm_campaign, a.week');
932-
expect(result.groupBy).to.equal('device, utm_campaign, week');
930+
expect(result.dimensionColumns).to.equal('device, utm_campaign, year, week');
931+
expect(result.dimensionColumnsPrefixed).to.equal('a.device, a.utm_campaign, a.year, a.week');
932+
expect(result.groupBy).to.equal('device, utm_campaign, year, week');
933933
});
934934

935935
it('should handle multiple dimensions with month when numTemporalSeries > 1', () => {
@@ -944,9 +944,9 @@ describe('Traffic analysis query functions', () => {
944944

945945
const result = getTrafficAnalysisQueryPlaceholdersFilled(params);
946946

947-
expect(result.dimensionColumns).to.equal('device, utm_campaign, month');
948-
expect(result.dimensionColumnsPrefixed).to.equal('a.device, a.utm_campaign, a.month');
949-
expect(result.groupBy).to.equal('device, utm_campaign, month');
947+
expect(result.dimensionColumns).to.equal('device, utm_campaign, year, month');
948+
expect(result.dimensionColumnsPrefixed).to.equal('a.device, a.utm_campaign, a.year, a.month');
949+
expect(result.groupBy).to.equal('device, utm_campaign, year, month');
950950
});
951951

952952
it('should not add temporal dimension when neither week nor month is provided', () => {
@@ -979,8 +979,8 @@ describe('Traffic analysis query functions', () => {
979979
const placeholders = getTrafficAnalysisQueryPlaceholdersFilled(params);
980980
const sql = getTrafficAnalysisQuery(placeholders);
981981

982-
expect(sql).to.include('device, week');
983-
expect(sql).to.include('a.device, a.week');
982+
expect(sql).to.include('device, year, week');
983+
expect(sql).to.include('a.device, a.year, a.week');
984984
expect(sql).to.include('week=28');
985985

986986
// Verify no dangling commas
@@ -1001,8 +1001,8 @@ describe('Traffic analysis query functions', () => {
10011001
const placeholders = getTrafficAnalysisQueryPlaceholdersFilled(params);
10021002
const sql = getTrafficAnalysisQuery(placeholders);
10031003

1004-
expect(sql).to.include('device, month');
1005-
expect(sql).to.include('a.device, a.month');
1004+
expect(sql).to.include('device, year, month');
1005+
expect(sql).to.include('a.device, a.year, a.month');
10061006
expect(sql).to.include('month=7');
10071007

10081008
// Verify no dangling commas
@@ -1017,6 +1017,7 @@ describe('DTO Tests', () => {
10171017
describe('TrafficDataResponseDto', () => {
10181018
it('should convert traffic data to JSON format', () => {
10191019
const inputData = {
1020+
year: 2025,
10201021
week: 12,
10211022
month: 6,
10221023
trf_type: 'organic',
@@ -1039,6 +1040,7 @@ describe('DTO Tests', () => {
10391040
const result = TrafficDataResponseDto.toJSON(inputData);
10401041

10411042
expect(result).to.deep.equal({
1043+
year: 2025,
10421044
week: 12,
10431045
month: 6,
10441046
type: 'organic',

packages/spacecat-shared-athena-client/test/traffic-analysis-queries.test.js

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -677,6 +677,7 @@ describe('Traffic Analysis Queries', () => {
677677

678678
// Verify all columns in raw CTE
679679
const rawColumns = [
680+
'year',
680681
'week',
681682
'month',
682683
'path',
@@ -1041,6 +1042,7 @@ describe('Traffic Analysis Queries', () => {
10411042

10421043
// Verify all columns in raw CTE
10431044
const rawColumns = [
1045+
'year',
10441046
'week',
10451047
'month',
10461048
'path',

packages/spacecat-shared-utils/src/calendar-week-helper.js

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -225,15 +225,15 @@ export function getTemporalCondition({
225225
let currentYear = year;
226226
if (currentWeek < 1) {
227227
currentYear -= 1;
228-
currentWeek = has53CalendarWeeks(currentYear) ? 53 : 52;
228+
currentWeek += has53CalendarWeeks(currentYear) ? 53 : 52;
229229
}
230230
log?.info(`[getTemporalCondition] currentWeek: ${currentWeek}, currentYear: ${currentYear}`);
231231
conditions.push(getWeekInfo(currentWeek, currentYear).temporalCondition);
232232
} else if (hasValidMonth) {
233233
let currentMonth = month - i;
234234
let currentYear = year;
235235
if (currentMonth < 1) {
236-
currentMonth = 12;
236+
currentMonth += 12;
237237
currentYear -= 1;
238238
}
239239
log?.info(`[getTemporalCondition] currentMonth: ${currentMonth}, currentYear: ${currentYear}`);

packages/spacecat-shared-utils/test/calendar-week-helper.test.js

Lines changed: 20 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -542,30 +542,22 @@ describe('Utils - temporal helpers', () => {
542542
expect(c).to.equal('(year=2025 AND month=2) OR (year=2025 AND month=1) OR (year=2024 AND month=12)');
543543
});
544544

545-
it('handles multiple transitions when week goes below 1 (demonstrates current implementation)', () => {
546-
// Week 1 of 2025, going back 5 weeks
547-
// Note: current implementation has a bug where week becomes 0, -1, -2, etc.
548-
// and always gets set to 52 (since 2025 is not a 53-week year)
549-
// This test documents the actual behavior
545+
it('correctly wraps weeks across year boundary', () => {
546+
// Week 1 of 2025, going back 5 weeks → weeks 1, 52, 51, 50, 49
550547
const c = getTemporalCondition({ week: 1, year: 2025, numSeries: 5 });
551-
const parts = c.split(' OR ');
552-
expect(parts.length).to.be.at.least(5);
553548
expect(c).to.include('(year=2025 AND month=1 AND week=1)');
554-
// All subsequent weeks will be set to week 52 due to the implementation
555549
expect(c).to.include('(year=2024 AND month=12 AND week=52)');
550+
expect(c).to.include('(year=2024 AND month=12 AND week=51)');
551+
expect(c).to.include('(year=2024 AND month=12 AND week=50)');
552+
expect(c).to.include('(year=2024 AND month=12 AND week=49)');
556553
});
557554

558-
it('handles multiple transitions when month goes below 1 (demonstrates current implementation)', () => {
559-
// Month 1 of 2025, going back 5 months
560-
// Note: current implementation has a bug where month becomes 0, -1, -2, etc.
561-
// and always gets set to 12
562-
// This test documents the actual behavior
555+
it('correctly wraps months across year boundary', () => {
556+
// Month 1 of 2025, going back 5 months → months 1, 12, 11, 10, 9
563557
const c = getTemporalCondition({ month: 1, year: 2025, numSeries: 5 });
564-
const parts = c.split(' OR ');
565-
expect(parts.length).to.equal(5);
566-
expect(c).to.include('(year=2025 AND month=1)');
567-
// All subsequent months will be set to month 12 due to the implementation
568-
expect(c).to.include('(year=2024 AND month=12)');
558+
expect(c).to.equal(
559+
'(year=2025 AND month=1) OR (year=2024 AND month=12) OR (year=2024 AND month=11) OR (year=2024 AND month=10) OR (year=2024 AND month=9)',
560+
);
569561
});
570562

571563
it('prefers week over month when both are provided with numSeries > 1', () => {
@@ -597,8 +589,16 @@ describe('Utils - temporal helpers', () => {
597589
expect(parts.length).to.equal(15);
598590
expect(parts[0]).to.equal('(year=2025 AND month=12)');
599591
expect(parts[11]).to.equal('(year=2025 AND month=1)');
600-
// After month goes below 1, it becomes 12 repeatedly due to implementation
601-
expect(parts[14]).to.equal('(year=2024 AND month=12)');
592+
expect(parts[12]).to.equal('(year=2024 AND month=12)');
593+
expect(parts[13]).to.equal('(year=2024 AND month=11)');
594+
expect(parts[14]).to.equal('(year=2024 AND month=10)');
595+
});
596+
597+
it('handles month=12 with numSeries=4 (no year wrap)', () => {
598+
const c = getTemporalCondition({ month: 12, year: 2025, numSeries: 4 });
599+
expect(c).to.equal(
600+
'(year=2025 AND month=12) OR (year=2025 AND month=11) OR (year=2025 AND month=10) OR (year=2025 AND month=9)',
601+
);
602602
});
603603
});
604604
});

0 commit comments

Comments
 (0)