Conversation
|
|
||
| #[deprecated(since = "46.0.0", note = "See get_example_types instead")] | ||
| pub fn get_possible_types(&self) -> Vec<Vec<DataType>> { | ||
| pub fn get_possible_types(&self) -> Vec<Vec<NativeType>> { |
There was a problem hiding this comment.
Hm. A public API break for a deprecated method ... Maybe it is time to just remove it ?!
There was a problem hiding this comment.
That's true. I will drop get_possible_types. However, get_example_types is also public - it is used in the information schema internally, and can be used by DF users.
Since it breaks the expr-common API, how about deprecating the get_example_types signature and adding a NativeType-based get_representative_types?
There was a problem hiding this comment.
@martin-g to avoid breaking changes in the get_example_types (my miss), I've introduced the new method, while deprecating get_example_types.
The Signature API is heavily DataType-based, so the get_example_types. Should we let them coexist, or even move the NativeType-based logic to the information schema? cc @jayzhan211
There was a problem hiding this comment.
It seems the NativeType is not powerful enough to fully replace DataType here.
For example https://github.com/apache/datafusion/pull/21737/changes#diff-52d29120f24c2a01793f6d729fdb7898abaa8d2320db75aba5cbb06cd714930eR505-R516 shows that there are no counterparts for Union's UnionMode and Map's keys_sorted and defaults should be used. And this may lead to confusions.
There was a problem hiding this comment.
Undoubtedly, it's not a replacement now. The question for this improvement is whether we'd like to provide changes only to the information-schema, which benefits from native types (then we move code there), or also provide a public API for the Signature to support NativeType alongside DataType (as now).
At the same time, UDFs are migrating to TypeSignature, abstracting from physical data types.
Co-authored-by: Martin Grigorov <martin-g@users.noreply.github.com>
Instead, add the new `get_representative_types` API with NativeType. Deprecated old `get_example_types` and related helpers, left as-is to avoid breaking API change.
| /// | ||
| /// This is used for `information_schema` and can be used to generate | ||
| /// documentation or error messages. | ||
| /// Remove with `get_example_types` |
There was a problem hiding this comment.
This should be a comment (//) instead of rustdoc (///)
|
|
||
| #[deprecated(since = "46.0.0", note = "See get_example_types instead")] | ||
| pub fn get_possible_types(&self) -> Vec<Vec<DataType>> { | ||
| pub fn get_possible_types(&self) -> Vec<Vec<NativeType>> { |
There was a problem hiding this comment.
It seems the NativeType is not powerful enough to fully replace DataType here.
For example https://github.com/apache/datafusion/pull/21737/changes#diff-52d29120f24c2a01793f6d729fdb7898abaa8d2320db75aba5cbb06cd714930eR505-R516 shows that there are no counterparts for Union's UnionMode and Map's keys_sorted and defaults should be used. And this may lead to confusions.
Jefffrey
left a comment
There was a problem hiding this comment.
sorry it took so long for me to get around to looking at this 😅
| NativeType::Boolean => Ok(DataType::Boolean), | ||
| NativeType::Int8 => Ok(DataType::Int8), | ||
| NativeType::Int16 => Ok(DataType::Int16), | ||
| NativeType::Int32 => Ok(DataType::Int32), |
There was a problem hiding this comment.
It feels like we're just moving this logic from where it was in get_example_types to here; I think to properly work towards closing the original issue we might need a larger rework around how datatypes work with information schema, otherwise we're still stuck with this datatype <---> nativetype interwork no matter where we move it 🤔
There was a problem hiding this comment.
@Jefffrey , thank you for this! I took another look and reworked it significantly.
I agree, UDF's field and return type resolvers are still on physical Arrow types, and the high-level rework should be done as part of the bigger #12622 epic. I think this could be a small step toward that migration.
Instead of moving a huge mapping around, I decided to reuse the existing LogicalType::default_cast_for logical-physical mapping. It is much leaner now, and all the logic is consolidated in one place, not spread across crates. We can reason on logical types for the catalog. When we remove the deprecated get_example_types, there won't be any traces of the mapping in the high-level catalog and expr crates. What do you think?
@martin-g, regarding your concern - the details of mapping, keys_sorted, and nested structures are now the responsibility of a LogicalType
|
Thank you for your contribution. Unfortunately, this pull request is stale because it has been open 60 days with no activity. Please remove the stale label or comment or this will be closed in 7 days. |
|
Thank you for opening this pull request! Reviewer note: cargo-semver-checks reported the current version number is not SemVer-compatible with the changes in this pull request (compared against the base branch). Details |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #21737 +/- ##
==========================================
+ Coverage 82.49% 82.66% +0.16%
==========================================
Files 1140 1147 +7
Lines 438555 446575 +8020
Branches 438555 446575 +8020
==========================================
+ Hits 361803 369176 +7373
- Misses 54880 54997 +117
- Partials 21872 22402 +530 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
bot unstale ? |
|
maybe @jayzhan211 can help take a look at this |
jayzhan211
left a comment
There was a problem hiding this comment.
Thanks @theirix , here is a suggestion
| .collect::<Result<Vec<FieldRef>>>()?; | ||
| // Even with collecting results into a set, drop duplicates early | ||
| arg_fields.sort(); | ||
| arg_fields.dedup(); |
There was a problem hiding this comment.
Field's Ord compares name first, and every field is named arg_{i}, so dedup() never removes anything. sort() is a no-op up to 10 args, but from 11 args it reorders them lexicographically (arg_0, arg_1, arg_10, arg_2, …). The return type is then computed from permuted argument types, while the arg_types shown keep the original order.
Repro: a UDF with Signature::exact(vec![DataType::Int32; 10] + [DataType::Utf8]) whose return_type returns the last arg. get_udf_args_and_return_types reports args [Int32 ×10, String] with return type Some("Int32") instead of Some("String").
The BTreeSet collect already dedups rows, so drop both lines:
- let mut arg_fields = arg_types
+ let arg_fields = arg_types
.iter()
.enumerate()
.map(|(i, t)| resolve_informational_field(i, t))
.collect::<Result<Vec<FieldRef>>>()?;
- // Even with collecting results into a set, drop duplicates early
- arg_fields.sort();
- arg_fields.dedup();Please add a test with more than 10 heterogeneous args.
There was a problem hiding this comment.
Thank you, it's a good spot. Removed the dedup to have an expected result (backed by a unit test)
jayzhan211
left a comment
There was a problem hiding this comment.
Thanks @theirix , here are suggestions
| fn resolve_informational_field(idx: usize, t: &NativeType) -> Result<FieldRef> { | ||
| // Since a native type maps to several physical types, resolve it against `Null` data type | ||
| // to get the canonical `DataType` for the native type | ||
| let data_type = t.default_cast_for(&DataType::Null)?; |
There was a problem hiding this comment.
Resolving each NativeType against Null collapses String to Utf8View, so string functions that return Int64 for LargeUtf8 lose that row. Diffing information_schema.routines/parameters against the base shows the Int64 OUT row disappearing for bit_length, char_length, character_length, find_in_set, instr, length, levenshtein, octet_length, position, strpos. If that's intended, please pin it in information_schema.slt and mention it under user-facing changes:
query TT rowsort
select routine_name, data_type from information_schema.routines where routine_name = 'length';
----
length Int32
Which issue does this PR close?
NativeTypeinstead ofDataTypeforget_example_types#14761Rationale for this change
Moving from physical types:
get_example_typesand the information schema use ArrowDataType, but it is usually sufficient to use Datafusion'sNativeTypeinstead.Let's introduce a new API
get_representative_types, based onNativeType, and deprecate the publicget_example_typesAPIIt is a logical continuation of #15965
What changes are included in this PR?
get_representative_typesto provideNativeTypeviaLogicalType::default_cast_forUnionto that helperNUMERICSfinally (a brush-up for Refactor away usage ofNUMERICS/INTEGERSindatafusion/expr-common/src/type_coercion/aggregates.rs#18092)Are these changes tested?
Are there any user-facing changes?
TypeSignature::get_example_typesAPI