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

Exactness Indicator of Parameters: Precision #7809

Merged
merged 4 commits into from
Oct 12, 2023
Merged

Exactness Indicator of Parameters: Precision #7809

merged 4 commits into from
Oct 12, 2023

Conversation

berkaysynnada
Copy link
Contributor

@berkaysynnada berkaysynnada commented Oct 12, 2023

Which issue does this PR close?

Closes #.

Rationale for this change

Some parameters need to be carried along with their exactness information. That exactness information can be provided by the source, or it can be set during the execution. This implementation which will initially be useful in statistics can also be used in different areas in the future.

If this approach makes sense and is approved by the community, I will subsequently open a new PR where its importance on statistics will be seen and some optimizations will be possible.

What changes are included in this PR?

An enumeration, named Precision, is implemented to define 3 types of exactness variants: Exact, Inexact, and Absent. The enum can be used in a generic way, and some utilities on usize and ScalarValue have also been added.

Are these changes tested?

Yes, unit tests are added.

Are there any user-facing changes?

@alamb
Copy link
Contributor

alamb commented Oct 12, 2023

This is part of #7793, correct? If so, thank you for extracting it to its own PR

@berkaysynnada
Copy link
Contributor Author

This is part of #7793, correct? If so, thank you for extracting it to its own PR

Yes, it is. #7804 is also another extracted part.

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.

LGTM -- thank you for extracting this out @berkaysynnada

datafusion/common/src/stats.rs Outdated Show resolved Hide resolved
datafusion/common/src/stats.rs Outdated Show resolved Hide resolved
datafusion/common/src/stats.rs Outdated Show resolved Hide resolved
@berkaysynnada berkaysynnada changed the title Exactness Indicator of Parameters: Sharpness Exactness Indicator of Parameters: Precision Oct 12, 2023
@berkaysynnada
Copy link
Contributor Author

LGTM -- thank you for extracting this out @berkaysynnada

Thank you for the reviews. After these 2 PRs are merged, I will resolve the conflicts of #7793 and request a review again.

@alamb
Copy link
Contributor

alamb commented Oct 12, 2023

Thanks again @berkaysynnada

@alamb alamb merged commit cf7a9a0 into apache:main Oct 12, 2023
@andygrove andygrove added the enhancement New feature or request label Nov 5, 2023
@berkaysynnada berkaysynnada deleted the feature/join-statistics branch November 10, 2023 11:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants