Conversation
Adds support for multi-arm Union[...] signals (Union[basic,basic], Union[Model,Model], mixed, nullable, and collection arms like Union[str, list[str]]). A union stores a hidden _type_tag discriminator plus one column-group per arm; the active arm is identified by the tag, inactive arms are NULL/default. Optional[Model] is the single-arm case. Includes func.variant_type(), readable arm access, and cross-backend (SQLite + ClickHouse) round-trips with tests.
Deploying datachain with
|
| Latest commit: |
c6a33fe
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://4712f294.datachain-2g6.pages.dev |
| Branch Preview URL: | https://union.datachain-2g6.pages.dev |
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
…ng and removing items
for more information, see https://pre-commit.ci
|
@dreadatour thank you for the review. all fixed - please take a look. |
|
One more finding: a subclass of a declared union arm can be matched to a less specific arm, causing silent data loss. from typing import Union
import datachain as dc
from datachain.lib.data_model import DataModel
from datachain.query import Session
class Animal(DataModel):
legs: int = 4
class Zebra(Animal):
stripes: int = 1
class BabyZebra(Zebra):
age: int = 0
chain = dc.read_values(
value=[BabyZebra(stripes=9, age=2)],
output={"value": Union[Animal, Zebra]},
session=Session.get(in_memory=True),
)
value = chain.to_values("value")[0]
print(type(value).__name__, value)Actual: Expected:
Could we select the most specific matching declared superclass and add a regression test using a subclass of |
Fixed: the isinstance fallback now picks the narrowest matching arm. Exact-type match still wins first, so bool vs int is unaffected. |
dreadatour
left a comment
dreadatour
left a comment
There was a problem hiding this comment.
Looks good to me overall as far as I can tell — the diff is too big to be able to review it properly.
One comment before merge — let's, please, cleanup CLAUDE.md file? 😀
| "tests/unit/lib/test_signal_schema.py" = ["UP006", "UP007", "UP035", "UP045"] | ||
| "tests/unit/lib/test_optional.py" = ["UP007", "UP045"] | ||
| "tests/func/test_optional.py" = ["UP007", "UP045"] | ||
| "tests/unit/lib/test_union_types.py" = ["UP007", "UP045"] |
There was a problem hiding this comment.
minor: can we actually fix this instead of keep growing exclusions?
There was a problem hiding this comment.
Done - all three entries gone, incl. N806.
| assert arm_selector(Foo) == "Foo" | ||
|
|
||
|
|
||
| # ---- flatten / unflatten round-trips --------------------------------------- |
There was a problem hiding this comment.
minor: cleanup style - I don't think we use sections like this (also review comments, better names, etc)
There was a problem hiding this comment.
Done - banners gone. Comments that restated the test name dropped. test_signal_schema_union_path_edges split into two named tests.
|
Thanks @shcheklein - all fixed. Plus one extra: a separate review pass caught a nested union restoring the wrong arm (the tag was written, then ignored). I only ran this through Claude Code, no other models. Enough, or would you like a separate Codex pass? Otherwise I'd just merge it. |
|
@dmpetrov check the Codex output please https://claude.ai/code/artifact/345d4fe5-57b2-47fb-8579-6d5a604327be |
|
@shcheklein thank you! Fixed 1-4, thanks. 5 and 6 are in the description now. 7 reproduces on main, leaving it out. |
Closes #1812
Adds multi-arm
Union[...]signals —Union[int, str, float],Union[ModelA, ModelB], mixed, and nullable.Storage reuses the hidden
_type_tagdiscriminator:_type_tagholds the active arm's name ("int","Text"),NULLmeansNone.Optional[Model]is just the single-arm case.Compatibility: the old numeric
_type_tag(arm index) is still read, with aFutureWarning; its removal is tracked in #1949. Mixing a legacy dataset with a new one in a single query is not supported — the discriminator column type changedInt64→String. Arms are now serialized in a fixed order, so the first run after upgrading recomputes once instead of hitting its checkpoint.Now rejected: same-shaped union arms inside a
list/dict— one JSON cell has no per-element tag, somainsilently reads back the wrong arm. Workaround: a distinguishing field (kind: Literal["..."]). A per-element tag could lift this later — no issue filed yet.Out of scope (follow-up): collection arms (
Union[str, list[str]]) — #1952