Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DataType::is_numeric should match the is_numeric function in Datafusion. #2611

Closed
HaoYang670 opened this issue Aug 30, 2022 · 4 comments · Fixed by #3121
Closed

DataType::is_numeric should match the is_numeric function in Datafusion. #2611

HaoYang670 opened this issue Aug 30, 2022 · 4 comments · Fixed by #3121
Labels
arrow Changes to the arrow crate enhancement Any new improvement worthy of a entry in the changelog

Comments

@HaoYang670
Copy link
Contributor

Is your feature request related to a problem or challenge? Please describe what you are trying to do.
In datafusion, we have:

/// Determine if a DataType is signed numeric or not
pub fn is_signed_numeric(dt: &DataType) -> bool {
    matches!(
        dt,
        DataType::Int8
            | DataType::Int16
            | DataType::Int32
            | DataType::Int64
            | DataType::Float16
            | DataType::Float32
            | DataType::Float64
            | DataType::Decimal128(_, _)
    )
}

/// Determine if a DataType is numeric or not
pub fn is_numeric(dt: &DataType) -> bool {
    is_signed_numeric(dt)
        || matches!(
            dt,
            DataType::UInt8 | DataType::UInt16 | DataType::UInt32 | DataType::UInt64
        )
}

We should update DataType::is_numeric to contain these constructors so that we could remove the redundant is_numeric in datafusion (apache/datafusion#1613)

Describe the solution you'd like

Describe alternatives you've considered

Additional context

@HaoYang670 HaoYang670 added the enhancement Any new improvement worthy of a entry in the changelog label Aug 30, 2022
@HaoYang670 HaoYang670 changed the title DataType::is_numeric should contains the is_numeric function in Datafusion. DataType::is_numeric should match the is_numeric function in Datafusion. Aug 31, 2022
@HaoYang670
Copy link
Contributor Author

HaoYang670 commented Aug 31, 2022

Hi @alamb, any background of why we don't support Float16 in the cast kernel and the ipc writer?

@liukun4515
Copy link
Contributor

liukun4515 commented Aug 31, 2022

Because lack of some kernel function for some data type which may be not used by the upstream project or other user, the checker of data type maybe duplicated and redundant.

@HaoYang670
Copy link
Contributor Author

Thank you @liukun4515.
I guess we cannot support Float16 as a numeric type so far because lexical_core::FromLexical doesn't implement for half::f16.

Then, the next question is that should we update the is_numeric function in datafusion to match the arrow-rs. (aka removing float16 and decimal128)? Or should we rename it if it provides different functionality from DataType::is_numeric?

@alamb
Copy link
Contributor

alamb commented Aug 31, 2022

I think f16 support was relatively recent -- added by @Jimexist in #888 . I don't think there was any specific reason that f16 doesn't have as wide support as the other types.

I would be in favor of having arrow-rs and datafusion match. I think decimal128 is a critical usecase for datafusion - as at least @liukun4515 and his team are using it heavily.

I don't know of anyone using f16 at the moment (but maybe there are) so I don't think it is as important

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
arrow Changes to the arrow crate enhancement Any new improvement worthy of a entry in the changelog
Projects
None yet
3 participants