viirya commented on PR #5567:
URL:
https://github.com/apache/datafusion-comet/pull/5567#issuecomment-5496850821
Thanks for the detailed review — three of these were straight mistakes on my
part.
**`withSQLConf` return type.** You're right that Spark 4.x declares
`withSQLConf[T](...)(f: => T): T`, and my comment claiming it returns `Unit`
was wrong. But the `val` form does not compile on 3.x: `SQLHelper.withSQLConf`
there is `public default void withSQLConf(Seq, Function0<BoxedUnit>)`, and
`-Pspark-3.5` fails with `value nonEmpty is not a member of Unit`. Since we
build against both, I kept the `var` and fixed the comment to say what is
actually going on. Added the missing blank lines.
**The interval test wasn't testing `CalendarIntervalType`.** Correct —
`INTERVAL '1' MONTH` comes out as `YearMonthIntervalType`. Switched to
`make_interval(...)`, so it now matches its comment and covers the type #5059
is about.
**The fuzz file does have nested complex types.** Also correct:
`generateSchema` adds `struct<array<..>>` and `array<struct<..>>` when both
flags are set, and maps are the only omission. Fixed my comment and the
inherited one in `CometFuzzTestBase`.
**Map entry order.** Added a test with `map('a',1,'b',2)` vs
`map('b',2,'a',1)` (and a three-entry pair) asserting the rows share a
`spark_partition_id()` and match Spark.
**Parity test could pass without Comet.** Added `checkCometExchange(..., 1,
true)` to both the shallow and deep cases.
**Null keys.** Added a test with null keys at four levels — top level,
inside a struct, as an array element, and as a map value — with pairs of equal
keys asserted into the same partition and compared against Spark.
On the `DataType::Struct(_)` branch: you are right that it recurses into
`columns()` without consulting its own null mask while `List` and `Map` guard
on `is_null(row_idx)`. I tried to construct a misroute through a plan-built
null struct (both `if(true, NULL, named_struct(...))` and nulling a struct that
has non-null children) and could not — Comet's null struct came out with null
children in both cases, so the two agreed. So I cannot show it biting today,
but the asymmetry with the other two branches is real, and the new tests pin
the current behavior. Happy to file it separately if you would like it tracked.
**Default and performance.** Agreed, and I have flipped it to `false`. You
are right that `struct<int, string>` is not the interesting case: the shapes
that miss the vectorized element paths fall into `hash_list_array!`, which
slices a one-element array and re-enters `create_murmur3_hashes` per element,
and I have no numbers for that. Turning it on by default before measuring was
the wrong call. I would like to do the benchmark as a follow-up issue and PR
rather than hold this one; all the tests opt in explicitly now.
**On the decimal point.** You are right that the description overstated the
parity with the `hash` expression — `HashUtils.unsupportedReasonFor` rejects
`decimal(precision > 18)` and `TimeType` at any depth and the shuffle gate does
not. I have said so explicitly in the description. Worth noting what I actually
measured: with the config on, every complex column in that fuzz file has
partition parity with Spark, including `array<decimal(36,18)>` and the structs
containing it. `FuzzDataGenerator` builds decimals from `nextDouble()`, so the
values stay in the range where the 16-byte LE `i128` and the minimal BE
`BigDecimal` encodings agree — the divergence you describe is real in the code,
just not reachable with the values that generator produces.
**`spark_partition_id()` in the fuzz test.** I tried this and backed it out.
Pairing rows needs a stable identifier and neither attempt held up: `c0` has
many duplicate values, so sorting by it does not pair rows deterministically,
and `CAST(<key> AS STRING)` is not a faithful key for nested values — that
version failed for **jvm** shuffle too, which this change does not touch, so it
was my test construction rather than routing. Doing it properly looks like it
needs a real row id, i.e. materializing the fuzz table with something like
`monotonically_increasing_id()`, which means changing the shared fixture that
all three variants (native + 2x jvm) run against. I would rather not do that
inside this PR unless you want it here. Happy to add it now or file a follow-up
— which would you prefer?
--
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]
