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

Modify S3IOBytes statistic property #18795

Merged
merged 11 commits into from
Sep 18, 2024

Conversation

qingxinhome
Copy link
Contributor

@qingxinhome qingxinhome commented Sep 14, 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 #14107

What this PR does / why we need it:

Modify S3IOBytes statistic property


PR Type

Bug fix, Enhancement


Description

  • Renamed S3IOByte to ScanBytes across multiple files to improve clarity and consistency.
  • Updated AnalyzeInfo structure and related methods to reflect the new naming convention.
  • Modified operatorAnalyzer and OperatorStats to use ScanBytes instead of S3IOByte.
  • Adjusted Protocol Buffers definitions to align with the new naming.

Changes walkthrough 📝

Relevant files
Enhancement
table_scan.go
Rename S3IOByte to ScanBytes in TableScan                               

pkg/sql/colexec/table_scan/table_scan.go

  • Renamed S3IOByte to ScanBytes in the analyzer method call.
+1/-1     
analyze_module.go
Update AnalyzeInfo to use ScanBytes                                           

pkg/sql/compile/analyze_module.go

  • Renamed S3IOByte to ScanBytes in AnalyzeInfo.
+1/-1     
deepcopy.go
Modify DeepCopyAnalyzeInfo for ScanBytes                                 

pkg/sql/plan/deepcopy.go

  • Changed S3IOByte to ScanBytes in DeepCopyAnalyzeInfo.
+3/-2     
marshal_query.go
Replace S3IOByte with ScanBytes in marshal_query                 

pkg/sql/plan/explain/marshal_query.go

  • Updated constant and usage from S3IOByte to ScanBytes.
+3/-3     
operator_analyzer.go
Refactor operatorAnalyzer to use ScanBytes                             

pkg/vm/process/operator_analyzer.go

  • Replaced S3IOByte with ScanBytes in operatorAnalyzer.
  • Updated OperatorStats to use TotalScanBytes.
  • +6/-6     
    plan.proto
    Update AnalyzeInfo message to use scan_bytes                         

    proto/plan.proto

    • Renamed s3IO_byte to scan_bytes in AnalyzeInfo message.
    +1/-1     

    💡 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: 2 🔵🔵⚪⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Key issues to review

    Inconsistent Error Message
    The error message in the ScanBytes function still refers to 'S3IOByte' instead of 'ScanBytes'.

    @matrix-meow matrix-meow added the size/S Denotes a PR that changes [10,99] lines label Sep 14, 2024
    @mergify mergify bot added the kind/bug Something isn't working label Sep 14, 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
    Possible issue
    Add a check for negative values when updating the total scan bytes

    Consider adding a check for negative values when updating TotalScanBytes to ensure
    data integrity and prevent potential underflow issues.

    pkg/vm/process/operator_analyzer.go [166-168]

     if bat != nil {
    -    opAlyzr.opStats.TotalScanBytes += int64(bat.Size())
    +    size := int64(bat.Size())
    +    if size >= 0 {
    +        opAlyzr.opStats.TotalScanBytes += size
    +    }
     }
     
    • Apply this suggestion
    Suggestion importance[1-10]: 9

    Why: Adding a check for negative values is a crucial improvement that enhances data integrity and prevents potential underflow issues, which could lead to incorrect statistics.

    9
    Maintainability
    Update panic message to reflect the new method name for consistency

    The panic message in the ScanBytes method still refers to S3IOByte. Update it to
    reflect the new method name for consistency.

    pkg/vm/process/operator_analyzer.go [161-163]

     if opAlyzr.opStats == nil {
    -    panic("operatorAnalyzer.S3IOByte: operatorAnalyzer.opStats is nil")
    +    panic("operatorAnalyzer.ScanBytes: operatorAnalyzer.opStats is nil")
     }
     
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: Updating the panic message to reflect the new method name ensures consistency and clarity in error messages, which is important for debugging and maintenance.

    8
    Enhancement
    Use a more descriptive name for the method call to improve code readability

    Consider using a more descriptive name for the analyzer.ScanBytes() method call. For
    example, analyzer.RecordScanBytes() or analyzer.UpdateScanBytes() would better
    convey the action being performed.

    pkg/sql/colexec/table_scan/table_scan.go [141]

    -analyzer.ScanBytes(tableScan.ctr.buf)
    +analyzer.RecordScanBytes(tableScan.ctr.buf)
     
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: The suggestion to use a more descriptive method name improves code readability and maintainability, making it clearer what the method does.

    7

    @mergify mergify bot merged commit 0b8e39d into matrixorigin:main Sep 18, 2024
    18 checks passed
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    Bug fix Enhancement kind/bug Something isn't working Review effort [1-5]: 2 size/S Denotes a PR that changes [10,99] lines
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    9 participants