ZMBNI-53 Type-check every table-config value, and stop swallowing null - #55
Merged
Merged
Conversation
Filed as "a null nested block crashes the loader". The sweep the story asked for
-- one derived from the schema rather than a hand-kept list -- found that the
crash was one of three shapes of a single root cause: **values were never checked
as they were read.** Scope widened deliberately, and the reason is the third
shape.
## The one that is not a message problem
{"remove_orphan_files": {"enabled": null}}
loaded, and **silently disabled reclamation**. `dict.get("enabled", True)`
returns `None` when the key is *present* with a null -- the default applies only
when the key is absent -- and `None` is falsy. So `maintain()` reported "disabled
in the config" for a config that never said so, which is exactly the "silently
reclaiming far less than expected" outcome `Retention.validate` warns about,
reached by a typo, in a tool whose job includes deleting files. Measured, not
inferred: `for_table(...).retention.remove_orphan_files.enabled is None`.
## The other two
**A null where a block belongs** escaped as a bare `TypeError: 'NoneType' object
is not iterable` out of `_reject_unknown`, which called `set(raw)` on whatever it
was handed. Measured across all fourteen nested blocks: thirteen crashed. Only
`namespaces.<ns>.tables` failed cleanly, and only because a missing `tables` is
checked separately.
**A wrong-typed scalar** reached arithmetic inside `validate()`:
`min_input_files: "day"` raised `TypeError: '<' not supported between 'str' and
'int'`. And a string did not even crash where a *block* was expected --
`set("day")` is a perfectly good set, so `"ordering": "day"` reported its own
characters as unknown keys.
`TableConfigError` is the type a caller is told to catch -- the CLI maps it to
exit 2, the user guide calls it *the* config failure -- so each of these was a
broken promise rather than an ugly message.
## The fix is one accessor, not thirteen guards
`_value(raw, key, where, expect=..., default=..., nullable=...)` reads every
value in the file. `_reject_unknown` gained the matching check for blocks. Both
name the path: `namespaces.a.tables.b.min_input_files: expected a number, found a
string`.
`bool` is refused where a number is wanted, because it is an `int` subclass and
`min_input_files: true` would otherwise be read as `1` -- a config that means
nothing accepted as one that means something.
A required key gets different advice from an optional one: "Omit the key to take
the default (2)" is wrong for `sort[].column`, which has none, so that path says
"this key needs a string" instead.
## What still accepts null, and why the schema had to move too
Unchanged: the settings documented as "leave whatever is there" -- the
`expire_snapshots` windows, the `metadata` properties, `target_file_size_bytes`,
`description` -- and four list-valued keys the loader coalesces to empty
(`namespaces`, `partition`, `partition_evolution.rules`, `ordering.sort`).
Narrowing those would refuse configs that work today for no gain.
**Those four were also a live disagreement with the schema shipped in #26**, in
the direction that makes a schema harmful: the loader accepted the null, the
schema refused it, so a valid file validated as an error. Declared in
`NULL_MEANS_EMPTY` -- a property of the parser, not of the annotation, which is
why it has to be stated -- and the schema regenerated. `ZOrder.columns` and
`NamespaceSettings.tables` coalesce too but `validate()` then refuses the empty
result, so the loader rejects those documents and the schema agreeing was already
correct.
#26's module docstring described the crash as current behaviour and is rewritten;
leaving it would have documented a bug as a design.
## The tests
`test_the_schema_and_the_loader_agree_about_null_everywhere` and
`test_no_document_makes_the_loader_crash_instead_of_refusing` sweep **every field
position derived from the schema**, building each document from the path alone --
no per-location fixture, so neither can fall behind a format that grows a block.
Together they are what found all three shapes, and the reason they found them is
that #26's `test_the_schema_never_rejects_what_the_loader_accepts` only spelled
`null` on *scalar* fields; the gap was the whole bug.
Verified load-bearing: reverting both guards fails **60** of them.
`test_the_sweep_reaches_the_fields_it_claims_to` guards the guard, since a
`field_paths` that stopped early or a `document_with` that built the wrong shape
would make both sweeps vacuously green.
`tests/test_tableconfig.py` carries the loader-side cases as a plain table, so
someone reading config tests finds them without going via the schema.
798 passed (775 before), ruff and mypy clean. `examples/table-config.json` and
the demo's config both still load.
Filed under **SAFETY** in the changelog with the fit argued: it narrows accepted
config values, which the BREAKING definition names, and ships in a patch anyway
for the reason SAFETY exists -- a configuration that silently disabled an
operation is worse than one that is refused.
Closes #53.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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.
Filed as "a null nested block crashes the loader" — a one-line fix. The sweep the story asked for (derived from the schema rather than a hand-kept list) found that the crash was one of three shapes of a single root cause: values were never checked as they were read. Scope widened deliberately; the third shape is why.
The one that is not a message problem
{"remove_orphan_files": {"enabled": null}}loaded, and silently disabled reclamation.
dict.get("enabled", True)returnsNonewhen the key is present with a null — the default applies only when the key is absent — andNoneis falsy. Somaintain()reported "disabled in the config" for a config that never said so.That is precisely the "silently reclaiming far less than expected" outcome
Retention.validatewarns about, reached by a typo, in a tool whose job includes deleting files. Measured, not inferred:for_table(...).retention.remove_orphan_files.enabled is None.The other two
A null where a block belongs escaped as a bare
TypeError: 'NoneType' object is not iterableout of_reject_unknown, which calledset(raw)on whatever it was handed. Measured across all fourteen nested blocks: thirteen crashed.A wrong-typed scalar reached arithmetic inside
validate()—min_input_files: "day"raisedTypeError: '<' not supported. A string did not even crash where a block was expected:set("day")is a perfectly good set, so"ordering": "day"reported its own characters as unknown keys.TableConfigErroris the type a caller is told to catch — the CLI maps it to exit 2, the user guide calls it the config failure — so each of these was a broken promise, not an ugly message.The fix is one accessor, not thirteen guards
_value(raw, key, where, expect=…, default=…, nullable=…)reads every value;_reject_unknowngained the matching check for blocks. Both name the path:boolis refused where a number is wanted — it is anintsubclass, somin_input_files: truewould otherwise read as1. A required key gets different advice from an optional one: "omit it to take the default" is wrong forsort[].column, which has none.The schema had to move too
Unchanged: settings documented as "leave whatever is there" (
expire_snapshotswindows,metadataproperties,target_file_size_bytes,description) and four list-valued keys the loader coalesces to empty. Narrowing those would refuse configs that work today for no gain.Those four were a live disagreement with the schema shipped in #26, in the direction that makes a schema harmful: the loader accepted the null, the schema refused it, so a valid file validated as an error. Now declared in
NULL_MEANS_EMPTY— a property of the parser, not the annotation, which is why it must be stated — and the schema regenerated.ZOrder.columnsandNamespaceSettings.tablescoalesce too, butvalidate()then refuses the empty result, so the schema agreeing was already correct.#26's module docstring described the crash as current behaviour; rewritten, since leaving it would document a bug as a design.
The tests
Two sweeps over every field position derived from the schema, each document built from the path alone — no per-location fixture, so neither can fall behind a format that grows a block. They are what found all three shapes, and the reason they found them is that #26's
test_the_schema_never_rejects_what_the_loader_acceptsonly spellednullon scalar fields. That gap was the whole bug.Verified load-bearing: reverting both guards fails 60 of them.
test_the_sweep_reaches_the_fields_it_claims_toguards the guard, since afield_pathsthat stopped early would make both sweeps vacuously green.tests/test_tableconfig.pycarries the loader-side cases as a plain table so config tests are readable without going via the schema.Compatibility
Filed under SAFETY, with the fit argued rather than assumed: this narrows accepted config values, which the BREAKING definition explicitly names. It ships in a patch anyway for the reason SAFETY exists — a configuration that silently disabled an operation is worse than one that is refused. If a
nullis in yourtable-config.jsonthe run now tells you where, and if it was on anenabledflag, that operation was not running.Verification
798 passed (775 before),
ruff check,ruff format --checkandmypyclean.examples/table-config.jsonand the demo's config both still load.Closes #53.