Fix pre-aggregation materialization correctness - #302
Conversation
Resolve pushed filters against model sources
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c569d57d97
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| # range before inserting its replacement so repeated refreshes remain | ||
| # idempotent instead of accumulating duplicate rollup rows. | ||
| if lookback: | ||
| connection.execute(f"DELETE FROM {table_name} WHERE {watermark_column} >= {watermark_str}") |
There was a problem hiding this comment.
When a lookback refresh uses the documented/common WHERE date > {WATERMARK} source query, this deletes the row exactly at the computed cutoff with >=, while the subsequent insert selects only rows strictly after it. For example, a max watermark of January 10 with a five-day lookback permanently removes January 5; the deletion boundary must preserve or reload exactly the same range selected by the source query.
Useful? React with 👍 / 👎.
| connection.execute(f"DELETE FROM {table_name} WHERE {watermark_column} >= {watermark_str}") | ||
| connection.execute(f"INSERT INTO {table_name} {incremental_sql}") |
There was a problem hiding this comment.
On autocommit connections such as a direct DuckDB connection, any schema, type, or execution error in the insert after this delete leaves the pre-aggregation permanently missing its entire lookback window. Run the delete-and-insert replacement in one transaction or stage the replacement before deleting existing rows.
Useful? React with 👍 / 👎.
| if agg_type == "COUNT" and not measure.sql: | ||
| measure_input = f"CASE WHEN {filter_sql} THEN 1 ELSE NULL END" | ||
| else: | ||
| measure_input = f"CASE WHEN {filter_sql} THEN {measure_input} ELSE NULL END" |
There was a problem hiding this comment.
sql="*" as a filtered row count
When a filtered row-count metric is represented as Metric(agg="count", sql="*", filters=[...]), measure.sql is truthy and this branch produces COUNT(CASE WHEN ... THEN * ELSE NULL END), which DuckDB and other engines reject because * cannot appear inside a CASE. Handle sql == "*" like the existing no-SQL row-count case and place 1 in the filtered aggregate input.
Useful? React with 👍 / 👎.
1 participant
Extracts the filtered-metric, total-rollup, and incremental lookback correctness fixes from #283 and #279.