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

add filter expr log and optimize expr print #17356

Merged
merged 6 commits into from
Jul 8, 2024

Conversation

XuPeng-SH
Copy link
Contributor

@XuPeng-SH XuPeng-SH commented Jul 6, 2024

User description

What type of PR is this?

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

Which issue(s) this PR fixes:

issue #14518

What this PR does / why we need it:

optimize expr print and reduce filter expr prints


PR Type

Enhancement


Description

  • Added handling for vector expressions in the doFormatExpr function in pkg/sql/plan/utils.go.
  • Introduced a new atomic variable traceFilterExprInterval2 in pkg/vm/engine/disttae/txn_table.go.
  • Modified the Ranges function to optimize filter expression logging based on new conditions.

Changes walkthrough 📝

Relevant files
Enhancement
utils.go
Add handling for vector expressions in `doFormatExpr`       

pkg/sql/plan/utils.go

  • Added a new case for Expr_Vec in the doFormatExpr function to handle
    vector expressions.
  • +2/-0     
    txn_table.go
    Optimize filter expression logging in `Ranges` function   

    pkg/vm/engine/disttae/txn_table.go

  • Introduced a new atomic variable traceFilterExprInterval2.
  • Modified the Ranges function to include logic for
    traceFilterExprInterval2.
  • Adjusted the logic for enabling logging of filter expressions based on
    new conditions.
  • +12/-3   

    💡 PR-Agent usage:
    Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

    Copy link

    PR-Agent was enabled for this repository. To continue using it, please link your git user with your CodiumAI identity here.

    PR Reviewer Guide 🔍

    ⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Key issues to review

    Possible Bug:
    The logic for setting tbl.enableLogFilterExpr.Store(true) in txn_table.go might be redundant or conflicting. The conditions for enabling logging based on traceFilterExprInterval and traceFilterExprInterval2 could potentially overlap, causing unexpected behavior. It's recommended to clarify the intended logic and ensure that the conditions are mutually exclusive or properly coordinated.

    Performance Concern:
    The introduction of additional logging conditions and the use of atomic operations in txn_table.go could impact performance, especially in high-throughput scenarios. It would be beneficial to measure the performance impact of these changes, particularly how often logging is enabled and the overhead introduced by atomic operations.

    @matrix-meow matrix-meow added the size/S Denotes a PR that changes [10,99] lines label Jul 6, 2024
    Copy link

    PR-Agent was enabled for this repository. To continue using it, please link your git user with your CodiumAI identity here.

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Performance
    Combine conditions to avoid redundant calls to tbl.enableLogFilterExpr.Store(true)

    To avoid redundant calls to tbl.enableLogFilterExpr.Store(true), consider combining the
    conditions that set this value.

    pkg/vm/engine/disttae/txn_table.go [606-617]

    -if traceFilterExprInterval.Add(step) >= 500000 {
    +if traceFilterExprInterval.Add(step) >= 500000 || traceFilterExprInterval2.Add(slowStep) >= 20 || rangesLen >= 50 {
         traceFilterExprInterval.Store(0)
    -    tbl.enableLogFilterExpr.Store(true)
    -}
    -if traceFilterExprInterval2.Add(slowStep) >= 20 {
         traceFilterExprInterval2.Store(0)
         tbl.enableLogFilterExpr.Store(true)
     }
    -if rangesLen >= 50 {
    -    tbl.enableLogFilterExpr.Store(true)
    -}
     
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: This suggestion improves performance by reducing redundant operations and simplifying the logic. It's a significant improvement in terms of code efficiency and clarity.

    8
    Maintainability
    Extract repeated fmt.Sprintf calls into a helper function for better readability and maintainability

    To improve readability and maintainability, consider extracting the repeated fmt.Sprintf
    calls into a helper function. This will make the code cleaner and easier to manage.

    pkg/sql/plan/utils.go [1804-1810]

    -out.WriteString(fmt.Sprintf("%sExpr_P(%d)", prefix, t.P.Pos))
    -out.WriteString(fmt.Sprintf("%sExpr_T(%s)", prefix, t.T.String()))
    -out.WriteString(fmt.Sprintf("%sExpr_Vec(len=%d)", prefix, t.Vec.Len))
    -out.WriteString(fmt.Sprintf("%sExpr_Unknown(%s)", prefix, expr.String()))
    +writeFormattedString(out, prefix, "Expr_P", t.P.Pos)
    +writeFormattedString(out, prefix, "Expr_T", t.T.String())
    +writeFormattedString(out, prefix, "Expr_Vec(len=%d)", t.Vec.Len)
    +writeFormattedString(out, prefix, "Expr_Unknown", expr.String())
     
    +func writeFormattedString(out *bytes.Buffer, prefix, format string, args ...interface{}) {
    +    out.WriteString(fmt.Sprintf("%s%s", prefix, fmt.Sprintf(format, args...)))
    +}
    +
    Suggestion importance[1-10]: 7

    Why: The suggestion correctly identifies repetitive code and proposes a helper function to improve maintainability and readability. This is a good practice in coding but not critical for functionality.

    7
    Best practice
    Rename traceFilterExprInterval2 to a more descriptive name to improve code clarity

    To avoid potential confusion and improve clarity, consider renaming
    traceFilterExprInterval2 to a more descriptive name that indicates its purpose or
    difference from traceFilterExprInterval.

    pkg/vm/engine/disttae/txn_table.go [67]

    -var traceFilterExprInterval2 atomic.Uint64
    +var traceFilterExprSlowInterval atomic.Uint64
     
    • Apply this suggestion
    Suggestion importance[1-10]: 6

    Why: The suggestion is valid as it addresses the clarity and readability of the code by proposing a more descriptive variable name. This is helpful for maintainability but not a major issue.

    6
    Possible issue
    Initialize slowStep to 0 by default to ensure it is always set

    To ensure that slowStep is always initialized, consider setting it to 0 by default before
    the conditional blocks.

    pkg/vm/engine/disttae/txn_table.go [591-593]

     var (
    -    step, slowStep uint64
    +    step, slowStep uint64 = 0, 0
     )
     
    • Apply this suggestion
    Suggestion importance[1-10]: 5

    Why: The suggestion is correct in ensuring that variables are initialized to prevent potential bugs. However, in Go, uint64 variables are zero-initialized by default, making this suggestion redundant but not incorrect.

    5

    @mergify mergify bot merged commit a38d908 into matrixorigin:main Jul 8, 2024
    16 of 18 checks passed
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    None yet

    4 participants