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.

Sign up for GitHub

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

Jump to bottom

change result type of count/count_distinct from uint64 to int64 #2636

Merged
merged 1 commit into from
May 30, 2022
Merged

change result type of count/count_distinct from uint64 to int64 #2636

merged 1 commit into from
May 30, 2022

Conversation

liukun4515
Copy link
Contributor

@liukun4515 liukun4515 commented May 28, 2022 •
edited
Loading

Which issue does this PR close?

Closes #2635
From the #2635, the result type of count is uint64 and this is not compatible with PG and other db system.

This pr just convert the result type from uint64 to int64

Rationale for this change

What changes are included in this PR?

Are there any user-facing changes?

Does this PR break compatibility with Ballista?

@github-actions github-actions bot added core Core DataFusion crate datafusion Changes in the datafusion crate logical-expr Logical plan and expressions optimizer Optimizer rules physical-expr Physical Expressions labels May 28, 2022
@liukun4515
Copy link
Contributor Author

@alamb @andygrove PTAL

andygrove
andygrove approved these changes May 29, 2022
View reviewed changes
Copy link
Member

@andygrove andygrove 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 @liukun4515

alamb
alamb approved these changes May 30, 2022
View reviewed changes
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.

Looks like COUNT has been u64 a long time in DataFusion ( I remembered vaguely that this got changed recently, but I can't find evidence of that)

FWIW I think postgres returns i64 because it has no distinction between u64 and i64; https://www.postgresql.org/docs/current/datatype-numeric.html#DATATYPE-INT

@alamb alamb merged commit 0ff59de into apache:master May 30, 2022
@alamb
Copy link
Contributor

alamb commented May 30, 2022

Thanks @liukun4515 and @andygrove !

@liukun4515 liukun4515 deleted the count_data_type branch June 1, 2022 08:05
@liukun4515
Copy link
Contributor Author

Looks like COUNT has been u64 a long time in DataFusion ( I remembered vaguely that this got changed recently, but I can't find evidence of that)

FWIW I think postgres returns i64 because it has no distinction between u64 and i64; https://www.postgresql.org/docs/current/datatype-numeric.html#DATATYPE-INT

Some database systems like pg or mysql don't support the unsigned integer and just support the signed integer

This was referenced Jun 1, 2022
Closed
Merged
@alamb alamb mentioned this pull request Jun 28, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@andygrove andygrove andygrove approved these changes

@alamb alamb alamb approved these changes

Assignees
No one assigned
Labels
core Core DataFusion crate datafusion Changes in the datafusion crate logical-expr Logical plan and expressions optimizer Optimizer rules physical-expr Physical Expressions
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

The result type of count/count_distinct
3 participants
@liukun4515 @alamb @andygrove