Repository navigation
feat: Use NativeType in get_example_type, information schema - #21737
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.75% 82.76%
========================================
Files 1147 1147
Lines 449881 450108 +227
Branches 449881 450108 +227
========================================
+ Hits 372306 372511 +205
- Misses 54905 54911 +6
- Partials 22670 22686 +16 ☔ 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 Int32There was a problem hiding this comment.
Yes, it's a tie between logical type and physical type. Changed to resolve ambiguity by checking more types, not just null. It allows emitting int32 and int64 as expected.
Also, refactored three udf/udaf/udwf functions into one to reduce code duplication.
jayzhan211
left a comment
There was a problem hiding this comment.
@theirix , still have 2 suggestions
| fn resolve_informational_fields(idx: usize, t: &NativeType) -> Result<Vec<FieldRef>> { | ||
| // Since native types map to several physical types, resolve it against | ||
| // ambiguous types to get canonical `DataType`s for the native type | ||
| let data_types = RESOLVE_CAST_SOURCES |
There was a problem hiding this comment.
default_cast_for(&LargeUtf8) has no arm for Struct/Map/Union (falls through to _internal_err!), and the ? here propagates it out of make_routines/make_parameters. So registering one UDF with such an argument (also nested, e.g. List<Struct>) makes information_schema.routines, information_schema.parameters and SHOW FUNCTIONS fail for every function. Before this PR the path was infallible. On head this test fails with Internal("Unavailable default cast for native type Struct(\"a\": Int32) from physical type LargeUtf8"):
#[test]
fn test_get_udf_args_and_return_types_nested() -> Result<()> {
use arrow::datatypes::Fields;
let struct_type =
DataType::Struct(Fields::from(vec![Field::new("a", DataType::Int32, true)]));
let signature = Signature::exact(vec![struct_type], Volatility::Stable);
let udf = Arc::new(ScalarUDF::from(TestScalarUDF { signature }));
let result = get_udf_args_and_return_types(&udf)?;
assert_eq!(result.len(), 1);
Ok(())
}Fix: skip origins the type has no cast from:
-fn resolve_informational_fields(idx: usize, t: &NativeType) -> Result<Vec<FieldRef>> {
+fn resolve_informational_fields(idx: usize, t: &NativeType) -> Vec<FieldRef> {
// Since native types map to several physical types, resolve it against
- // ambiguous types to get canonical `DataType`s for the native type
- let data_types = RESOLVE_CAST_SOURCES
+ // ambiguous types to get canonical `DataType`s for the native type.
+ // Skip origins the type has no cast from (e.g. `Struct` from `LargeUtf8`)
+ RESOLVE_CAST_SOURCES
.iter()
- .map(|source| t.default_cast_for(source))
- .collect::<Result<Vec<DataType>, _>>()?;
- Ok(data_types
- .into_iter()
+ .filter_map(|source| t.default_cast_for(source).ok())
.unique()
.map(|dt| Arc::new(Field::new(format!("arg_{idx}"), dt, true)))
- .collect())
+ .collect()
} .map(|(i, t)| resolve_informational_fields(i, t))
- .collect::<Result<Vec<_>>>()?;
+ .collect::<Vec<_>>();There was a problem hiding this comment.
It's reasonable, since default_cast_for cannot handle it on its own - added
| .map(|(i, t)| resolve_informational_fields(i, t)) | ||
| .collect::<Result<Vec<_>>>()?; | ||
| // Build combinations of arg types with the return type | ||
| let return_types = arg_fields |
There was a problem hiding this comment.
generate_series/range now report only (String, Date, Interval) → List(String), but the query returns List(Date32). Base also listed List(Date), but only because its Date64 example happened to hit (_, Some(Date64)) in Range::return_type. That match is lost now that Date resolves only to Date32. The underlying cause predates this PR: the return type is computed on arguments that haven't been coerced, which the get_representative_types docs say callers must do. Fine to handle in a follow-up.
select arrow_typeof(generate_series('2020-01-01', DATE '2020-01-03', INTERVAL '1 day'));
-- List(Date32)
select data_type from information_schema.parameters
where specific_name = 'generate_series' and parameter_mode = 'OUT';
-- has List(String) for (String, Date, Interval), no List(Date)Fix: coerce the arguments the same way the planner does before asking for the return type. I tried this locally: it fixes this row and also corrects sum(Int32) (NULL → Int64), trunc(Int64) (NULL → Float64), median(Int64) (Int64 → Float64) and date_trunc(String, Date) (Date → Timestamp(ns)), all checked against arrow_typeof. It does change the existing date_trunc expectations in information_schema.slt.
use datafusion_expr::function::WindowUDFFieldArgs;
+use datafusion_expr::type_coercion::functions::fields_with_udf; get_args_and_return_types(udf.signature(), |arg_fields| {
+ let arg_fields = &fields_with_udf(arg_fields, udf.as_ref())?;
let scalar_arguments = &vec![None; arg_fields.len()]; get_args_and_return_types(udaf.signature(), |arg_fields| {
- udaf.return_field(arg_fields)
+ udaf.return_field(&fields_with_udf(arg_fields, udaf.as_ref())?)
}) get_args_and_return_types(udwf.signature(), |arg_fields| {
+ let arg_fields = &fields_with_udf(arg_fields, udwf.as_ref())?;
udwf.field(WindowUDFFieldArgs::new(arg_fields, udwf.name()))There was a problem hiding this comment.
Added, tests are updated to check date_trunc and a new generate_series case
|
Merge main to re-trigger CI |
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