Repository navigation
fix: #1430 Array/Map property held by several sources of an embedded node passes the one-type guard - #1554
fix: #1430 Array/Map property held by several sources of an embedded node passes the one-type guard#1554hari2353 wants to merge 7 commits into
Conversation
…embedded node passes the one-type guard (genezhang#1430)
…ed from one_type_guard)
genezhang
left a comment
There was a problem hiding this comment.
Thanks for #1430. The motivating case works: an Array(String) property held by several sources now reads on ClickHouse 26.7. I found a case that goes from a loud error to a silently wrong answer, so I think the approach needs narrowing.
What improves (26.7.1). Two labels with tags Array(String), MATCH (n) RETURN n.tags returns all four rows (main: Code 50 from the guard). An embedded Airport node held by three sources (tags and city present in some) also reads correctly, with NULLs where a source has no value.
What regresses. The toTypeName(coalesce(...)) NOT LIKE 'Variant(%' check is removed for every multi-source property, and the comment above one_type_guard explains why it exists: the class comparison treats all integer widths as one class (^U?Int\d+$ becomes Int), so Int64 vs UInt64 passes it. On 26.7, coalesce(Int64, UInt64) is Variant(Int64, UInt64), and the Variant check was the only thing rejecting it.
Repro: rv.a(id UInt64, v Int64) holding 8 and -1, rv.b(id UInt64, v UInt64) holding 8 and 100, labels A and B with a v property:
MATCH (n) RETURN count(DISTINCT n.v)
-- main: Code 50 (guard, loud) PR: 4 (Neo4j: 3, since 8 = 8)
MATCH (n) RETURN DISTINCT n.v
-- PR: 8, 100, 8, -1
So the Variant keeps the Int64 8 and the UInt64 8 apart, which is the situation the guard was written to refuse.
Other points
- 162 goldens change here (every golden containing the guard), while the description says "corpus goldens byte-identical". The golden updates look mechanical and correct, but please fix the wording.
- On 25.8 the same query still fails (Code 386 at the
UNION ALL), so nothing changes there.
Direction to consider (I have not tested it). Keep the Variant check, and skip it only when the columns are themselves the same Variant(...) type, which is the Array/Map case you hit. A test with Int64 vs UInt64 across two labels (expect the loud error) next to the Array case would pin both.
Please also add a regression test for the Array case; only an existing assertion was edited. Happy to re-review.
What
Fixes #1430: an Array/Map property held by several sources of an embedded node fails the one-type guard (UNKNOWN_TYPE), even when the types match.
Cause: each source's column in the union rows is
Variant(Array(T))(because NULL UNION ALL Array(T) produces a Variant). Thecoalesceinsideone_type_guardis then a Variant too, and the guard'stoTypeName(coalesce(...)) NOT LIKE 'Variant(%'check rejects it.Fix: remove the Variant check from
one_type_guard. The class comparison (class(columns[0]) = class(columns[i])) already rejects genuinely mixed types. The Variant check only added a false positive for matching types that ClickHouse represents as a Variant coalesce.Gates (local, 1.94.1,
--no-default-features)Spec-driven
Backward-compatible (only removes a false-positive rejection; mixed types still rejected by the class comparison). Corpus goldens byte-identical (ratchet green).
Closes #1430