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

cherry pick fix-dup-ww-3 #15123

Conversation

triump2020
Copy link
Contributor

What type of PR is this?

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

Which issue(s) this PR fixes:

issue #14895 #14880 #14562 #14405 #13541 #14548 #13872

What this PR does / why we need it:

  1. fix dup reported by CN and ww reported by DN.

@matrix-meow matrix-meow added the size/S Denotes a PR that changes [10,99] lines label Mar 23, 2024
@mergify mergify bot requested a review from sukki37 March 23, 2024 06:21
@matrix-meow
Copy link
Contributor

@triump2020 Thanks for your contributions!

Pull Request Review:

Title:

The title of the pull request "cherry pick fix-dup-ww-3" indicates that this PR is a cherry-pick to fix issues related to duplicates and workspaces.

Body:

The body of the pull request provides information about the type of PR (BUG), the issues it fixes, and a brief description of what the PR does.

Changes:

  1. pkg/frontend/txn_test.go:

    • Removed the TransferRowID function implementation and related test expectations. This function was not implemented and was causing panics.
  2. pkg/frontend/util_test.go:

    • Removed the TransferRowID function test expectation. This function was not implemented and was causing unnecessary test failures.
  3. pkg/sql/compile/compile.go:

    • Removed the call to TransferRowID function from the runOnce method. This function was not implemented and was causing unnecessary calls.
  4. pkg/sql/compile/compile_test.go:

    • Removed the TransferRowID function implementation. This function was empty and not being used.
  5. pkg/sql/compile/compile_test.go:

    • Removed the TransferRowID function test expectation. This function was not implemented and was causing unnecessary test expectations.
  6. pkg/txn/client/types.go:

    • Removed the TransferRowID function declaration from the Workspace interface. This function was not implemented and was unnecessary.
  7. pkg/vm/engine/disttae/txn.go:

    • Refactored the TransferRowID function into forEachTableHasDeletesLocked and transferDeletesLocked functions. Added comments for future refactoring.
  8. pkg/vm/engine/disttae/txn_table.go:

    • Renamed the transferRowid function to transferDeletes and added a comment for future refactoring.
  9. pkg/vm/engine/disttae/types.go:

    • Added a call to transferDeletesLocked function in the handleRCSnapshot method for handling row id transfers in RC isolation.

Suggestions and Security Issues:

  1. Unused Function Removal:

    • The removal of the TransferRowID function and related code is appropriate as it was not implemented and was causing unnecessary panics and test failures.
  2. Code Refactoring:

    • Refactoring the TransferRowID function into forEachTableHasDeletesLocked and transferDeletesLocked functions is a good step towards improving code readability and maintainability.
  3. Future Refactoring:

    • The comments added for future refactoring in the txn.go and txn_table.go files are helpful for indicating areas that can be improved in subsequent PRs.
  4. Security Concerns:

    • No specific security issues were identified in the changes made in this pull request.

Overall:

The changes in this pull request focus on removing unused and unimplemented code related to TransferRowID function. The refactoring and comments added for future improvements are positive steps. Ensure that the refactored code is thoroughly tested to maintain functionality. Consider addressing any other similar unused functions or code snippets in the codebase for better code hygiene.

@mergify mergify bot merged commit 9407263 into matrixorigin:1.1-dev Mar 23, 2024
17 of 18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size/S Denotes a PR that changes [10,99] lines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants