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

#699 fix return type conflict when calling builtin math fuctions #716

Merged
merged 1 commit into from
Jul 16, 2021

Conversation

lvheyang
Copy link
Contributor

@lvheyang lvheyang commented Jul 12, 2021

Which issue does this PR close?

Closes #699

Rationale for this change

When we infer schema we should also consider the input arguments' datatype. such as sqrt(f32) -> f32, sqrt(64) -> f64. But now builtin functions statically return Float64. It causes column types and schema types conflict.

What changes are included in this PR?

Change the return type in datafusion/src/physical_plan/functions.rs. This PR made builtin math functions return the same type as the input argument

Are there any user-facing changes?

@github-actions github-actions bot added the datafusion Changes in the datafusion crate label Jul 12, 2021
@lvheyang lvheyang marked this pull request as draft July 12, 2021 07:34
Copy link
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

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

Thank you @lvheyang -- I think this is on the right track. I will comment more on #699 as well

@lvheyang lvheyang force-pushed the my0712 branch 4 times, most recently from 2bb386f to 23a254d Compare July 13, 2021 13:43
Copy link
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

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

This is looking good to me -- thank you @lvheyang !

I think all this PR needs to be mergeable are some tests (either in context.rs or sql.rs) of running math queries against columns that have float32, int etc.

Let me know if you would like some help or more specific pointers

@lvheyang lvheyang marked this pull request as ready for review July 14, 2021 02:33
@lvheyang lvheyang force-pushed the my0712 branch 2 times, most recently from 5b3d6df to 0000617 Compare July 14, 2021 05:10
@lvheyang
Copy link
Contributor Author

lvheyang commented Jul 14, 2021

Thanks for your kind help! @alamb I added test cases in context.rs, test SQLs:

  1. SELECT sqrt(f64) FROM t
  2. SELECT sqrt(f32) FROM t // would panic before this pull request
  3. SELECT sqrt(i64) FROM t
  4. SELECT sqrt(i32) FROM t

Is there any case else needed?

@houqp
Copy link
Member

houqp commented Jul 14, 2021

@lvheyang
Copy link
Contributor Author

@houqp 👌, I will add them all in the test case.

@lvheyang
Copy link
Contributor Author

@alamb @houqp test case added. The case covered int8/int16/int32/int64/uint8/uint16/uint32/uint64/float32/float64. Please review. Thanks a lot!

Copy link
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

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

This looks like an improvement to me. Thank you @lvheyang !

"| 1 |",
"+---------+",
];
let results = plan_and_collect(&mut ctx, "SELECT sqrt(v) FROM t")
Copy link
Contributor

Choose a reason for hiding this comment

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

👍 it is a good test. We can finagle the types later if needed (e.g. if we ever want to support sqrt(numeric) --> numeric.

I think this PR makes DataFusion better than it is on master.

@alamb
Copy link
Contributor

alamb commented Jul 14, 2021

Any thoughts @Dandandan or @jorgecarleitao ?

@alamb
Copy link
Contributor

alamb commented Jul 16, 2021

Thank you again for the contribution @lvheyang .

@alamb alamb merged commit a1c794c into apache:master Jul 16, 2021
@houqp houqp added the bug Something isn't working label Jul 31, 2021
andygrove pushed a commit to andygrove/datafusion that referenced this pull request Jan 31, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working datafusion Changes in the datafusion crate
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Datafusion: math function does not support array type f32
3 participants