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

bug fix: unique key/secondary key get incorrect value (#18711) #18717

Merged
merged 1 commit into from
Sep 12, 2024

Conversation

ouyuanning
Copy link
Contributor

@ouyuanning ouyuanning commented Sep 11, 2024

User description

bug fix: unique key/secondary key get incorrect value

Approved by: @heni02, @badboynt1, @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/4059

What this PR does / why we need it:

bug fix: unique key/secondary key get incorrect value


PR Type

Bug fix, Tests


Description

  • Fixed a bug where unique and secondary keys were getting incorrect values by simplifying the logic for computing primary key positions.
  • Removed unnecessary map creation and redundant loops to improve code efficiency.
  • Added new test cases to verify the correctness of queries involving composite primary keys.

Changes walkthrough 📝

Relevant files
Bug fix
build_dml_util.go
Simplify primary key position computation logic                   

pkg/sql/plan/build_dml_util.go

  • Removed unnecessary map creation for primary key names.
  • Simplified logic to find primary key positions in columns.
  • Improved efficiency by removing redundant loops.
  • +7/-8     
    Tests
    select.result
    Add test cases for composite primary key queries                 

    test/distributed/cases/dml/select/select.result

  • Added test cases for new table structure with composite primary key.
  • Verified query results for specific conditions.
  • +8/-1     
    select.test
    Introduce tests for tables with composite primary keys     

    test/distributed/cases/dml/select/select.test

  • Introduced new test setup for tables with composite primary keys.
  • Included test queries to validate new logic.
  • +5/-0     

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

    …8711)
    
    bug fix: unique key/secondary key get incorrect value
    
    Approved by: @heni02, @badboynt1, @aunjgr
    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 🔵🔵⚪⚪⚪
    🧪 PR contains tests
    🔒 No security concerns identified
    ⚡ Key issues to review

    Code Simplification
    The simplified logic for finding primary key positions may need careful review to ensure it covers all edge cases.

    @matrix-meow matrix-meow added the size/S Denotes a PR that changes [10,99] lines label Sep 11, 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
    Use a map to improve efficiency when finding primary key positions in columns

    Consider using a more efficient method to find primary key positions. Instead of
    nested loops, you could use a map to store column positions and then directly access
    them for primary key columns.

    pkg/sql/plan/build_dml_util.go [763-770]

    +colPosMap := make(map[string]int)
    +for i, coldef := range tableDef.Cols {
    +  colPosMap[coldef.Name] = i
    +}
     for _, name := range tableDef.Pkey.Names {
    -  for i, coldef := range tableDef.Cols {
    -    if coldef.Name == name {
    -      prikeyPos = append(prikeyPos, i)
    -      break
    -    }
    +  if pos, ok := colPosMap[name]; ok {
    +    prikeyPos = append(prikeyPos, pos)
       }
     }
     
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: The suggestion correctly identifies a performance improvement by replacing nested loops with a map for direct access, which reduces time complexity and enhances efficiency.

    8
    Pre-allocate slice capacity to improve performance and reduce memory allocations

    Consider pre-allocating the prikeyPos slice with the expected capacity to avoid
    potential reallocation overhead.

    pkg/sql/plan/build_dml_util.go [762-770]

    -prikeyPos := make([]int, 0)
    +prikeyPos := make([]int, 0, len(tableDef.Pkey.Names))
     for _, name := range tableDef.Pkey.Names {
       for i, coldef := range tableDef.Cols {
         if coldef.Name == name {
           prikeyPos = append(prikeyPos, i)
           break
         }
       }
     }
     
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: Pre-allocating the slice with the expected capacity is a minor optimization that can reduce memory allocations and improve performance slightly, but it is not as impactful as the first suggestion.

    7

    @mergify mergify bot requested a review from sukki37 September 11, 2024 14:35
    @mergify mergify bot added the kind/bug Something isn't working label Sep 11, 2024
    @sukki37 sukki37 merged commit 9eebbf7 into matrixorigin:1.2-dev Sep 12, 2024
    16 of 17 checks passed
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    Bug fix kind/bug Something isn't working Review effort [1-5]: 2 size/S Denotes a PR that changes [10,99] lines Tests
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    5 participants