andygrove commented on PR #5025:
URL:
https://github.com/apache/datafusion-comet/pull/5025#issuecomment-5496817696
> **Note on this review:** this was drafted by an LLM (Claude Code) at my
request. Unlike my previous pass, the findings here were checked against a
local build of this branch rather than read off the diff, but please still push
back on anything that is wrong or already handled.
Thanks for turning the last round around so quickly. I checked out the
branch, rebuilt native, and confirmed the `CalendarIntervalType` fix. With
`spark.comet.shuffle.mode=jvm` and `spark.comet.shuffle.enabled=true`:
```scala
spark.sql("select id, make_interval(1,2,3,4,5,6,7) as i from
range(100)").repartition(4).collect()
```
now plans a plain Spark `Exchange` and returns 100 rows, where before it
failed with `Comet Internal Error: Unsupported type: Interval(MonthDayNano)`
out of `Native.writeSortedFileNative`. Defaulting the new flag to `true` is the
right call given every other boundary accepted calendar intervals pre-refactor.
I also re-traced all four boundaries against the six-flag signature and they
reproduce the old predicates exactly, including the local scan case where
`DataTypeSupport`'s `StructType` arm recurses through the unqualified
`isTypeSupported` and dispatches back into the override. Your reading of the
`super` call was right and the comment you added makes it clear. I tried to
find a matching latent failure on the native shuffle boundary, which accepts
`CalendarIntervalType` both before and after this PR, and could not produce
one, so I am not raising it here.
**Nothing locks the fix in place.** The new unit test asserts that the
predicate returns `false`, which pins the predicate but not the call site. If
someone drops `allowCalendarInterval = false` from the columnar shuffle call,
the unit test still passes and the bug comes straight back. Could we add the
query above as a regression test, asserting the plan contains a Spark
`Exchange` rather than a `CometColumnarExchange`? Right now the only thing
standing between us and a repeat is a comment.
**The test shape cannot catch the bug we just found.** More generally the
test asserts each flag in isolation, but the defect was a call site passing the
wrong flag set, and flag-level assertions structurally cannot catch that. Since
the whole value of this refactor is that four boundaries share one predicate,
would you consider restructuring it as a per-boundary table? One row per call
site, each listing types that must be accepted and rejected, so the columnar
shuffle row pins down that calendar intervals and duplicate field names are out
while the native shuffle row pins down that intervals are in. That is cheap to
write and it is the artifact that would have caught this before review.
**The `allowAnyStringType` assertion is vacuous on two of the five CI
versions.** `assert(!supportedDataType(CharType(1), allowAnyStringType =
false))` only exercises the flag where `CharType` is a `StringType` subclass.
Checking the Spark sources, `v3.5.8` has `case class CharType(length: Int)
extends AtomicType` and it only became `extends StringType(...)` in `v4.0.1`,
so on Spark 3.4 and 3.5 it hits the default `false` arm regardless of the flag.
Since the comment in `CometLocalTableScanExec` says the intent is rejecting
non-default collations, could we use a collated `StringType` instead? I
verified on 4.1 that `supportedDataType(StringType("UTF8_LCASE"),
allowAnyStringType = false)` is `false` and `true` by default, so it exercises
the flag directly. `QueryPlanSerde.isStringCollationType` returns `false` on
the Spark 3.x shim if that helps with gating.
While we are in that test, it is a pure predicate test rather than an
expression correctness test, and `CometExpressionSuite` is already very large.
Would `spark/src/test/scala/org/apache/comet/serde/` be a better home,
alongside `CometScalarFunctionSuite`?
**`supportedDataType` still has no scaladoc.** The inline comments you added
at the columnar shuffle call site are exactly right, particularly the `row.rs`
pointer. Could we go one step further and document the function itself,
covering what each of the six flags gates and which boundary needs it? I am not
reopening the named-wrappers question, your answer on that was fair. But the
function is now the single chokepoint for four boundaries plus the default
expression-side callers in `hash.scala`, `arithmetic.scala`, `literals.scala`,
and `CometScalarSubquery.scala`, and it carries no documentation at all. The
native shuffle rationale that used to sit above the deleted predicate, that it
relies on the Arrow IPC writer and so should support everything Comet supports,
is gone with nowhere to land. That missing knowledge is the mechanism that
produced the bug. Nothing in the function records which boundary can handle
which type, so a type added to the unconditional base arm silently wi
dens all four at once. It would also be worth noting in the doc that the
polarity is mixed, since `allowComplex` and `allowIntervals` default
restrictive while the other four default permissive.
**On CI:** `Verify TPC-H Results` is red at the `Build project` step. I
could not read the log, but the same step passed in `Verify TPC-DS Results` and
every other build, lint, and test-group job is green, which rules out a compile
error. Looks like flake. Could you kick off a re-run so we have a clean board
before merge?
--
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]
