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

refer to #8781, convert the internal_err! in datetime_expression.rs to exec_err! #9083

Merged
merged 1 commit into from
Jan 31, 2024

Conversation

Tangruilin
Copy link
Contributor

Which issue does this PR close?

Closes #.

Rationale for this change

releateto #8781

What changes are included in this PR?

change the internal_err maroc in datetime_expression.ts to exec_err!

Are these changes tested?

no

Are there any user-facing changes?

no

@github-actions github-actions bot added the physical-expr Physical Expressions label Jan 31, 2024
@Tangruilin
Copy link
Contributor Author

@Omega359
I submit a PR to change the internal_err to exec_err

you can review this patch

@Tangruilin
Copy link
Contributor Author

internal_err: something that wasn't expected/anticipated by the implementation and that is most likely a bug (the error message even encourages users to open a bug report). I user should not be able to trigger this under normal circumstances. Note that I/O errors (or any error that happens due to external systems) do NOT fall under this category (there's an external error variant for that)

exec_err: a processing error that happens during execution, e.g. the user passed malformed arguments to a SQL method, opened a CSV file that is broken, tried to divide an integer by zero. these errors shouldn't happen for a "good" query + "good" data, but since both the query and the data can easily be broken or misaligned, these are common occurrences in ETL systems / databases.

@Omega359
Copy link
Contributor

I am uncertain as to whether the unsupported data type errors should always be internal or exec. I think it may depend on the signature of the function. For example, from_unixtime is specified as having a signature of

BuiltinScalarFunction::FromUnixtime => {
                Signature::uniform(1, vec![Int64], self.volatility())
            }

so if that function got anything other than Int64 it would definitely be unexpected and thus I would think that internal_err might be appropriate there. However, to_timestamp(_xxx) methods are defined as

BuiltinScalarFunction::ToTimestamp
            | BuiltinScalarFunction::ToTimestampSeconds
            | BuiltinScalarFunction::ToTimestampMillis
            | BuiltinScalarFunction::ToTimestampMicros
            | BuiltinScalarFunction::ToTimestampNanos => {
                Signature::variadic_any(self.volatility())
            }

So it would be expected that the impl would have to handle and return unacceptable types (thus exec_err)

@Tangruilin
Copy link
Contributor Author

I am uncertain as to whether the unsupported data type errors should always be internal or exec. I think it may depend on the signature of the function. For example, from_unixtime is specified as having a signature of

BuiltinScalarFunction::FromUnixtime => {
                Signature::uniform(1, vec![Int64], self.volatility())
            }

so if that function got anything other than Int64 it would definitely be unexpected and thus I would think that internal_err might be appropriate there. However, to_timestamp(_xxx) methods are defined as

BuiltinScalarFunction::ToTimestamp
            | BuiltinScalarFunction::ToTimestampSeconds
            | BuiltinScalarFunction::ToTimestampMillis
            | BuiltinScalarFunction::ToTimestampMicros
            | BuiltinScalarFunction::ToTimestampNanos => {
                Signature::variadic_any(self.volatility())
            }

So it would be expected that the impl would have to handle and return unacceptable types (thus exec_err)

May I need to tidy up them and modify this PR?

Copy link
Contributor

@comphead comphead left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm thanks @Tangruilin
Going fwd i'm thinking of giving available signatures for specific built in functions, instead of just complaining about params

@alamb alamb merged commit f22b33c into apache:main Jan 31, 2024
22 checks passed
@alamb
Copy link
Contributor

alamb commented Jan 31, 2024

Thank you @comphead and @Tangruilin

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
physical-expr Physical Expressions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants