Skip to content

fix(account_parameter): cast FLOAT-typed parameters to a Python float - #76

Merged
noel merged 1 commit into
mainfrom
fix/account-parameter-float-type
Sep 9, 2026
Merged

noel merged 1 commit into
mainfrom
fix/account-parameter-float-type

Conversation

@noel

@noel noel commented Sep 9, 2026

Copy link
Copy Markdown
Contributor

Summary

snowcap apply reported a phantom ~ UPDATE for INITIAL_REPLICATION_SIZE_LIMIT_IN_TB on every run, with the diff table showing identical 10.0 -> 10.0.

Root cause: SHOW PARAMETERS IN ACCOUNT reports this parameter's type column as FLOAT (confirmed live), not NUMBER. _cast_param_value only handled BOOLEAN/NUMBER/STRING, so it fell through to the else: return raw_value branch and the fetched value stayed a raw string ("10.0"). The YAML-declared value parses via PyYAML into a Python float (10.0). The diff in blueprint.py compares these with a plain !=, so "10.0" != 10.0 registers as a real change, while the printed table stringifies both sides independently for display, hiding the type mismatch.

_cast_param_value is the single shared function behind every SHOW-PARAMETERS-driven fetch (account, warehouse, table, task, and user parameters), so this fixes the same latent bug class for any FLOAT-typed parameter across all of those, not just this one.

Fix

Added a FLOAT branch to _cast_param_value that parses the raw string to a Python float, mirroring the existing NUMBER branch's error handling.

Test plan

  • Added test_float / test_invalid_float_raises to TestCastParamValue, and an end-to-end TestFetchAccountParameter test reproducing the exact reported repro (SHOW PARAMETERS row typed FLOAT).
  • Verified the new tests fail against the pre-fix code ('10.0' == 10.0 assertion error) and pass after.
  • Full non-integration suite passes: python -m pytest tests/ --ignore=tests/integration (2218 passed, 1 skipped, 1 xfailed).
  • make lint (black, codespell, ruff) and make typecheck (mypy) pass on tracked files.
  • Ran ponytail-review (no findings) and code-review (no findings).

SHOW PARAMETERS reports INITIAL_REPLICATION_SIZE_LIMIT_IN_TB (and other
decimal-valued parameters) with type FLOAT, not NUMBER. _cast_param_value
had no branch for it, so it fell through to the raw-string default and
stayed a str, while the YAML-declared value parses to a Python float.
Comparing "10.0" != 10.0 made plan propose the same UPDATE on every run,
even though both sides render identically in the diff table.
@github-actions

github-actions Bot commented Sep 9, 2026

Copy link
Copy Markdown

Review of PR #76

No issues found. The change adds a FLOAT branch to _cast_param_value (snowcap/data_provider.py:652-656), mirroring the existing NUMBER/BOOLEAN/STRING branches in the same function, so SHOW PARAMETERS-reported FLOAT-typed values (e.g. INITIAL_REPLICATION_SIZE_LIMIT_IN_TB) are cast to Python float instead of falling through to the raw string in the else branch (data_provider.py:659-660), which previously caused spurious plan diffs against YAML-declared float values.

  • Correctness: matches the pattern of the adjacent NUMBER branch and raises a clear exception on unparseable input, consistent with existing error handling.
  • Tests: tests/test_data_provider.py adds direct unit coverage for _cast_param_value("10.0", "FLOAT") and the invalid-input path, plus an integration-style regression test through fetch_account_parameter that mocks execute and asserts the returned value is a float. Good coverage for the fix's actual failure mode (str vs. float causing drift in blueprint.py diffing).
  • No SQL generation, privilege handling, or resource-architecture changes involved — this is purely a data-parsing fix confined to _cast_param_value.

@noel
noel merged commit ef5e152 into main Sep 9, 2026
6 checks passed
@noel
noel deleted the fix/account-parameter-float-type branch September 9, 2026 16:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant