grorge123 commented on PR #5526:
URL:
https://github.com/apache/datafusion-comet/pull/5526#issuecomment-6017607383
@mbutrovich thanks, that's fair, and I agree that a finding which reproduces
on `main` belongs in its own issue. I'll split the PR as you suggest. Here is
the plan; let me know if you'd group anything differently.
1. Fixes for bugs on `main`, each with an issue and its own PR:
- the native `to_csv`, `arrays_overlap` and `GetStructField` fixes;
- the dispatcher's interpreted `eval` fallback when generated code
doesn't compile;
- refusing non-deterministic user functions, `reflect` and subqueries in
the dispatcher. @dwsmith1983 has taken the per-occurrence kernel state in #6711
/ #6712, so I'll drop my duplicate implementation and build on that;
- tracking where Spark evaluates through `eval`, with the `arrays_zip`,
`array_repeat` and `trunc` changes. It goes after the `eval` fallback, which it
reuses;
- the CASE WHEN / `coalesce` branch casts and the one-argument
`coalesce`, and the stateful `coalesce` guard, which serializes its non-final
children twice;
- the set-op argument alignment with the relabel-only cast, and the
`array_repeat` item nullability, if each reproduces with typed inputs.
I'll reproduce each one on `main` before opening its issue. Anything that
only fails with a `NullType` output stays in this PR, and any other defect I
find to be reachable on `main` gets its own issue.
2. The null guards: I agree with @dwsmith1983's proposal in #5781 to take
the dispatcher route from #5867 for `size`, `array_append`, `arrays_zip` and
`map_from_arrays`, so I'll drop those checks from this PR. Narrowing which
guards are emitted can be a follow-up.
3. This PR, rebased on the above. It keeps the new admission boundary for
`NullType` outputs and the protections it needs, covered by SQL file fixtures.
I've replied to your inline comments on those parts.
4. `CometNullTypeColumnsBenchmark` and `CometNullTypeCompositionSuite` as
follow-ups.
I've pushed one commit for your comments on the tests (267b5ce):
`CometNullTypeCompositionSuite` and `CometNullTypeColumnsBenchmark` are out of
this PR and its CI workflows, and the duplicate planner case is gone. Otherwise
I'll leave this branch as it is until the first group lands, rather than
rebasing it again in the meantime.
Assisted-by: Claude Code (claude-opus-5-5)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
