andygrove commented on PR #5234:
URL:
https://github.com/apache/datafusion-comet/pull/5234#issuecomment-5170492552
Thanks for picking this up. Inverting the order so the Spark cast owns the
value semantics and Arrow owns the dictionary construction is the right shape,
and deleting the parquet branch is a nice cleanup.
I spent some time verifying the two preconditions the issue asked about, and
both hold up:
- `can_cast_types(Dictionary(_, value_type), to)` in arrow-cast 58.4
delegates straight to `can_cast_types(value_type, to)`, and the dict-to-dict
arm delegates to the value types. So the `can_cast_types` fallback in
`parquet_convert_array` covers exactly the same set of combinations the old
recursion covered through its own arrow fallback. No combination that used to
convert now falls through to `_ => Ok(array)`.
- None of the Comet-specific arms in `parquet_convert_array` has `Utf8` or
`LargeUtf8` as its from-type. They are `Struct`, `List`, `Timestamp(us, None)`,
`Map`, and `FixedSizeBinary(16)`. Since the deleted branch restricted values to
`Utf8`/`LargeUtf8`, the recursion could only ever reach identity or the arrow
fallback, so nothing Spark-specific is lost by deleting it.
Dropping the `spark_cast_postprocess` call on the new return also looks
safe. All four of its arms require `to_type` to be `Int64` or `Utf8`, so with a
`Dictionary` target it was always the identity.
I also liked the test data choice in
`test_cast_to_dictionary_deduplicates_casted_values`. `"0.2"` and `"."` both
cast to `0`, matching Spark's `UTF8String.toInt` truncation behavior, so it is
a real post-cast collision rather than a contrived one.
A few things I would like to work through before this merges.
### The new path supports fewer dictionary value types than the old one
`arrow::compute::cast` reaches `cast_to_dictionary`
(`arrow-cast-58.4.0/src/cast/dictionary.rs:178`), and that function has no arm
for `Boolean`. It also has none for `Null` or any nested type. Those all land
on `_ => Err("Unsupported output type for dictionary packing:
{dict_value_type}")`.
Under the old code, `cast_array(StringArray, Dictionary(Int32, Boolean))`
worked. `dict_from_values` wrapped the strings into `Dictionary(Int32, Utf8)`,
the recursion then hit the from-dictionary arm, and
`spark_cast_utf8_to_boolean` was applied to the values. Under the new code the
same call Spark-casts to `Boolean` correctly and then fails at the packing step.
What makes this awkward to guard against is that `can_cast_types` disagrees
with `cast` here. `can_cast_types(Boolean, Dictionary(Int32, Boolean))` returns
`true`, because the `(_, Dictionary(_, value_type))` arm just recurses on the
value type. So a caller cannot probe for support first.
Could you add a test for `StringArray -> Dictionary(Int32, Boolean)` so we
pin down what the behavior is either way? None of the existing tests cover a
boolean dictionary target, which is why CI is green on this. If it does
regress, one option is to keep constructing the dictionary manually when Arrow
cannot pack the value type.
### The value cast is threaded with cast options but the packing call is not
Would you mind using `cast_with_options(&values, to_type,
&native_cast_options)` instead of the bare `arrow::compute::cast`? Every other
arrow call in `cast_array` threads the options through, and the bare `cast`
silently picks up `CastOptions::default()`. I do not think it changes results
today, since `values` already has the target value type and the inner cast
inside `pack_numeric_to_dictionary` is an identity, but it is the one call in
the function that drops the eval mode.
### The surviving from-dictionary arm still hand-rolls, and it ignores the
requested key type
This one predates the PR, but it is the same function and the same class of
bug you just fixed on the parquet side, so I would rather not leave it floating.
The `Dictionary(Int32, Utf8 | LargeUtf8 | Binary | LargeBinary)` arm in
`cast_array` is hardcoded to `DictionaryArray::<Int32Type>`. For `to_type =
Dictionary(Int16, Int32)` it returns `Dictionary(Int32, Int32)`, so the
returned `DataType` does not match what was requested. After this PR,
`cast_array` honours the requested key type when the source is plain and
ignores it when the source is a string or binary dictionary, which is a
confusing split.
Arrow's `dictionary_to_dictionary_cast` already handles both the key cast
and the value cast, and it errors cleanly when keys do not fit rather than
silently returning the wrong type, so this arm may be able to collapse the same
way. If that is out of scope here, could you file a tracking issue and link it
from this PR? There is also an `Arc::new(casted_dictionary.clone())` in that
arm cloning something already owned, if you end up touching it.
### Test coverage
The two new tests are well targeted at what changed. Two gaps I would like
closed:
- Could you add a case with a non-`Int32` key type on the target, say
`Dictionary(Int8, Utf8)`? Your parquet test covers `Int32 -> Int16` keys but
the cast test does not, and that is exactly where the key-type question above
lives.
- A case for a non-string source would be good too, for example `Int32Array
-> Dictionary(Int32, Int64)`. The old path could not reach that, so it is new
behavior worth pinning down.
### One question on scope
It would help if the description said whether any real query plan can
request a `Dictionary` cast target. The doc comment on `spark_cast_postprocess`
says "Spark cannot specify Dictionary as to_type", and the required schemas
that reach `SparkSchemaAdapter` come from Spark types, which never carry
dictionaries. If that is right, the nested-dictionary bug you fixed on the
parquet side is not user-reachable today and this is defensive cleanup, which
is worth stating so reviewers know why there is no SQL-level test. If there is
a reachable path, an end-to-end test would be valuable.
--
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]
