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

support decimal data type in create table #1431

Merged
merged 2 commits into from
Dec 11, 2021

Conversation

liukun4515
Copy link
Contributor

Which issue does this PR close?

related #122

Rationale for this change

What changes are included in this PR?

Are there any user-facing changes?

@github-actions github-actions bot added datafusion Changes in the datafusion crate sql labels Dec 10, 2021
@houqp houqp added the enhancement New feature or request label Dec 10, 2021
// TODO add bound checker in some utils file or function
if *p > 38 || *s > *p {
return Err(DataFusionError::Internal(format!(
"Error Decimal Type ({:?})",
Copy link
Contributor

Choose a reason for hiding this comment

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

A slightly more informative error would be better, such as "precision above 38 / scale bigger than precision" not supported.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for your suggestion.
It has been modified.

// TODO add bound checker in some utils file or function
if *p > 38 || *s > *p {
return Err(DataFusionError::Internal(format!(
"Error Decimal Type ({:?})",
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here

Copy link
Contributor

Choose a reason for hiding this comment

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

async fn register_simple_aggregate_csv_with_decimal_by_sql(ctx: &mut ExecutionContext) {
let df = ctx
.sql(
" CREATE EXTERNAL TABLE aggregate_simple (
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
" CREATE EXTERNAL TABLE aggregate_simple (
"CREATE EXTERNAL TABLE aggregate_simple (

@liukun4515
Copy link
Contributor Author

PTAL @Dandandan again.

@liukun4515
Copy link
Contributor Author

@alamb please merge this.
Thanks.

@houqp houqp merged commit dc80c11 into apache:master Dec 11, 2021
@houqp
Copy link
Member

houqp commented Dec 11, 2021

Thanks @liukun4515

@alamb
Copy link
Contributor

alamb commented Dec 12, 2021

🎉 Thank you very much @liukun4515 , @Dandandan and @houqp

)));
}
(Some(p), Some(s)) => {
// TODO add bound checker in some utils file or function
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

#[tokio::test]
async fn csv_query_with_decimal_by_sql() -> Result<()> {
let mut ctx = ExecutionContext::new();
register_simple_aggregate_csv_with_decimal_by_sql(&mut ctx).await;
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
datafusion Changes in the datafusion crate enhancement New feature or request sql
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants