Repository navigation
fix(lifecycle): apply every field when an update changes several fields - #91
Merged
Merged
Conversation
datacoves#70/datacoves#93 merged into main while this branch was open, rewriting update_scanner_package/update_schema to escape values via quote_value. This branch independently restructured the same two functions to apply every field in a multi-field delta. Combined both: the per-field-call loop and the _update_with_separate_clauses delegation from this branch, using quote_value's escaping from main. Also: - Fixed two tests left over from the merge that predated this branch's rewrite (MockProps stub, lowercase attr names, old $$-less expectations). - Widened create_scanner_package/drop_scanner_package's return type to Union[str, list[str]] to match update_scanner_package's now-broader return type. - Fixed a casing bug surfaced by code review: Python's str(False) is "False", not SQL's FALSE, so update_scanner_package quoted a bool "enabled" value as $$False$$, which Snowflake's SET_CONFIGURATION rejects. Bools now render as TRUE/FALSE before quoting. make check (black, ruff, mypy, full test suite) passes clean.
Contributor
|
Resolved the merge conflict with Also found and fixed, via code review on the resolution itself:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What and why
A plan that changes several fields of one object now applies all of them. Before,
update_schema,update_table,update_iceberg_table,update_masking_policy, andupdate_scanner_packagecalleddata.popitem()and wrote SQL for one field only.update_procedurewrote onlyEXECUTE ASwhenexecute_aschanged. The apply reported success, and the next plan showed the lost changes as drift. Fixes #89.ENABLE/DISABLE MANAGED ACCESS,SET BODY ->,EXECUTE AS), then oneupdate__defaultstatement for the other fieldsupdate__defaultstatement. A column change raisesNotImplementedError.SET_CONFIGURATIONcall for each fieldA new helper,
_update_with_separate_clauses, holds the shared shape. A rename must still arrive alone, as inupdate__default. A schematransientchange now raisesNotImplementedErrorinstead ofException. The handlers no longer change the caller'schange.delta. Schema SET values now render throughSchema.props, so they use$$quoting, for exampleSET COMMENT = $$it's here$$.Validation
tests/test_lifecycle.pyhas a multi-field test for each of the six handlers. 9 tests failed before the fix. On commit 73d5443,make checkpasses (2235 passed) and mypy is clean.Blueprinton the test account:ENABLE MANAGED ACCESS,SET MAX_DATA_EXTENSION_TIME_IN_DAYS = 9 COMMENT = $$v2$$SET BODY -> '***',SET COMMENT = $$v2$$EXECUTE AS CALLER,SET COMMENT = $$v2$$SET DATA_RETENTION_TIME_IN_DAYS = 3 COMMENT = $$v2$$For the table,
SHOW TABLESafter the apply showedretention_time=3andcomment=v2. The re-plan still showsdata_retention_time_in_daysbecausefetch_tablenever reads it (the mapping is commented out indata_provider.py). That gap is older than this PR.transientexception type. The live run does not reach that path.How to review
_update_with_separate_clausesinsnowcap/lifecycle.py, then the six handlers.update_schemaandupdate_scanner_package, and they conflict inlifecycle.py. This PR already fixes theupdate_schemaapostrophe bug from Quote ALTER SCHEMA SET values #70. To resolve, keep this PR'supdate_schema. Inupdate_scanner_package, keep this PR's loop and use Quote ALTER SCHEMA SET values #70'squote_valuequoting.update_task(fix(task): apply a body change together with other fields #88).update__defaultstill skips fields that have no prop (Props.render), and it still puts fields such ascluster_byafterSET. These need separate issues.