Skip to content

Don't drop rows for a non-null field under a nullable parent - #893

Open
phdoerfler wants to merge 8 commits into
typelevel:mainfrom
phdoerfler:fix/issue-888-nullable-parent-join
Open

Don't drop rows for a non-null field under a nullable parent#893
phdoerfler wants to merge 8 commits into
typelevel:mainfrom
phdoerfler:fix/issue-888-nullable-parent-join

Conversation

@phdoerfler

@phdoerfler phdoerfler commented Aug 8, 2026

Copy link
Copy Markdown
Contributor

This PR was mostly done by an LLM. I have poked and prodded at it, encouraged it to verify rather than assume, enforced a proper software engineering workflow including TDD and reviews on top of which I did a manual review. It was an iterative process. The PR is now at a stage where I can't find anything obviously wrong with it but I will admit that the SQL generation is a bit arcane and things might have slipped past me.

And now, without further ado, the detailed LLM analysis:


Fixes #888. Also fixes #892, which #888 turned out to depend on.

Three defects, one observable symptom. The first is the one #888 describes: a nested select carrying an INNER join was flattened into the enclosing select's join chain even when it attached via a LEFT join, so the inner join sat below the outer one and eliminated the rows the outer join had null padded. SqlSelect.nest now declines to flatten in that case and builds a subquery instead, which keeps the inner join scoped where it's genuinely valid rather than discarding the optimisation.

Fixing that alone had no effect on MSSQL, which turned out to have a second and older defect, described in #892: an OUTER APPLY was never correlated to its enclosing select, so it behaved as an inner join and discarded the same rows again for an unrelated reason. Laterality.joinPredicates is replaced by distributeJoinConditions, which can rewrite the join as well as yield predicates for the enclosing select, and Apply uses that to wrap the subquery of an OUTER APPLY in a correlating select. Applying the conditions to the subquery's result rather than pushing them into it preserves any LIMIT, OFFSET or DISTINCT, and keeps a same named table inside the subquery from shadowing one of the enclosing select — both of which regressed when tried the other way round.

Correlating once isn't enough, which is the third fix. Nesting a select re-examines joins an earlier nesting already correlated, and correlate didn't recognise the wrapper it had itself built — so it declined, the conditions were lifted a second time, and the correlation was undone. It now recognises that wrapper and leaves the join alone. With that, no query in any of the four backends' suites reaches a decline at all.

None of the three is observable without the others, which is why they're here together: without the first no APPLY is emitted for these queries at all, without the second the first is a no-op on MSSQL, and without the third the second only holds to a depth of one.

Replacing joinPredicates isn't binary compatible, so the last commit bumps tlBaseVersion to 0.30. It's kept separate in case you'd rather own that yourself or take it in a different release.

Worth noting that the issue understates the reach. The join is chosen by !context.tpe.isNullable && !context.tpe.isList, so a list is attached the same way a nullable field is, and a parent with no children at all was being dropped for the same reason: [B!]! with a non-null field beneath B lost every parent whose list was empty. That needs no nullable field anywhere in the schema, so it's the easier of the two to hit by accident. There's a test for each shape.

The tests are a shared mapping and suite run on all four SQL backends, with fixture data including a row whose non-null reference dangles — the case that distinguishes a row being absent from a row being dropped — and a parent with an empty list. A third test descends two lists through a non-null field, which is what reaches the repeated correlation; it needs a second reference back to the parent type, so that the inner list can be empty where the outer one isn't.

One thing deliberately out of scope. A genuinely dangling non-null reference is still not reported as an error: before this change the row vanished from the result entirely, and after it the parent row is returned with the offending field as null. Both are wrong under §6.4.3 step 1, which asks for an execution error propagated to the nearest nullable ancestor. That's a separate defect with a separate remedy — the object field path at SqlCursor narrows without checking, where the interface and union path checks first via narrowsTo — and I'd rather raise it on its own than widen this. The tests assert the post-fix behaviour as it stands, so they'll need updating when it's addressed.

Verified on all four backends: doobiepg, doobieoracle and skunkJVM are green, and doobiemssql is green apart from three pre-existing MovieSuite failures which fail identically at the base commit. Those are a local artifact rather than anything in this change — the container runs in the host's timezone, so moviesShownBetween resolves its range boundary an hour out and returns a fourth film — and they're what #862 pins to UTC.

