pr-427-review.md

ALICE

Review: Broaden _requires_full_refresh to check entire query tree

Summary

This PR simplifies _requires_full_refresh by replacing a targeted SELECT-projection-only check with a whole-query parsed.find(expression) search. The motivation is that Snowflake dynamic tables don't support change tracking for CURRENT_TIMESTAMP()/CURRENT_DATE/CURRENT_TIME anywhere in the query, not just in SELECT projections. The old code incorrectly allowed INCREMENTAL mode when these functions appeared in WHERE clauses.

This is a correctness fix — the old behavior could produce dynamic tables that Snowflake silently downgrades or errors on.

Findings

1. Test name/docstring/assertion contradict new behavior (bug in tests)

The diff changes line 1937 from refresh_mode = incremental to refresh_mode = auto, but leaves three artifacts pointing at the old behavior:

Test name (test_feature_query_builder.py:1917):

def test_dynamic_table_incremental_refresh(self, builder):

Docstring (test_feature_query_builder.py:1918):

"""Feature with CURRENT_TIMESTAMP only in WHERE should use INCREMENTAL."""

Assertion message (test_feature_query_builder.py:1976):

assert_sql_equal(ddl, expected, "INCREMENTAL dynamic table mismatch")

All three now describe the opposite of what the test actually asserts. Suggested fix:

def test_dynamic_table_auto_refresh_where_clause(self, builder):
    """Feature with CURRENT_TIMESTAMP in WHERE also requires AUTO refresh."""
    ...
    assert_sql_equal(ddl, expected, "AUTO dynamic table (WHERE clause) mismatch")

This matters because six months from now someone reading test_dynamic_table_incremental_refresh will expect it to test incremental behavior, not auto.

2. Docstring in production code is stale (feature_query_builder.py:577-589)

The old docstring on _requires_full_refresh reads:

"""
Detect if query requires FULL refresh mode using AST traversal.

Snowflake dynamic tables don't support change tracking for:
- CURRENT_TIMESTAMP() in SELECT projections (allowed in WHERE/HAVING/QUALIFY)

This is now incorrect — the whole point of the PR is that these functions are not allowed in WHERE/HAVING/QUALIFY either. The docstring should be updated to match the new semantics:

"""
Detect if query requires FULL refresh mode using AST traversal.

Snowflake dynamic tables don't support change tracking for queries
containing CURRENT_TIMESTAMP(), CURRENT_DATE, or CURRENT_TIME anywhere
in the query — including WHERE, HAVING, and QUALIFY clauses.

Note: On main, line 580 already has the stale "allowed in WHERE/HAVING/QUALIFY" text that the diff doesn't touch — confirming this was missed.

3. The build_dynamic_table docstring is also stale (feature_query_builder.py:455-456)

Wraps the feature query in a CREATE OR REPLACE DYNAMIC TABLE statement
with incremental refresh mode.

This should say "with automatic refresh mode detection" or similar, since the function now emits either AUTO or INCREMENTAL.

4. The removed comment about MAX_BY/MIN_BY was useful context

The old code had:

# Note: MAX_BY/MIN_BY (ArgMax/ArgMin) are now converted to
# QUALIFY with ROW_NUMBER(), which supports incremental refresh

This taught readers why those aggregation types aren't in the check list. The new code removes this context. Since MAX_BY/MIN_BY compatibility is tested separately (test_max_by_allows_incremental_refresh, test_min_by_allows_incremental_refresh), this isn't blocking — but adding a one-liner back would be kind to future readers:

for expression in [exp.CurrentTimestamp, exp.CurrentDate, exp.CurrentTime]:
    # MAX_BY/MIN_BY use QUALIFY (not these functions), so they stay incremental
    if parsed.find(expression) is not None:
        return True

5. Code simplification is clean

The core logic change — from triple-nested for loops to parsed.find() — is a genuine improvement. find() does exactly the right tree traversal and the code is now 3 lines instead of 13. No correctness concerns with the sqlglot API usage.

Recommendation

Request changes — two stale docstrings and a misleading test name. The fixes are small but the teaching quality matters: a test named test_dynamic_table_incremental_refresh that actually asserts refresh_mode = auto is a trap for the next reader.