You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "theirix (via GitHub)" <gi...@apache.org> on 2026/04/19 20:04:05 UTC
[PR] feat: Use NativeType in get_example_type, information schema [datafusion]
theirix opened a new pull request, #21737:
URL: https://github.com/apache/datafusion/pull/21737
## Which issue does this PR close?
- Closes #14761.
## Rationale for this change
Moving from physical types: `get_example_types` and the information schema use Arrow `DataType`, but it is sufficient to use Datafusion's `NativeType` instead. It is a logical continuation of #15965
## What changes are included in this PR?
- Switch `get_example_types` to provide `NativeType`
- Remove `NUMERICS` finally (a brush-up for #18092)
- Switch information schema to use native type.
- Repurpose the old `get_data_types` helper `get_data_type_for_schema` to resolve a NativeType back to the most appropriate DataType to show in schema
- Add missing conversions for `Struct`, `Union`, `Map` to this helper
## Are these changes tested?
Tests are passing
## Are there any user-facing changes?
No
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: Use NativeType in get_example_type, information schema [datafusion]
Posted by "theirix (via GitHub)" <gi...@apache.org>.
theirix commented on code in PR #21737:
URL: https://github.com/apache/datafusion/pull/21737#discussion_r3112566390
##########
datafusion/catalog/src/information_schema.rs:
##########
@@ -411,6 +412,127 @@ impl InformationSchemaConfig {
}
}
+/// Resolve a native type `NativeType` to `DataType` for use in the information schema
+/// Since it is one-to-many, use the most representative type on tie
+fn get_data_type_for_schema(native_type: &NativeType) -> Option<DataType> {
+ match native_type {
+ NativeType::Null => Some(DataType::Null),
+ NativeType::Boolean => Some(DataType::Boolean),
+ NativeType::Int8 => Some(DataType::Int8),
+ NativeType::Int16 => Some(DataType::Int16),
+ NativeType::Int32 => Some(DataType::Int32),
+ NativeType::Int64 => Some(DataType::Int64),
+ NativeType::UInt8 => Some(DataType::UInt8),
+ NativeType::UInt16 => Some(DataType::UInt16),
+ NativeType::UInt32 => Some(DataType::UInt32),
+ NativeType::UInt64 => Some(DataType::UInt64),
+ NativeType::Float16 => Some(DataType::Float16),
+ NativeType::Float32 => Some(DataType::Float32),
+ NativeType::Float64 => Some(DataType::Float64),
+ NativeType::Date => Some(DataType::Date32), // A tie
+ NativeType::Binary => Some(DataType::Binary), // A tie
+ NativeType::String => Some(DataType::Utf8), // A tie
+ NativeType::Decimal(precision, scale) => {
+ Some(DataType::Decimal256(*precision, *scale)) // A tie, use the widest type
+ }
+ NativeType::Timestamp(time_unit, timezone) => {
+ Some(DataType::Timestamp(*time_unit, timezone.to_owned()))
+ }
+ NativeType::Time(TimeUnit::Second) => Some(DataType::Time32(TimeUnit::Second)),
+ NativeType::Time(TimeUnit::Millisecond) => {
+ Some(DataType::Time32(TimeUnit::Millisecond))
+ }
+ NativeType::Time(TimeUnit::Microsecond) => {
+ Some(DataType::Time64(TimeUnit::Microsecond))
+ }
+ NativeType::Time(TimeUnit::Nanosecond) => {
+ Some(DataType::Time64(TimeUnit::Nanosecond))
+ }
+ NativeType::Duration(time_unit) => Some(DataType::Duration(*time_unit)),
+ NativeType::Interval(interval_unit) => Some(DataType::Interval(*interval_unit)),
+ NativeType::FixedSizeBinary(size) => Some(DataType::FixedSizeBinary(*size)),
+ NativeType::FixedSizeList(logical_field, size) => get_data_type_for_schema(
+ logical_field.logical_type.native(),
+ )
+ .map(|child_dt| {
+ DataType::FixedSizeList(
+ Arc::new(Field::new(
+ logical_field.name.clone(),
+ child_dt,
+ logical_field.nullable,
+ )),
+ *size,
+ )
+ }),
+ NativeType::List(logical_field) => get_data_type_for_schema(
+ logical_field.logical_type.native(),
+ )
+ .map(|child_dt| {
+ // A tie, use List
+ DataType::List(Arc::new(Field::new(
+ logical_field.name.clone(),
+ child_dt,
+ logical_field.nullable,
+ )))
+ }),
+ NativeType::Struct(logical_fields) => {
+ let fields = logical_fields
+ .iter()
+ .map(|logical_field| {
+ let dt =
+ get_data_type_for_schema(logical_field.logical_type.native())?;
+ Some(Arc::new(Field::new(
+ logical_field.name.clone(),
+ dt,
+ logical_field.nullable,
+ )))
+ })
+ .collect::<Option<Fields>>()?;
+ Some(DataType::Struct(fields))
+ }
+ NativeType::Union(logical_fields) => {
+ let ids = logical_fields.iter().map(|(i, _)| *i).collect::<Vec<i8>>();
+ let fields: Vec<FieldRef> = logical_fields
+ .iter()
+ .map(|(_, logical_field)| {
+ let dt =
+ get_data_type_for_schema(logical_field.logical_type.native())?;
+ Some(Arc::new(Field::new(
+ logical_field.name.clone(),
+ dt,
+ logical_field.nullable,
Review Comment:
It exists only in an arrow schema Field, not in a logical field or DataType
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: Use NativeType in get_example_type, information schema [datafusion]
Posted by "theirix (via GitHub)" <gi...@apache.org>.
theirix commented on PR #21737:
URL: https://github.com/apache/datafusion/pull/21737#issuecomment-5330109438
bot unstale ?
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: Use NativeType in get_example_type, information schema [datafusion]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #21737:
URL: https://github.com/apache/datafusion/pull/21737#issuecomment-5288630488
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.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: Use NativeType in get_example_type, information schema [datafusion]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #21737:
URL: https://github.com/apache/datafusion/pull/21737#issuecomment-5328638929
<!-- semver-check-comment -->
Thank you for opening this pull request!
Reviewer note: [cargo-semver-checks](https://github.com/obi1kenobi/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>
<summary>Details</summary>
```
Cloning apache/main
Building datafusion-catalog v54.1.0 (current)
Built [ 48.210s] (current)
Parsing datafusion-catalog v54.1.0 (current)
Parsed [ 0.026s] (current)
Building datafusion-catalog v54.1.0 (baseline)
Built [ 40.001s] (baseline)
Parsing datafusion-catalog v54.1.0 (baseline)
Parsed [ 0.026s] (baseline)
Checking datafusion-catalog v54.1.0 -> v54.1.0 (no change; assume patch)
Checked [ 0.170s] 223 checks: 223 pass, 31 skip
Summary no semver update required
Finished [ 89.776s] datafusion-catalog
Building datafusion-common v54.1.0 (current)
Built [ 32.907s] (current)
Parsing datafusion-common v54.1.0 (current)
Parsed [ 0.065s] (current)
Building datafusion-common v54.1.0 (baseline)
Built [ 32.559s] (baseline)
Parsing datafusion-common v54.1.0 (baseline)
Parsed [ 0.067s] (baseline)
Checking datafusion-common v54.1.0 -> v54.1.0 (no change; assume patch)
Checked [ 1.039s] 223 checks: 223 pass, 31 skip
Summary no semver update required
Finished [ 68.453s] datafusion-common
Building datafusion-expr-common v54.1.0 (current)
Built [ 18.693s] (current)
Parsing datafusion-expr-common v54.1.0 (current)
Parsed [ 0.019s] (current)
Building datafusion-expr-common v54.1.0 (baseline)
Built [ 19.034s] (baseline)
Parsing datafusion-expr-common v54.1.0 (baseline)
Parsed [ 0.019s] (baseline)
Checking datafusion-expr-common v54.1.0 -> v54.1.0 (no change; assume patch)
Checked [ 0.328s] 223 checks: 222 pass, 1 fail, 0 warn, 31 skip
--- failure type_method_marked_deprecated: type method #[deprecated] added ---
Description:
A type method is now #[deprecated]. Downstream crates will get a compiler warning when using this method.
ref: https://doc.rust-lang.org/reference/attributes/diagnostics.html#the-deprecated-attribute
impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.50.0/src/lints/type_method_marked_deprecated.ron
Failed in:
method datafusion_expr_common::signature::TypeSignature::get_example_types in /home/runner/work/datafusion/datafusion/datafusion/expr-common/src/signature.rs:934
Summary semver requires new minor version: 0 major and 1 minor checks failed
Finished [ 38.769s] datafusion-expr-common
Building datafusion-sqllogictest v54.1.0 (current)
Built [ 98.652s] (current)
Parsing datafusion-sqllogictest v54.1.0 (current)
Parsed [ 0.023s] (current)
Building datafusion-sqllogictest v54.1.0 (baseline)
Built [ 98.526s] (baseline)
Parsing datafusion-sqllogictest v54.1.0 (baseline)
Parsed [ 0.025s] (baseline)
Checking datafusion-sqllogictest v54.1.0 -> v54.1.0 (no change; assume patch)
Checked [ 0.122s] 223 checks: 223 pass, 31 skip
Summary no semver update required
Finished [ 200.545s] datafusion-sqllogictest
```
</details>
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: Use NativeType in get_example_type, information schema [datafusion]
Posted by "theirix (via GitHub)" <gi...@apache.org>.
theirix commented on code in PR #21737:
URL: https://github.com/apache/datafusion/pull/21737#discussion_r4183587271
##########
datafusion/catalog/src/information_schema.rs:
##########
@@ -454,26 +454,32 @@ impl InformationSchemaConfig {
}
}
+/// Build the argument field for `information_schema` to provide a return type
+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)?;
Review 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.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: Use NativeType in get_example_type, information schema [datafusion]
Posted by "martin-g (via GitHub)" <gi...@apache.org>.
martin-g commented on code in PR #21737:
URL: https://github.com/apache/datafusion/pull/21737#discussion_r3108825521
##########
datafusion/catalog/src/information_schema.rs:
##########
@@ -411,6 +412,127 @@ impl InformationSchemaConfig {
}
}
+/// Resolve a native type `NativeType` to `DataType` for use in the information schema
+/// Since it is one-to-many, use the most representative type on tie
+fn get_data_type_for_schema(native_type: &NativeType) -> Option<DataType> {
+ match native_type {
+ NativeType::Null => Some(DataType::Null),
+ NativeType::Boolean => Some(DataType::Boolean),
+ NativeType::Int8 => Some(DataType::Int8),
+ NativeType::Int16 => Some(DataType::Int16),
+ NativeType::Int32 => Some(DataType::Int32),
+ NativeType::Int64 => Some(DataType::Int64),
+ NativeType::UInt8 => Some(DataType::UInt8),
+ NativeType::UInt16 => Some(DataType::UInt16),
+ NativeType::UInt32 => Some(DataType::UInt32),
+ NativeType::UInt64 => Some(DataType::UInt64),
+ NativeType::Float16 => Some(DataType::Float16),
+ NativeType::Float32 => Some(DataType::Float32),
+ NativeType::Float64 => Some(DataType::Float64),
+ NativeType::Date => Some(DataType::Date32), // A tie
+ NativeType::Binary => Some(DataType::Binary), // A tie
+ NativeType::String => Some(DataType::Utf8), // A tie
+ NativeType::Decimal(precision, scale) => {
+ Some(DataType::Decimal256(*precision, *scale)) // A tie, use the widest type
+ }
+ NativeType::Timestamp(time_unit, timezone) => {
+ Some(DataType::Timestamp(*time_unit, timezone.to_owned()))
+ }
+ NativeType::Time(TimeUnit::Second) => Some(DataType::Time32(TimeUnit::Second)),
+ NativeType::Time(TimeUnit::Millisecond) => {
+ Some(DataType::Time32(TimeUnit::Millisecond))
+ }
+ NativeType::Time(TimeUnit::Microsecond) => {
+ Some(DataType::Time64(TimeUnit::Microsecond))
+ }
+ NativeType::Time(TimeUnit::Nanosecond) => {
+ Some(DataType::Time64(TimeUnit::Nanosecond))
+ }
+ NativeType::Duration(time_unit) => Some(DataType::Duration(*time_unit)),
+ NativeType::Interval(interval_unit) => Some(DataType::Interval(*interval_unit)),
+ NativeType::FixedSizeBinary(size) => Some(DataType::FixedSizeBinary(*size)),
+ NativeType::FixedSizeList(logical_field, size) => get_data_type_for_schema(
+ logical_field.logical_type.native(),
+ )
+ .map(|child_dt| {
+ DataType::FixedSizeList(
+ Arc::new(Field::new(
+ logical_field.name.clone(),
+ child_dt,
+ logical_field.nullable,
+ )),
+ *size,
+ )
+ }),
+ NativeType::List(logical_field) => get_data_type_for_schema(
+ logical_field.logical_type.native(),
+ )
+ .map(|child_dt| {
+ // A tie, use List
+ DataType::List(Arc::new(Field::new(
+ logical_field.name.clone(),
+ child_dt,
+ logical_field.nullable,
+ )))
+ }),
+ NativeType::Struct(logical_fields) => {
+ let fields = logical_fields
+ .iter()
+ .map(|logical_field| {
+ let dt =
+ get_data_type_for_schema(logical_field.logical_type.native())?;
+ Some(Arc::new(Field::new(
+ logical_field.name.clone(),
+ dt,
+ logical_field.nullable,
+ )))
+ })
+ .collect::<Option<Fields>>()?;
+ Some(DataType::Struct(fields))
+ }
+ NativeType::Union(logical_fields) => {
+ let ids = logical_fields.iter().map(|(i, _)| *i).collect::<Vec<i8>>();
+ let fields: Vec<FieldRef> = logical_fields
+ .iter()
+ .map(|(_, logical_field)| {
+ let dt =
+ get_data_type_for_schema(logical_field.logical_type.native())?;
+ Some(Arc::new(Field::new(
+ logical_field.name.clone(),
+ dt,
+ logical_field.nullable,
Review Comment:
Should the metadata be preserved as well ?
##########
datafusion/expr-common/src/signature.rs:
##########
@@ -595,18 +589,25 @@ impl Display for ArrayFunctionArgument {
}
}
-static NUMERICS: &[DataType] = &[
- DataType::Int8,
- DataType::Int16,
- DataType::Int32,
- DataType::Int64,
- DataType::UInt8,
- DataType::UInt16,
- DataType::UInt32,
- DataType::UInt64,
- DataType::Float16,
- DataType::Float32,
- DataType::Float64,
+/// Constant that is used as a Decimal type for `get_example_types`
+/// Use Decimal256 precision as a reasonable default
+const NATIVE_TYPE_DECIMAL: NativeType =
+ NativeType::Decimal(DECIMAL256_MAX_PRECISION, DECIMAL256_MAX_SCALE);
Review Comment:
precision == scale == 76 => no room for integer digits
```suggestion
NativeType::Decimal(DECIMAL256_MAX_PRECISION, DECIMAL_DEFAULT_SCALE);
```
##########
datafusion/expr-common/src/signature.rs:
##########
@@ -881,36 +882,41 @@ impl TypeSignature {
}
#[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>> {
Review Comment:
Hm. A public API break for a deprecated method ... Maybe it is time to just remove it ?!
##########
datafusion/catalog/src/information_schema.rs:
##########
@@ -411,6 +412,127 @@ impl InformationSchemaConfig {
}
}
+/// Resolve a native type `NativeType` to `DataType` for use in the information schema
+/// Since it is one-to-many, use the most representative type on tie
+fn get_data_type_for_schema(native_type: &NativeType) -> Option<DataType> {
+ match native_type {
+ NativeType::Null => Some(DataType::Null),
+ NativeType::Boolean => Some(DataType::Boolean),
+ NativeType::Int8 => Some(DataType::Int8),
+ NativeType::Int16 => Some(DataType::Int16),
+ NativeType::Int32 => Some(DataType::Int32),
+ NativeType::Int64 => Some(DataType::Int64),
+ NativeType::UInt8 => Some(DataType::UInt8),
+ NativeType::UInt16 => Some(DataType::UInt16),
+ NativeType::UInt32 => Some(DataType::UInt32),
+ NativeType::UInt64 => Some(DataType::UInt64),
+ NativeType::Float16 => Some(DataType::Float16),
+ NativeType::Float32 => Some(DataType::Float32),
+ NativeType::Float64 => Some(DataType::Float64),
+ NativeType::Date => Some(DataType::Date32), // A tie
+ NativeType::Binary => Some(DataType::Binary), // A tie
+ NativeType::String => Some(DataType::Utf8), // A tie
+ NativeType::Decimal(precision, scale) => {
+ Some(DataType::Decimal256(*precision, *scale)) // A tie, use the widest type
+ }
+ NativeType::Timestamp(time_unit, timezone) => {
+ Some(DataType::Timestamp(*time_unit, timezone.to_owned()))
+ }
+ NativeType::Time(TimeUnit::Second) => Some(DataType::Time32(TimeUnit::Second)),
+ NativeType::Time(TimeUnit::Millisecond) => {
+ Some(DataType::Time32(TimeUnit::Millisecond))
+ }
+ NativeType::Time(TimeUnit::Microsecond) => {
+ Some(DataType::Time64(TimeUnit::Microsecond))
+ }
+ NativeType::Time(TimeUnit::Nanosecond) => {
+ Some(DataType::Time64(TimeUnit::Nanosecond))
+ }
+ NativeType::Duration(time_unit) => Some(DataType::Duration(*time_unit)),
+ NativeType::Interval(interval_unit) => Some(DataType::Interval(*interval_unit)),
+ NativeType::FixedSizeBinary(size) => Some(DataType::FixedSizeBinary(*size)),
+ NativeType::FixedSizeList(logical_field, size) => get_data_type_for_schema(
+ logical_field.logical_type.native(),
+ )
+ .map(|child_dt| {
+ DataType::FixedSizeList(
+ Arc::new(Field::new(
+ logical_field.name.clone(),
+ child_dt,
+ logical_field.nullable,
+ )),
+ *size,
+ )
+ }),
+ NativeType::List(logical_field) => get_data_type_for_schema(
+ logical_field.logical_type.native(),
+ )
+ .map(|child_dt| {
+ // A tie, use List
+ DataType::List(Arc::new(Field::new(
+ logical_field.name.clone(),
+ child_dt,
+ logical_field.nullable,
+ )))
+ }),
+ NativeType::Struct(logical_fields) => {
+ let fields = logical_fields
+ .iter()
+ .map(|logical_field| {
+ let dt =
+ get_data_type_for_schema(logical_field.logical_type.native())?;
+ Some(Arc::new(Field::new(
+ logical_field.name.clone(),
+ dt,
+ logical_field.nullable,
+ )))
+ })
+ .collect::<Option<Fields>>()?;
+ Some(DataType::Struct(fields))
+ }
+ NativeType::Union(logical_fields) => {
+ let ids = logical_fields.iter().map(|(i, _)| *i).collect::<Vec<i8>>();
+ let fields: Vec<FieldRef> = logical_fields
+ .iter()
+ .map(|(_, logical_field)| {
+ let dt =
+ get_data_type_for_schema(logical_field.logical_type.native())?;
+ Some(Arc::new(Field::new(
+ logical_field.name.clone(),
+ dt,
+ logical_field.nullable,
+ )))
+ })
+ .collect::<Option<Vec<FieldRef>>>()?;
+ Some(DataType::Union(
+ UnionFields::try_new(ids, fields).ok()?,
+ UnionMode::Dense,
+ ))
+ }
+ NativeType::Map(logical_field) => get_data_type_for_schema(
+ logical_field.logical_type.native(),
+ )
+ .map(|child_dt| {
+ DataType::Map(
+ Arc::new(Field::new(
+ logical_field.name.clone(),
+ child_dt,
+ logical_field.nullable,
+ )),
+ true,
+ )
+ }),
+ }
+}
+
+pub fn resolve_informational_field(idx: usize, t: &NativeType) -> Result<FieldRef> {
Review Comment:
```suggestion
fn resolve_informational_field(idx: usize, t: &NativeType) -> Result<FieldRef> {
```
##########
datafusion/catalog/src/information_schema.rs:
##########
@@ -411,6 +412,127 @@ impl InformationSchemaConfig {
}
}
+/// Resolve a native type `NativeType` to `DataType` for use in the information schema
+/// Since it is one-to-many, use the most representative type on tie
+fn get_data_type_for_schema(native_type: &NativeType) -> Option<DataType> {
+ match native_type {
+ NativeType::Null => Some(DataType::Null),
+ NativeType::Boolean => Some(DataType::Boolean),
+ NativeType::Int8 => Some(DataType::Int8),
+ NativeType::Int16 => Some(DataType::Int16),
+ NativeType::Int32 => Some(DataType::Int32),
+ NativeType::Int64 => Some(DataType::Int64),
+ NativeType::UInt8 => Some(DataType::UInt8),
+ NativeType::UInt16 => Some(DataType::UInt16),
+ NativeType::UInt32 => Some(DataType::UInt32),
+ NativeType::UInt64 => Some(DataType::UInt64),
+ NativeType::Float16 => Some(DataType::Float16),
+ NativeType::Float32 => Some(DataType::Float32),
+ NativeType::Float64 => Some(DataType::Float64),
+ NativeType::Date => Some(DataType::Date32), // A tie
+ NativeType::Binary => Some(DataType::Binary), // A tie
+ NativeType::String => Some(DataType::Utf8), // A tie
+ NativeType::Decimal(precision, scale) => {
+ Some(DataType::Decimal256(*precision, *scale)) // A tie, use the widest type
+ }
+ NativeType::Timestamp(time_unit, timezone) => {
+ Some(DataType::Timestamp(*time_unit, timezone.to_owned()))
+ }
+ NativeType::Time(TimeUnit::Second) => Some(DataType::Time32(TimeUnit::Second)),
+ NativeType::Time(TimeUnit::Millisecond) => {
+ Some(DataType::Time32(TimeUnit::Millisecond))
+ }
+ NativeType::Time(TimeUnit::Microsecond) => {
+ Some(DataType::Time64(TimeUnit::Microsecond))
+ }
+ NativeType::Time(TimeUnit::Nanosecond) => {
+ Some(DataType::Time64(TimeUnit::Nanosecond))
+ }
+ NativeType::Duration(time_unit) => Some(DataType::Duration(*time_unit)),
+ NativeType::Interval(interval_unit) => Some(DataType::Interval(*interval_unit)),
+ NativeType::FixedSizeBinary(size) => Some(DataType::FixedSizeBinary(*size)),
+ NativeType::FixedSizeList(logical_field, size) => get_data_type_for_schema(
+ logical_field.logical_type.native(),
+ )
+ .map(|child_dt| {
+ DataType::FixedSizeList(
+ Arc::new(Field::new(
+ logical_field.name.clone(),
+ child_dt,
+ logical_field.nullable,
+ )),
+ *size,
+ )
+ }),
+ NativeType::List(logical_field) => get_data_type_for_schema(
+ logical_field.logical_type.native(),
+ )
+ .map(|child_dt| {
+ // A tie, use List
+ DataType::List(Arc::new(Field::new(
+ logical_field.name.clone(),
+ child_dt,
+ logical_field.nullable,
+ )))
+ }),
+ NativeType::Struct(logical_fields) => {
+ let fields = logical_fields
+ .iter()
+ .map(|logical_field| {
+ let dt =
+ get_data_type_for_schema(logical_field.logical_type.native())?;
+ Some(Arc::new(Field::new(
+ logical_field.name.clone(),
+ dt,
+ logical_field.nullable,
+ )))
+ })
+ .collect::<Option<Fields>>()?;
+ Some(DataType::Struct(fields))
+ }
+ NativeType::Union(logical_fields) => {
+ let ids = logical_fields.iter().map(|(i, _)| *i).collect::<Vec<i8>>();
+ let fields: Vec<FieldRef> = logical_fields
+ .iter()
+ .map(|(_, logical_field)| {
+ let dt =
+ get_data_type_for_schema(logical_field.logical_type.native())?;
+ Some(Arc::new(Field::new(
+ logical_field.name.clone(),
+ dt,
+ logical_field.nullable,
+ )))
+ })
+ .collect::<Option<Vec<FieldRef>>>()?;
+ Some(DataType::Union(
+ UnionFields::try_new(ids, fields).ok()?,
+ UnionMode::Dense,
+ ))
+ }
+ NativeType::Map(logical_field) => get_data_type_for_schema(
+ logical_field.logical_type.native(),
+ )
+ .map(|child_dt| {
+ DataType::Map(
+ Arc::new(Field::new(
+ logical_field.name.clone(),
+ child_dt,
+ logical_field.nullable,
+ )),
+ true,
+ )
+ }),
+ }
+}
+
+pub fn resolve_informational_field(idx: usize, t: &NativeType) -> Result<FieldRef> {
+ if let Some(data_type) = get_data_type_for_schema(t) {
Review Comment:
It would be better `get_data_type_for_schema()` to return a `Result` and return an Err instead of None when some type is not resolved.
Currently it will use `t` for the error but for nested types (like Struct, List, Union, ...) the problem might be in a child.
##########
datafusion/expr-common/src/signature.rs:
##########
@@ -595,18 +589,25 @@ impl Display for ArrayFunctionArgument {
}
}
-static NUMERICS: &[DataType] = &[
- DataType::Int8,
- DataType::Int16,
- DataType::Int32,
- DataType::Int64,
- DataType::UInt8,
- DataType::UInt16,
- DataType::UInt32,
- DataType::UInt64,
- DataType::Float16,
- DataType::Float32,
- DataType::Float64,
+/// Constant that is used as a Decimal type for `get_example_types`
+/// Use Decimal256 precision as a reasonable default
Review Comment:
nit: Decimal128 looks more reasonable to me.
76-digit precision is used for special cases.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: Use NativeType in get_example_type, information schema [datafusion]
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 commented on code in PR #21737:
URL: https://github.com/apache/datafusion/pull/21737#discussion_r4155661435
##########
parquet-testing:
##########
Review Comment:
Do we need this change?
##########
datafusion/catalog/src/information_schema.rs:
##########
@@ -454,26 +454,32 @@ impl InformationSchemaConfig {
}
}
+/// Build the argument field for `information_schema` to provide a return type
+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)?;
Review 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:
```sql
query TT rowsort
select routine_name, data_type from information_schema.routines where routine_name = 'length';
----
length Int32
```
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: Use NativeType in get_example_type, information schema [datafusion]
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 commented on code in PR #21737:
URL: https://github.com/apache/datafusion/pull/21737#discussion_r4093491057
##########
datafusion/catalog/src/information_schema.rs:
##########
@@ -454,26 +454,35 @@ impl InformationSchemaConfig {
}
}
+/// Build the argument field for `information_schema` to provide a return type
+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)?;
+ Ok(Arc::new(Field::new(format!("arg_{idx}"), data_type, true)))
+}
+
/// get the arguments and return types of a UDF
/// returns a tuple of (arg_types, return_type)
fn get_udf_args_and_return_types(
udf: &Arc<ScalarUDF>,
) -> Result<BTreeSet<(Vec<String>, Option<String>)>> {
let signature = udf.signature();
- let arg_types = signature.type_signature.get_example_types();
+ let arg_types = signature.type_signature.get_representative_types();
if arg_types.is_empty() {
Ok(vec![(vec![], None)].into_iter().collect::<BTreeSet<_>>())
} else {
- Ok(arg_types
+ arg_types
.into_iter()
.map(|arg_types| {
- let arg_fields: Vec<FieldRef> = arg_types
+ let mut arg_fields = arg_types
.iter()
.enumerate()
- .map(|(i, t)| {
- Arc::new(Field::new(format!("arg_{i}"), t.clone(), true))
- })
- .collect();
+ .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();
Review 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:
```diff
- 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.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: Use NativeType in get_example_type, information schema [datafusion]
Posted by "theirix (via GitHub)" <gi...@apache.org>.
theirix commented on code in PR #21737:
URL: https://github.com/apache/datafusion/pull/21737#discussion_r4188692946
##########
datafusion/catalog/src/information_schema.rs:
##########
@@ -454,118 +455,98 @@ impl InformationSchemaConfig {
}
}
-/// get the arguments and return types of a UDF
-/// returns a tuple of (arg_types, return_type)
+/// Origins used to enumerate the physical types a native type can take
+const RESOLVE_CAST_SOURCES: [DataType; 2] = [DataType::Null, DataType::LargeUtf8];
+
+/// Build argument fields for `information_schema` to provide possible return types
+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
+ .iter()
+ .map(|source| t.default_cast_for(source))
+ .collect::<Result<Vec<DataType>, _>>()?;
+ Ok(data_types
+ .into_iter()
+ .unique()
+ .map(|dt| Arc::new(Field::new(format!("arg_{idx}"), dt, true)))
+ .collect())
+}
+
+/// Function information schema is a set of tuples - argument types and an optional return type
+type FunctionInformationSchema = BTreeSet<(Vec<String>, Option<String>)>;
+
+/// Get the arguments and return types of a function from its signature
+fn get_args_and_return_types(
+ signature: &Signature,
+ return_field: impl Fn(&[FieldRef]) -> Result<FieldRef>,
+) -> Result<FunctionInformationSchema> {
+ let arg_types = signature.type_signature.get_representative_types();
+ if arg_types.is_empty() {
+ // Edge case if function doesn't have arguments
+ return Ok(BTreeSet::from([(vec![], None)]));
+ }
+ arg_types
+ .into_iter()
+ .map(|arg_types| {
+ // Get possible types for each input arg
+ let arg_fields = arg_types
+ .iter()
+ .enumerate()
+ .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
Review Comment:
Added, tests are updated to check `date_trunc` and a new `generate_series` case
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: Use NativeType in get_example_type, information schema [datafusion]
Posted by "theirix (via GitHub)" <gi...@apache.org>.
theirix commented on code in PR #21737:
URL: https://github.com/apache/datafusion/pull/21737#discussion_r3141880544
##########
datafusion/expr-common/src/signature.rs:
##########
@@ -881,36 +882,41 @@ impl TypeSignature {
}
#[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>> {
Review 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.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: Use NativeType in get_example_type, information schema [datafusion]
Posted by "theirix (via GitHub)" <gi...@apache.org>.
theirix commented on code in PR #21737:
URL: https://github.com/apache/datafusion/pull/21737#discussion_r3109297573
##########
datafusion/expr-common/src/signature.rs:
##########
@@ -881,36 +882,41 @@ impl TypeSignature {
}
#[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>> {
Review 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`?
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: Use NativeType in get_example_type, information schema [datafusion]
Posted by "martin-g (via GitHub)" <gi...@apache.org>.
martin-g commented on code in PR #21737:
URL: https://github.com/apache/datafusion/pull/21737#discussion_r3124329748
##########
datafusion/expr-common/src/signature.rs:
##########
@@ -416,11 +417,12 @@ impl TypeSignatureClass {
///
/// This is used for `information_schema` and can be used to generate
/// documentation or error messages.
+ /// Remove with `get_example_types`
Review Comment:
This should be a comment (`//`) instead of rustdoc (`///`)
##########
datafusion/expr-common/src/signature.rs:
##########
@@ -881,36 +882,41 @@ impl TypeSignature {
}
#[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>> {
Review 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.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: Use NativeType in get_example_type, information schema [datafusion]
Posted by "Jefffrey (via GitHub)" <gi...@apache.org>.
Jefffrey commented on code in PR #21737:
URL: https://github.com/apache/datafusion/pull/21737#discussion_r3408970299
##########
datafusion/catalog/src/information_schema.rs:
##########
@@ -411,26 +412,136 @@ impl InformationSchemaConfig {
}
}
+/// Resolve a native type `NativeType` to `DataType` for use in the information schema
+/// Since it is one-to-many, use the most representative type on tie
+fn get_data_type_for_schema(native_type: &NativeType) -> Result<DataType> {
+ match native_type {
+ NativeType::Null => Ok(DataType::Null),
+ NativeType::Boolean => Ok(DataType::Boolean),
+ NativeType::Int8 => Ok(DataType::Int8),
+ NativeType::Int16 => Ok(DataType::Int16),
+ NativeType::Int32 => Ok(DataType::Int32),
Review 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 🤔
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: Use NativeType in get_example_type, information schema [datafusion]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #21737:
URL: https://github.com/apache/datafusion/pull/21737#issuecomment-5328849496
## [Codecov](https://app.codecov.io/gh/apache/datafusion/pull/21737?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `97.54098%` with `6 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 81.26%. Comparing base ([`bbf739a`](https://app.codecov.io/gh/apache/datafusion/commit/bbf739a3cfe1d5cc7646e37250c947c6a56b5b0e?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`687fa6a`](https://app.codecov.io/gh/apache/datafusion/commit/687fa6a4c4df22c1795c2f4d37b9403e5c1483fd?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 8 commits behind head on main.
| [Files with missing lines](https://app.codecov.io/gh/apache/datafusion/pull/21737?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [datafusion/catalog/src/information\_schema.rs](https://app.codecov.io/gh/apache/datafusion/pull/21737?src=pr&el=tree&filepath=datafusion%2Fcatalog%2Fsrc%2Finformation_schema.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-ZGF0YWZ1c2lvbi9jYXRhbG9nL3NyYy9pbmZvcm1hdGlvbl9zY2hlbWEucnM=) | 88.23% | [0 Missing and 4 partials :warning: ](https://app.codecov.io/gh/apache/datafusion/pull/21737?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [datafusion/common/src/types/native.rs](https://app.codecov.io/gh/apache/datafusion/pull/21737?src=pr&el=tree&filepath=datafusion%2Fcommon%2Fsrc%2Ftypes%2Fnative.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-ZGF0YWZ1c2lvbi9jb21tb24vc3JjL3R5cGVzL25hdGl2ZS5ycw==) | 97.53% | [0 Missing and 2 partials :warning: ](https://app.codecov.io/gh/apache/datafusion/pull/21737?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## main #21737 +/- ##
==========================================
+ Coverage 81.23% 81.26% +0.03%
==========================================
Files 1111 1112 +1
Lines 390208 391791 +1583
Branches 390208 391791 +1583
==========================================
+ Hits 316990 318400 +1410
- Misses 54591 54670 +79
- Partials 18627 18721 +94
```
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/datafusion/pull/21737?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: Use NativeType in get_example_type, information schema [datafusion]
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 commented on PR #21737:
URL: https://github.com/apache/datafusion/pull/21737#issuecomment-6071836366
Merge main to re-trigger CI
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: Use NativeType in get_example_type, information schema [datafusion]
Posted by "theirix (via GitHub)" <gi...@apache.org>.
theirix commented on code in PR #21737:
URL: https://github.com/apache/datafusion/pull/21737#discussion_r3120111920
##########
datafusion/expr-common/src/signature.rs:
##########
@@ -881,36 +882,41 @@ impl TypeSignature {
}
#[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>> {
Review 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
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: Use NativeType in get_example_type, information schema [datafusion]
Posted by "theirix (via GitHub)" <gi...@apache.org>.
theirix commented on code in PR #21737:
URL: https://github.com/apache/datafusion/pull/21737#discussion_r3804653416
##########
datafusion/catalog/src/information_schema.rs:
##########
@@ -411,26 +412,136 @@ impl InformationSchemaConfig {
}
}
+/// Resolve a native type `NativeType` to `DataType` for use in the information schema
+/// Since it is one-to-many, use the most representative type on tie
+fn get_data_type_for_schema(native_type: &NativeType) -> Result<DataType> {
+ match native_type {
+ NativeType::Null => Ok(DataType::Null),
+ NativeType::Boolean => Ok(DataType::Boolean),
+ NativeType::Int8 => Ok(DataType::Int8),
+ NativeType::Int16 => Ok(DataType::Int16),
+ NativeType::Int32 => Ok(DataType::Int32),
Review 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`
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: Use NativeType in get_example_type, information schema [datafusion]
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 merged PR #21737:
URL: https://github.com/apache/datafusion/pull/21737
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: Use NativeType in get_example_type, information schema [datafusion]
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 commented on code in PR #21737:
URL: https://github.com/apache/datafusion/pull/21737#discussion_r4184652052
##########
datafusion/catalog/src/information_schema.rs:
##########
@@ -454,118 +455,98 @@ impl InformationSchemaConfig {
}
}
-/// get the arguments and return types of a UDF
-/// returns a tuple of (arg_types, return_type)
+/// Origins used to enumerate the physical types a native type can take
+const RESOLVE_CAST_SOURCES: [DataType; 2] = [DataType::Null, DataType::LargeUtf8];
+
+/// Build argument fields for `information_schema` to provide possible return types
+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
Review 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")`:
```rs
#[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:
```diff
-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()
}
```
```diff
.map(|(i, t)| resolve_informational_fields(i, t))
- .collect::<Result<Vec<_>>>()?;
+ .collect::<Vec<_>>();
```
##########
datafusion/catalog/src/information_schema.rs:
##########
@@ -454,118 +455,98 @@ impl InformationSchemaConfig {
}
}
-/// get the arguments and return types of a UDF
-/// returns a tuple of (arg_types, return_type)
+/// Origins used to enumerate the physical types a native type can take
+const RESOLVE_CAST_SOURCES: [DataType; 2] = [DataType::Null, DataType::LargeUtf8];
+
+/// Build argument fields for `information_schema` to provide possible return types
+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
+ .iter()
+ .map(|source| t.default_cast_for(source))
+ .collect::<Result<Vec<DataType>, _>>()?;
+ Ok(data_types
+ .into_iter()
+ .unique()
+ .map(|dt| Arc::new(Field::new(format!("arg_{idx}"), dt, true)))
+ .collect())
+}
+
+/// Function information schema is a set of tuples - argument types and an optional return type
+type FunctionInformationSchema = BTreeSet<(Vec<String>, Option<String>)>;
+
+/// Get the arguments and return types of a function from its signature
+fn get_args_and_return_types(
+ signature: &Signature,
+ return_field: impl Fn(&[FieldRef]) -> Result<FieldRef>,
+) -> Result<FunctionInformationSchema> {
+ let arg_types = signature.type_signature.get_representative_types();
+ if arg_types.is_empty() {
+ // Edge case if function doesn't have arguments
+ return Ok(BTreeSet::from([(vec![], None)]));
+ }
+ arg_types
+ .into_iter()
+ .map(|arg_types| {
+ // Get possible types for each input arg
+ let arg_fields = arg_types
+ .iter()
+ .enumerate()
+ .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
Review 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.
```sql
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`.
```diff
use datafusion_expr::function::WindowUDFFieldArgs;
+use datafusion_expr::type_coercion::functions::fields_with_udf;
```
```diff
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()];
```
```diff
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())?)
})
```
```diff
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()))
```
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: Use NativeType in get_example_type, information schema [datafusion]
Posted by "Jefffrey (via GitHub)" <gi...@apache.org>.
Jefffrey commented on PR #21737:
URL: https://github.com/apache/datafusion/pull/21737#issuecomment-5812186157
maybe @jayzhan211 can help take a look at this
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: Use NativeType in get_example_type, information schema [datafusion]
Posted by "theirix (via GitHub)" <gi...@apache.org>.
theirix commented on code in PR #21737:
URL: https://github.com/apache/datafusion/pull/21737#discussion_r4117068549
##########
datafusion/catalog/src/information_schema.rs:
##########
@@ -454,26 +454,35 @@ impl InformationSchemaConfig {
}
}
+/// Build the argument field for `information_schema` to provide a return type
+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)?;
+ Ok(Arc::new(Field::new(format!("arg_{idx}"), data_type, true)))
+}
+
/// get the arguments and return types of a UDF
/// returns a tuple of (arg_types, return_type)
fn get_udf_args_and_return_types(
udf: &Arc<ScalarUDF>,
) -> Result<BTreeSet<(Vec<String>, Option<String>)>> {
let signature = udf.signature();
- let arg_types = signature.type_signature.get_example_types();
+ let arg_types = signature.type_signature.get_representative_types();
if arg_types.is_empty() {
Ok(vec![(vec![], None)].into_iter().collect::<BTreeSet<_>>())
} else {
- Ok(arg_types
+ arg_types
.into_iter()
.map(|arg_types| {
- let arg_fields: Vec<FieldRef> = arg_types
+ let mut arg_fields = arg_types
.iter()
.enumerate()
- .map(|(i, t)| {
- Arc::new(Field::new(format!("arg_{i}"), t.clone(), true))
- })
- .collect();
+ .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();
Review Comment:
Thank you, it's a good spot. Removed the dedup to have an expected result (backed by a unit test)
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: Use NativeType in get_example_type, information schema [datafusion]
Posted by "theirix (via GitHub)" <gi...@apache.org>.
theirix commented on code in PR #21737:
URL: https://github.com/apache/datafusion/pull/21737#discussion_r4187385317
##########
datafusion/catalog/src/information_schema.rs:
##########
@@ -454,118 +455,98 @@ impl InformationSchemaConfig {
}
}
-/// get the arguments and return types of a UDF
-/// returns a tuple of (arg_types, return_type)
+/// Origins used to enumerate the physical types a native type can take
+const RESOLVE_CAST_SOURCES: [DataType; 2] = [DataType::Null, DataType::LargeUtf8];
+
+/// Build argument fields for `information_schema` to provide possible return types
+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
Review Comment:
It's reasonable, since `default_cast_for` cannot handle it on its own - added
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: Use NativeType in get_example_type, information schema [datafusion]
Posted by "theirix (via GitHub)" <gi...@apache.org>.
theirix commented on code in PR #21737:
URL: https://github.com/apache/datafusion/pull/21737#discussion_r4183562727
##########
parquet-testing:
##########
Review Comment:
Accidentally added
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org