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

[Tech Request]: time and datetime type can not fold correctly (#14698) #15439

Merged
merged 2 commits into from
Apr 10, 2024

Conversation

badboynt1
Copy link
Contributor

constand fold time and datetime type, and calculate stats correctly

Approved by: @aunjgr

What type of PR is this?

  • API-change
  • BUG
  • Improvement
  • Documentation
  • Feature
  • Test and CI
  • Code Refactoring

Which issue(s) this PR fixes:

issue #https://github.com/matrixorigin/MO-Cloud/issues/2959

What this PR does / why we need it:

constand fold time and datetime type, and calculate stats correctly

…origin#14698)

constand fold time and datetime type, and calculate stats correctly

Approved by: @aunjgr
@matrix-meow matrix-meow added the size/S Denotes a PR that changes [10,99] lines label Apr 10, 2024
@mergify mergify bot requested a review from sukki37 April 10, 2024 08:25
@mergify mergify bot added the kind/bug Something isn't working label Apr 10, 2024
@matrix-meow
Copy link
Contributor

@badboynt1 Thanks for your contributions!

Pull Request Review:

Title:

The title of the pull request is clear and informative, indicating that there is an issue with the folding of time and datetime types.

Body:

The body of the pull request provides a brief description of the changes made in the PR, mentioning constant folding of time and datetime types and ensuring correct calculation of stats. It also references the related issue and the approval status.

Changes:

  1. constant_fold.go:

    • In the GetConstantValue function, the conditions for types.T_time and types.T_datetime have been removed. This change might lead to unexpected behavior if the transAll condition was originally intended to prevent certain operations.
    • It seems like the removal of the conditions might have been unintentional. It's important to verify if these conditions were necessary for the correct functioning of the code.
    • If the conditions were removed intentionally, it would be beneficial to add comments explaining the reasoning behind this change for better code readability.
  2. stats.go:

    • Additional cases for types.T_time and types.T_datetime have been added to update the MinValMap and MaxValMap. This change seems appropriate for handling time and datetime types correctly.
    • However, it's crucial to ensure that the decoding and handling of time and datetime values are accurate to prevent any data inconsistencies or errors.
  3. stats.go:

    • In the calcSelectivityByMinMax function, a default case has been added to set ret to 0.3 if the funcName does not match any existing cases. This default value might not be appropriate and could lead to unexpected behavior.
    • It would be better to handle unknown cases more explicitly, such as logging a warning or throwing an error, rather than assigning a default value that may not be suitable for all scenarios.
    • The additional checks to ensure ret is within the range of 0 to 1 are good practices to prevent invalid selectivity values. However, the comment return -1 // never reach here is misleading and should be removed to avoid confusion.

Suggestions for Improvement:

  1. Revisit Condition Removal:

    • Review the removal of conditions in constant_fold.go to ensure it aligns with the intended logic. If the conditions were necessary, consider reintroducing them or adding comments to explain the rationale behind their removal.
  2. Verify Time and Datetime Handling:

    • Double-check the decoding and handling of time and datetime values in stats.go to guarantee accuracy and consistency in the calculations.
  3. Handle Unknown Cases:

    • Instead of assigning a default value in calcSelectivityByMinMax, consider handling unknown cases explicitly to maintain code clarity and prevent unintended consequences.
  4. Remove Misleading Comment:

    • Remove the misleading comment return -1 // never reach here in calcSelectivityByMinMax to avoid confusion and ensure code readability.

By addressing these points, the pull request can be improved in terms of correctness, maintainability, and clarity. It's essential to thoroughly test the changes to ensure they function as expected and do not introduce any regressions or security vulnerabilities.

@mergify mergify bot merged commit d301800 into matrixorigin:1.1-dev Apr 10, 2024
18 checks passed
@badboynt1 badboynt1 deleted the datetime1.1 branch April 11, 2024 02:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Something isn't working size/S Denotes a PR that changes [10,99] lines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants