Repository navigation
Conversation
Eppo's API sends the constant 1 as the value column of an "Each Record" fact, which the importer looked up as a column and rejected with 'Column "1" isn't in the Fact Table'. Summing it now counts records; aggregations that need a real column say the fact counts each record.
[Medium impact] The change affects the Eppo metric-import workflow. The PR appears safe to merge, with a non-blocking request to test the each-record threshold branch. Reviews (1) · Last reviewed commit: "Eppo importer: treat an each-record fact..." · Reviewed by Greptile |
| t.aggregation_type === "sum" && !isEachRecord(agg.column) | ||
| ? valueColumn() | ||
| : "$$count", |
There was a problem hiding this comment.
Threshold fix lacks a test
The new each-record threshold branch has no regression test. Existing threshold tests sum "revenue" or explicitly request a count, so they would still pass if this fix were removed. Add a case with aggregation_type: "sum" and column: "1" that checks aggregateFilterColumn: "$$count" and preserves the comparison.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/front-end/services/importing/eppo/eppo-importing.ts
Line: 923-925
Comment:
**Threshold fix lacks a test**
The new each-record threshold branch has no regression test. Existing threshold tests sum `"revenue"` or explicitly request a count, so they would still pass if this fix were removed. Add a case with `aggregation_type: "sum"` and `column: "1"` that checks `aggregateFilterColumn: "$$count"` and preserves the comparison.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Features and Changes
Fixes
Column "1" isn't in the X Fact Tableon import.Eppo's "Each Record" facts have no value column; the API sends the constant
1instead, and the importer treated that as a column name. A sum of an each-record fact now imports as a count ($$count), the same for a threshold metric's sum. Aggregations that need a real column (count distinct, percentile) say the fact counts each record instead of a misleading missing-column error, and any other constant is called out as a constant.Dependencies
None.
Testing
pnpm --filter front-end test test/services/eppo-importing.test.ts(50 tests, with the each-record cases added)Screenshots
None.