Skip to content

Remove eventType variable from relational and clickhouse queries #3948

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
mod08 wants to merge 2 commits into from
Closed

Remove eventType variable from relational and clickhouse queries #3948

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d460e31
Remove eventType variable from relational and clickhouse queries
mod08 Jan 9, 2026
fbac74a
Refactor goal query filters to separate eventType handling for total …
mod08 Jan 9, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
47 changes: 41 additions & 6 deletions src/queries/sql/reports/getGoal.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,23 @@ async function relationalQuery(
const { rawQuery, parseFilters } = prisma;
const eventType = type === 'path' ? EVENT_TYPE.pageView : EVENT_TYPE.customEvent;
const column = type === 'path' ? 'url_path' : 'event_name';
const { filterQuery, dateQuery, joinSessionQuery, cohortQuery, queryParams } = parseFilters({

// Filters WITHOUT eventType for total count (all sessions)
const {
filterQuery: baseFilterQuery,
dateQuery,
joinSessionQuery,
cohortQuery,
queryParams: baseQueryParams,
} = parseFilters({
...filters,
websiteId,
startDate,
endDate,
});
Comment on lines +42 to +47
Copy link
Contributor

@greptile-apps greptile-apps bot Jan 13, 2026

Choose a reason for hiding this comment

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

The filters object is spread into the first parseFilters call, which means if the user includes eventType in their filters (e.g., via query parameters), it will still be applied to the total count calculation. This defeats the purpose of this fix, which is to exclude eventType filtering from the total count.

The getRequestFilters function in src/lib/request.ts (lines 70-81) extracts all filter parameters including eventType from the request, so users can pass eventType as a filter. When this happens:

  1. baseFilterQuery will include and event_type = {{eventType}}
  2. queryParams (after merging on line 59) will contain the explicit eventType value from line 56
  3. The total count subquery will be incorrectly filtered by event_type

To fix this, eventType should be explicitly excluded from the first parseFilters call:

Suggested change
} = parseFilters({
...filters,
websiteId,
startDate,
endDate,
});
// Filters WITHOUT eventType for total count (all sessions)
const { eventType: _, ...filtersWithoutEventType } = filters;
const {
filterQuery: baseFilterQuery,
dateQuery,
joinSessionQuery,
cohortQuery,
queryParams: baseQueryParams,
} = parseFilters({
...filtersWithoutEventType,
websiteId,
startDate,
endDate,
});

This ensures the total count is never filtered by eventType, regardless of whether it's provided in the user's filters.

Prompt To Fix With AI 
This is a comment left during a code review.
Path: src/queries/sql/reports/getGoal.ts
Line: 42:47

Comment:
The `filters` object is spread into the first parseFilters call, which means if the user includes `eventType` in their filters (e.g., via query parameters), it will still be applied to the total count calculation. This defeats the purpose of this fix, which is to exclude eventType filtering from the total count.

The `getRequestFilters` function in `src/lib/request.ts` (lines 70-81) extracts all filter parameters including `eventType` from the request, so users can pass eventType as a filter. When this happens:

1. `baseFilterQuery` will include `and event_type = {{eventType}}`
2. `queryParams` (after merging on line 59) will contain the explicit `eventType` value from line 56
3. The total count subquery will be incorrectly filtered by event_type

To fix this, `eventType` should be explicitly excluded from the first parseFilters call:

```suggestion
  // Filters WITHOUT eventType for total count (all sessions)
  const { eventType: _, ...filtersWithoutEventType } = filters;
  const {
    filterQuery: baseFilterQuery,
    dateQuery,
    joinSessionQuery,
    cohortQuery,
    queryParams: baseQueryParams,
  } = parseFilters({
    ...filtersWithoutEventType,
    websiteId,
    startDate,
    endDate,
  });
```

This ensures the total count is never filtered by eventType, regardless of whether it's provided in the user's filters.

How can I resolve this? If you propose a fix, please make it concise.


// Filters WITH eventType for goal-specific count
const { filterQuery: goalFilterQuery, queryParams: goalQueryParams } = parseFilters({
...filters,
websiteId,
value,
Expand All @@ -40,6 +56,8 @@ async function relationalQuery(
eventType,
});
Comment on lines 33 to 57
Copy link
Contributor

@greptile-apps greptile-apps bot Jan 9, 2026

Choose a reason for hiding this comment

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

[P0] Removing the eventType filter causes incorrect query results. When type === 'path', the query should filter by eventType: EVENT_TYPE.pageView (1), and when type !== 'path', it should filter by eventType: EVENT_TYPE.customEvent (2). Without this filter, path-based goals will incorrectly include custom events that occurred on that path, and event-based goals will incorrectly include page views. The eventType parameter must be added back to parseFilters.

Prompt To Fix With AI 
This is a comment left during a code review.
Path: src/queries/sql/reports/getGoal.ts
Line: 32:39

Comment:
[P0] Removing the `eventType` filter causes incorrect query results. When `type === 'path'`, the query should filter by `eventType: EVENT_TYPE.pageView` (1), and when `type !== 'path'`, it should filter by `eventType: EVENT_TYPE.customEvent` (2). Without this filter, path-based goals will incorrectly include custom events that occurred on that path, and event-based goals will incorrectly include page views. The eventType parameter must be added back to parseFilters.

How can I resolve this? If you propose a fix, please make it concise.


const queryParams = { ...baseQueryParams, ...goalQueryParams };

return rawQuery(
`
select count(distinct website_event.session_id) as num,
Expand All @@ -50,15 +68,15 @@ async function relationalQuery(
${joinSessionQuery}
where website_event.website_id = {{websiteId::uuid}}
${dateQuery}
${filterQuery}
${baseFilterQuery}
) as total
from website_event
${cohortQuery}
${joinSessionQuery}
where website_event.website_id = {{websiteId::uuid}}
and ${column} = {{value}}
${dateQuery}
${filterQuery}
${goalFilterQuery}
`,
queryParams,
).then(results => results?.[0]);
Expand All @@ -73,7 +91,22 @@ async function clickhouseQuery(
const { rawQuery, parseFilters } = clickhouse;
const eventType = type === 'path' ? EVENT_TYPE.pageView : EVENT_TYPE.customEvent;
const column = type === 'path' ? 'url_path' : 'event_name';
const { filterQuery, dateQuery, cohortQuery, queryParams } = parseFilters({

// Filters WITHOUT eventType for total count (all sessions)
const {
filterQuery: baseFilterQuery,
dateQuery,
cohortQuery,
queryParams: baseQueryParams,
} = parseFilters({
...filters,
websiteId,
startDate,
endDate,
});
Comment on lines +101 to +106
Copy link
Contributor

@greptile-apps greptile-apps bot Jan 13, 2026

Choose a reason for hiding this comment

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

Same issue as in the relational query: if eventType is included in the user's filters, it will be applied to the total count calculation, defeating the purpose of this fix.

The eventType should be explicitly excluded from the filters passed to this parseFilters call:

Suggested change
} = parseFilters({
...filters,
websiteId,
startDate,
endDate,
});
// Filters WITHOUT eventType for total count (all sessions)
const { eventType: _, ...filtersWithoutEventType } = filters;
const {
filterQuery: baseFilterQuery,
dateQuery,
cohortQuery,
queryParams: baseQueryParams,
} = parseFilters({
...filtersWithoutEventType,
websiteId,
startDate,
endDate,
});
Prompt To Fix With AI 
This is a comment left during a code review.
Path: src/queries/sql/reports/getGoal.ts
Line: 101:106

Comment:
Same issue as in the relational query: if `eventType` is included in the user's filters, it will be applied to the total count calculation, defeating the purpose of this fix.

The `eventType` should be explicitly excluded from the filters passed to this parseFilters call:

```suggestion
  // Filters WITHOUT eventType for total count (all sessions)
  const { eventType: _, ...filtersWithoutEventType } = filters;
  const {
    filterQuery: baseFilterQuery,
    dateQuery,
    cohortQuery,
    queryParams: baseQueryParams,
  } = parseFilters({
    ...filtersWithoutEventType,
    websiteId,
    startDate,
    endDate,
  });
```

How can I resolve this? If you propose a fix, please make it concise.


// Filters WITH eventType for goal-specific count
const { filterQuery: goalFilterQuery, queryParams: goalQueryParams } = parseFilters({
...filters,
websiteId,
value,
Expand All @@ -82,6 +115,8 @@ async function clickhouseQuery(
eventType,
});
Comment on lines 93 to 116
Copy link
Contributor

@greptile-apps greptile-apps bot Jan 9, 2026

Choose a reason for hiding this comment

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

[P0] Same issue as in the relational query - the eventType filter must be added back to parseFilters to prevent incorrect mixing of pageView and customEvent types. Without it, both the numerator (goal count) and denominator (total count) will include events of the wrong type.

Prompt To Fix With AI 
This is a comment left during a code review.
Path: src/queries/sql/reports/getGoal.ts
Line: 72:79

Comment:
[P0] Same issue as in the relational query - the `eventType` filter must be added back to parseFilters to prevent incorrect mixing of pageView and customEvent types. Without it, both the numerator (goal count) and denominator (total count) will include events of the wrong type.

How can I resolve this? If you propose a fix, please make it concise.


const queryParams = { ...baseQueryParams, ...goalQueryParams };

return rawQuery(
`
select count(distinct session_id) as num,
Expand All @@ -91,14 +126,14 @@ async function clickhouseQuery(
${cohortQuery}
where website_id = {websiteId:UUID}
${dateQuery}
${filterQuery}
${baseFilterQuery}
) as total
from website_event
${cohortQuery}
where website_id = {websiteId:UUID}
and ${column} = {value:String}
${dateQuery}
${filterQuery}
${goalFilterQuery}
`,
queryParams,
).then(results => results?.[0]);
Expand Down