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

Fix extract parquet statistics from Decimal256 columns #10777

Merged
merged 5 commits into from
Jun 4, 2024
Merged

Fix extract parquet statistics from Decimal256 columns #10777

merged 5 commits into from
Jun 4, 2024

Conversation

xinlifoobar
Copy link
Contributor

Which issue does this PR close?

Closes #10755 .

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

@github-actions github-actions bot added the core Core DataFusion crate label Jun 3, 2024
alamb
alamb approved these changes Jun 3, 2024
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.

Thank you @xinlifoobar -- again a really nice PR 🙏

datafusion/core/src/datasource/physical_plan/parquet/statistics.rs
None,
Some(i256::from(22000)),
// row group 2
Some(i256::MAX),
Copy link
Contributor

Choose a reason for hiding this comment

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

👍 nice for adding limits

@alamb
Copy link
Contributor

alamb commented Jun 4, 2024

I am slowly merging these PRs after resolving conflicts. I hope to be done by tomorrow

@xinlifoobar
Copy link
Contributor Author

I am slowly merging these PRs after resolving conflicts. I hope to be done by tomorrow

Hey @alamb, because we are not in the same time zone, I couldn't resolve such issues on time. I will periodically check my PRs to resolve conflicts in my daytime and just feel free to leave a comment when you found one. Thanks in advance :)

@xinlifoobar xinlifoobar closed this Jun 4, 2024
@xinlifoobar xinlifoobar reopened this Jun 4, 2024
@alamb
Copy link
Contributor

alamb commented Jun 4, 2024

I am slowly merging these PRs after resolving conflicts. I hope to be done by tomorrow

Hey @alamb, because we are not in the same time zone, I couldn't resolve such issues on time. I will periodically check my PRs to resolve conflicts in my daytime and just feel free to leave a comment when you found one. Thanks in advance :)

No worries at all -- thank @xinlifoobar -- there were about 5 PRs adding support for the different types that conflicted so Iw as just working through each PR one by one and resolving the conflicts myself. Otherwise as you pointed out, timezone differences mean it would likely take 5 days to get them all in

I'll handle getting this one in (it is next on the list)

Weijun-H
Weijun-H approved these changes Jun 4, 2024
View reviewed changes
Copy link
Member

@Weijun-H Weijun-H left a comment

Choose a reason for hiding this comment

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

Nice work! @xinlifoobar 👍

@alamb alamb merged commit 76a3720 into apache:main Jun 4, 2024
23 checks passed
@alamb
Copy link
Contributor

alamb commented Jun 4, 2024

Thanks @Weijun-H and @xinlifoobar

findepi pushed a commit to findepi/datafusion that referenced this pull request Jul 16, 2024
@xinlifoobar @alamb @findepi
Fix extract parquet statistics from Decimal256 columns (apache#10777)
9f1880d 
* Fix Extract parquet statistics from Decimal256 columns

* Fix comment

---------

Co-authored-by: Andrew Lamb <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alamb alamb alamb approved these changes

@Weijun-H Weijun-H Weijun-H approved these changes

Assignees
No one assigned
Labels
core Core DataFusion crate
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Extract parquet statistics from Decimal256 columns
3 participants
@xinlifoobar @alamb @Weijun-H