Remove Optional from FilterStatsCalculator and ComparisonStatsCalculator method signatures#11686
Merged
arhimondr merged 4 commits intoprestodb:masterfrom Oct 11, 2018
Conversation
- Reorder methods - Reorder arguments - Give more meaningful name to variables - Give more readable names to methogs
In fact Optional.empty() meant the same as PlanNodeStatsEstimate.unknown(). Although it wasn't clear if it is possible for ComparisonStatsCalculator to return Optional.of(PlanNodeStatsEstimate.unknown()), and whether the code that calls ComparisonStatsCalculator should check for unknown within the Optional. This commit tries to address this ambiguity.
- Extract SymbolStatsEstimate#isSingleValue method - Give variables more readable names - Reorder methods - Return Optional.empty() explicitly instead of delegating to visitExpression() - Simplify getType and getExpressionStats methods. Functional style is extra there
Member
Author
|
I ran the tests from the #11665, the plans are the same. |
kokosing
reviewed
Oct 11, 2018
Contributor
kokosing
left a comment
kokosing
left a comment
There was a problem hiding this comment.
I am ok with this refactor.
sopel39
approved these changes
Oct 11, 2018
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Having Optional in these interfaces makes code messy.
Optional.empty()indicates an absence of the estimate. However it doesn't mean that if the result is not empty, the estimate is not unknown. That makes code harder to reason about.