fix: keep None in an array wherever it sits - #1970
Open
shcheklein wants to merge 15 commits into
Open
Conversation
`list[int | None]` fails to save whenever the first element is None:
[1, None] -> [1, None]
[None, 2] -> UdfError: None incompatible with Int64
[None, None] -> UdfError
convert_type reads val[0] to decide how to handle a whole array. A leading
match returns the array untouched, and None slips through with it; anything
else converts element by element, where None has no type to convert and
raises. So the same annotation and the same values succeed or fail on order
alone.
None carries no type, so it cannot say what an array holds: probe the first
element that can, and let a None element pass through the per-element path
instead of being handed to a converter. The probe stops at the first item
unless the array starts with None, so the fast path costs what it did --
1.3 ms for a million ints, against 1.5 ms before.
Reading needed the same treatment: TypeReadConverter.float turns None into
nan, which is right for a bare float column and wrong for a nullable element,
and without it these arrays would save and then read back nan. A nullable
element now keeps its None, as a nullable column already does.
This also removes an order-dependent storage layout: with Array(JSON),
[{"k": 1}, None] was stored nested while [None, {"k": 1}] was stored as
strings.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Deploying datachain with
|
| Latest commit: |
1d9b362
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://5be407d5.datachain-2g6.pages.dev |
| Branch Preview URL: | https://fix-array-conversion-element.datachain-2g6.pages.dev |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
The JSON-array test pinned SQLite's stored form. ClickHouse resolves a JSON item
to str where SQLite resolves it to dict, so it stores the item as a string and
the assertion failed there.
What an item becomes is the backend's business. That a None beside it does not
change the answer is the thing this fix is about, so compare the two orders to
each other instead of to a literal. Holds under both mappings:
JSON -> dict [{'k': 1}, None] / [None, {'k': 1}]
JSON -> str ['{"k":1}', None] / [None, '{"k":1}']
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Probing one element could not answer a question about the whole array. Reviewing
the previous commit found two more ways it went wrong:
list[dict[str, Item] | None] [None, {"a": Item(n=1)}]
probed past the None, found a dict, called the array converted, and returned it
with the models still inside -- TypeError on write. Main got this right by
accident, because a leading None failed the isinstance check and sent the array
down the per-element path. And a None reached a non-nullable Array(Int64)
unchecked, which main allowed only when the None came last.
The column already says what its items are, so ask it rather than the values:
- items this backend keeps as JSON objects are normalized, never dumped, which
is what a dict item already got and what a model nested in one needed
- everything else converts element by element, and a None is kept only where
item_type.dc_nullable says one belongs
Element order no longer changes anything. Non-finite floats and mixed element
types still do, and are #1968's.
Converting every element costs ~10x the old single isinstance on a microbenchmark
(1536 floats: 1.4us -> 21.7us) and nothing measurable in a write: 4000 rows of
512-dim embeddings ran 3.24s against 3.42s, JSON encoding and IO dominating. A
sound C-level scan was tried first and dropped as unnecessary.
Array(JSON) now stores an int item as 1 rather than "1", so a tuple column
written before this reads back the same but no longer compares equal to one
written after. Deliberate: the old layout also wrote None as the string "null".
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Deciding the whole JSON branch from the declared item type swept in arrays this
fix has no business touching. A tuple of ints moved from ["1","2"] to [1,2] and a
tuple of models from strings to objects, neither of which contains a None. Both
declare Array(JSON) exactly as before, so a dataset written either side of the
change read back the same while comparing as different: merge on such a column
returned nothing, distinct counted one row as two.
An array of JSON objects keeps its old shape, so only arrays that actually hold a
None are affected, and of those only the order that was already inconsistent with
itself:
[{"k": 1}, None] [{"k":1},null] unchanged
[None, {"k": 1}] ["null","{\"k\":1}"] -> [null,{"k":1}]
Normalizing each item rather than passing the array through is still what reaches
a model nested inside a dict item, so that stays fixed.
Whether a tuple should store like a list is a real question and the answer is
probably yes, but it is a storage change that needs its own migration, not a side
effect of a None fix.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Two ways the last commit still reached past its own scope, both in mixed arrays:
(1, {"a": Item(n=2)}) ['1', '{"a":{"n":2}}'] -> ['1', {'a': Item(n=2)}]
({"k": 1}, 2) [{'k': 1}, 2] -> [{'k': 1}, '2']
The first left a model unconverted and failed to write; the int made the
all-dict test false, and the per-element path then took the dict for already
converted and skipped normalizing it. The second changed the stored shape of an
array with no None in it at all, so datasets written either side of the change
stopped comparing equal -- the thing the previous commit set out to stop.
An array without a None now takes exactly the path it took before. Only arrays
holding one reach the new handling, which is all this fix was ever about.
A model beside a plain value in an array without a None is still left raw, as on
main: ({"a": Item(n=2)}, 1) does not write. Same defect as the pair above,
reached without a None, and it belongs to #1968 with the rest of the mixed-array
behaviour.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
tuple[int | None, ...] maps to Array(Nullable(JSON)), and short-circuiting None
before the item's own converter changed what it stored:
(1, None) ["1","null"] -> ["1",null]
That case was never broken. A JSON item already produced "null" in either order,
because json.dumps writes the null itself; only types with no in-band null --
Int64, Float, String -- raise on a None and needed one held back. So keep None
back only for those, and let JSON encode its own.
It also leaves the two representations coherent rather than mixed. Under
Array(JSON) every element is a JSON document written as a string, so an absent
one is the string "null"; a bare null beside quoted numbers belonged to neither
convention.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Two more places the fix reached past what was broken. An array of JSON objects was decided by "every element is a dict or None", which (None, None) satisfies with no dict in it at all. That stored [null,null] where main stored ["null","null"], for a value main handled the same in either order. An array with no object in it is not an array of objects, so require one. list[Annotated[int, "meta"] | None] and list[Literal["a", "b"] | None] came out as non-nullable items, because nullability was decided by testing the annotation against (int, float, str, ...) -- which Annotated[int, "meta"] and Literal["a", "b"] are not, though both resolve to a scalar that takes a NULL. [1, None] then raised where main passed it through. Decide from the type the annotation resolves to instead. That second one is a fix rather than a restoration: the item was declared non-nullable, so on ClickHouse a None in such an array had nowhere to go and became 0. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Three more from review. Optional and Annotated nest either way round, and the peeling only went one deep, so list[Annotated[int | None, "x"]] came out non-nullable and [1, None] raised where main passed it. A Literal can also carry the None among its values. Peel until there is nothing left to peel. Nullability was then decided by identity against the SQL types, which a user's own Int64 subclass is not. Decide by subclass. And the list of non-None elements was being built for every array holding a None, scalar ones included, after which each element went through a full recursive conversion: a million ints plus one None took 267 ms against main's 1.2 ms. Build it only where objects are actually inspected, and let a scalar that already matches its column stay as it is -- 32 ms now. Arrays are still walked once to find out whether a None is present, which the old code never did. That is what keeps a stray None out of a non-nullable column, and it costs 12.7 us on a 1536-dim embedding against 1.5 us. On a real write it disappears: 4000 rows of 512-dim ran 2.83 s against 2.81 s. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A fixed tuple keeps all of its slots in the one column, but only the first was asked about None, so tuple[int, int | None] came out non-nullable and (1, None) raised where main passed it. Every slot decides now. A None also has more spellings than the peeling knew. Union arms are inspected in turn, so Literal[None] sitting in one counts: list[str | Literal[None]] is the same type as list[str | None] and now behaves like it. Nullability is decided from the resolved column type rather than by resolving the annotation a second time, which also drops the last use of unwrap_optional here -- its "is type(None) among the arms" test is what missed these spellings. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
list[Annotated[str, "meta"] | Literal[None]] raised "Cannot recognize type" before nullability was ever considered: the item type was resolved from the annotation as written, and neither the scalar table nor the union handling knows that composition, though each half is recognized alone. Pydantic accepts the annotation and ["x", None] with it. Peel first, then resolve. Ellipsis is deliberately left in place -- it is what marks a variadic tuple, and dropping it here would quietly turn tuple[int, ...] into Array(Int64), which is #1963's change to make with its own migration. This raised no TypeError on main either, so it is a gap rather than a regression. Declared types are unchanged across seventeen annotations and storage across fourteen. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Substituting the peeled annotation everywhere broke four schemas that had worked. Literal[None] peels to NoneType, which nothing maps, so tuple[str, Literal[None]] fell back to Array(JSON) and read back as JSONDecodeError, while list[Literal[None]] and tuple[Literal[None], ...] stopped building at all. A bare model peeled out of a union stopped resolving too, since only the union around it made it JSON. Peeling is only worth anything here when what is left resolves to something, so try it and keep the annotation as written when it does not. All four map as they did on main, and list[Literal[None]] now stores its None instead of raising. Declared types are unchanged across seventeen annotations and storage across fourteen. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
is_chain_type accepts list[Child | None], and list[Child] writes, but map(output=...) raised "Cannot recognize type Child" while building the schema: the union arm hid the model from the check that maps a model element to JSON. Ask that check what the annotation resolves to as well. Fixing the schema then reached flatten, which decided from value[0] the same way convert_type did -- a leading model meant model_dump() on every element, including the Nones, and a leading None meant the models were left raw. Ask the first element that carries a type, and leave the Nones alone. Both orders and an all-None list now round-trip. Nothing that already wrote is affected: these raised before, so there is no stored form to stay compatible with. Storage is unchanged across fourteen annotations, and the declared type changes only for list[Model | None], which had none. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Picking the shape from the first element that carried one was the same mistake a
level up. For list[dict | list[dict] | None] holding
[None, [{"a": 1}], {"b": 2}], the nested list was chosen and the later dict then
went through a list-only path and tripped its assert; main leaves such a list
alone, as it maps to JSON either way. Every element that carries a type has to
agree before a shape is assumed, which also stops model_dump() being called on
elements that are not models.
The reader also does not look through Annotated when rebuilding a nested model,
so list[Annotated[Child, "meta"] | None] wrote but came back as plain dicts
instead of models. Admitting a model element is deliberately shallow now: only a
plain Model | None, which reads back as models. The wrapped spelling raises as
it does on main, rather than storing something that reads wrong.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Admitting a model element from the first slot alone let a fixed tuple through on
the strength of a slot that was not checked:
tuple[A | None, Annotated[B, "meta"] | None] was stored, and a leaf read handed
back the second slot as {"y": 2} rather than B(y=2) -- a whole-model read hides
it, since Pydantic revalidates. Main refuses that annotation. Every slot is
checked now, so it is refused again, while tuple[A | None, B | None], whose
slots both read back as models, is allowed.
Flattening also built a list of every non-None element before looking at any of
them, on lists that settle on the first: a million ints cost 8.5 MB and 8.5 ms
for a probe that only had to see one value. Take the first element that carries
a type, and walk the rest only when it is a model or a list.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A root model dumps to whatever it wraps, and the reader only rebuilds a model from a mapping, so list[RootModel[int] | None] wrote and then handed back [None, 1] rather than [None, R(1)]. A whole-model read hides it, since Pydantic revalidates the scalar. list[R] already reads back that way on main; what this must not do is admit a spelling main refuses and store it wrong. Refused again. Nullable nested lists -- list[list[A | None] | None] -- now resolve where main refused them, but only because the inner list[Model | None] resolves at all now. The outer None remains unwritable, exactly as for list[list[int] | None], which main resolves and cannot write either. Rejecting the annotation would take the working all-present case with it, so it is left alone and recorded with the other array limitations. 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.
list[int | None]fails to save when the first element isNone:convert_typedecided how to handle a whole array by looking atval[0]. Aleading
1matched the item type, so the array was stored untouched and theNonerode along with it. A leadingNonedid not match, so every element wassent to a converter — and
Nonehas nothing to convert, so it raised.Each element now answers for itself, and a
Nonesurvives only whereitem_type.dc_nullablesays one belongs.What that changes
Read back after a save,
mainagainst this PR:list[int | None][1, None][1, None][1, None][None, 2]UdfError[None, 2][None, None]UdfError[None, None]list[float | None][1.0, None][1.0, nan][1.0, None][None, 2.0]UdfError[None, 2.0]nanin that first row is the read side of the same bug:TypeReadConverter.floatmaps
Nonetonan, which is right for a float column and wrong for a nullableelement. Scalar
Optional[float]already returnsNone, so arrays were theoutlier.
A model nested inside a dict item was converted in one order and not the other:
list[dict[str, Item] | None][{"a": Item(n=1)}, None]TypeError: Object of type Item is not JSON serializable[{'a': Item(n=1)}, None][None, {"a": Item(n=1)}][None, {'a': Item(n=1)}][None, {'a': Item(n=1)}]And a
Nonereached a non-nullable column unchecked, again depending onwhere it sat:
On disk
Only arrays that contain a
Nonechange, and only the order that was alreadyinconsistent with itself:
list[dict | None][{"k": 1}, None][{"k":1},null][None, {"k": 1}]["null","{\"k\":1}"][null,{"k":1}]An array without a
Nonetakes exactly the path it took before, so nothing elsemoves. Checked by writing twelve annotations with both versions and diffing the
stored column —
list[int],list[str],list[dict],list[Model],list[list[int]],tuple[int, ...],tuple[str, ...],tuple[Model, Model],tuple[int, int]and the rest are byte-identical, and so are mixed arrays like({"k": 1}, 2).Earlier revisions of this PR did not hold that line: one moved
tuple[int, ...]from["1","2"]to[1,2], another moved({"k": 1}, 2)from[{"k":1},2]to[{"k":1},"2"]. Both mademergeon such a column returnnothing across old and new datasets, since the two hydrate the same but compare
as different in SQL.
Still unfixed here, exactly as on
main: a model beside a plain value in anarray with no
None,({"a": Item(n=2)}, 1), is left raw and fails to write.Same defect as the row above reached without a
None; it belongs with the restof the mixed-array behaviour in #1968.
Converting every element costs about 10× more per array on a microbenchmark and
nothing measurable on a write: 4000 rows of 512-dim embeddings ran 2.83 s
against 2.81 s, JSON encoding and IO dominating. Arrays are also walked once to
find out whether a
Noneis present, which the old code never did — that iswhat keeps a stray
Noneout of a non-nullable column.Left for #1968
All of these are how
mainbehaves today and are untouched here, because fixingthem changes the stored shape of arrays that already round-trip — which breaks
comparison between datasets written either side of the change. They are written
up under An array's stored shape is decided by one of its
elements:
({"a": Item(n=2)}, 1)raisesTypeError: Object of type Item is not JSON serializable; reverse the order andit writes. Same defect as the
Nonecase fixed here, reached without aNone.({"k": 1}, 2)stores[{'k': 1}, 2],(2, {"k": 1})stores['2', '{"k":1}'].list[int]stores[1,2];tuple[int, ...]stores["1","2"]. This is what#1963 runs into.
nullin loosely typed fields —dictorAny,not
list[float].Noneat the outer level.list[list[int] | None]writes[[1, 2], [3]]but not[None, [1, 2]]: only ascalar item is made nullable, and an array has nowhere to keep the null. Since
list[Model | None]resolves now,list[list[Model | None] | None]reaches thesame wall, where it used to stop earlier for want of an inner type. Rejecting
the annotation outright would take the working all-present case with it.
list[RootModel[int]]returns
[1, 2], not[R(1), R(2)], because the reader rebuilds a model onlyfrom a mapping. The
Model | Nonespelling is refused here rather than storedthat way.
First of the steps found while reviewing #1963 — that PR's order-dependent
nullable failure is this same bug, reached through a different annotation.