Laterality.joinPredicates is replaced by distributeJoinConditions, which
is not binary compatible with 0.29.
scalafmt packs the conjunction and, forbidden from breaking on a chain's
first dot, breaks it mid-chain instead. Giving each condition its own line
leaves nothing long enough to need breaking.
A non-null field nested beneath a nullable parent generated an INNER JOIN
in the same flat join chain as the parent's LEFT JOIN, so rows whose
nullable parent was absent were eliminated. They should be returned with
the parent as null: the child's non-null-ness only applies when the parent
exists.

SqlSelect.nest now declines to merge a nested select into the parent's
join chain when this join is LEFT and the nested select carries INNER
joins, which once flattened would sit below it and filter the null-padded
rows away. The existing mkSubquery path then scopes those joins inside a
subquery, where the INNER remains valid and cannot eliminate outer rows,
so the optimisation is preserved rather than discarded.

Fixes typelevel#888.
@phdoerfler
phdoerfler force-pushed the fix/issue-888-nullable-parent-join branch from ec4574c to 65e31ee Compare August 8, 2026 13:44
MSSQL renders a lateral join as OUTER/CROSS APPLY, which takes no ON
clause, so the join's conditions were lifted into the enclosing select's
WHERE clause instead. For CROSS APPLY that is equivalent to an ON clause,
but for OUTER APPLY it is not: the WHERE clause is applied after the join
and discards precisely the null padded rows the outer join produced, so
an OUTER APPLY behaved as an inner join.

Wrap the subquery of an OUTER APPLY in a correlating select which applies
the conditions to its result, referring to the enclosing select's tables.
That is legal because APPLY is lateral, and makes OUTER APPLY equivalent
to the LEFT JOIN LATERAL other backends emit.

Applying the conditions to the subquery's result rather than pushing them
into it preserves any LIMIT, OFFSET or DISTINCT, and keeps a same named
table within the subquery from shadowing one of the enclosing select.

Laterality.joinPredicates is replaced by distributeJoinConditions, which
can rewrite the join as well as yield predicates for the enclosing
select.
Exercises a nullable field with a non-null field beneath it, on every SQL
backend, via a shared mapping and suite. The fixture data includes a row
whose non-null reference dangles, which is the case that distinguishes a
row being absent from a row being dropped.
The flattening bug turns on the attaching join being LEFT rather than
INNER, and SqlSelect.nest makes that choice for lists exactly as it does
for nullable fields: `inner` is false for both. Only the nullable half
was covered, yet the list half is the shape more schemas will meet, since
a list beneath which something is non-null needs no nullable field
anywhere for its rows to be eliminated.

Extends the shared fixture with a second, one-to-many group of tables — a
`D` with `es: [E!]!`, an `E` with a non-null `f` — including a `D` with no
`E`s at all. Before the fix that `D` disappeared from the result instead
of being returned with `es` as `[]`; neutralising the fix makes the new
test fail, so it does pin down the behaviour it claims to.
@phdoerfler
phdoerfler force-pushed the fix/issue-888-nullable-parent-join branch from 65e31ee to f3fddcd Compare August 9, 2026 11:16
Nesting a select re-examines joins which an earlier nesting already
correlated. `correlate` didn't recognise the wrapper it had built for such
a join, declined, and the caller fell back to lifting the conditions into
the enclosing select — which for an OUTER APPLY discards the null padded
rows the join produced, undoing the correlation and losing exactly the rows
it was there to keep.

Recognise that wrapper by the `correlated` flag it carries and yield the
join unchanged. It is recognised only while it still holds its conditions,
so one which had somehow lost them falls back to lifting rather than being
taken for correlated with nothing to apply.

With the wrapper recognised, no OUTER APPLY reachable by `nest` declines, so
`distributeJoinConditions` now treats a failure to correlate as a bug
rather than silently lifting into a wrong result.
Descends two lists through a non-null field, which nests the same shape
twice and so presents the inner join for correlation a second time. `E`
gains a second reference to `D` so that the second list can be empty where
the first is not — a shape the World fixture can't express, since a country
reached through a city always has that city.
@phdoerfler
phdoerfler force-pushed the fix/issue-888-nullable-parent-join branch from f3fddcd to 8e61746 Compare August 9, 2026 12:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

1 participant