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

Fix mo cloud-issue-3298 (#16254) #16281

Merged

Conversation

triump2020
Copy link
Contributor

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/3298

What this PR does / why we need it:

  1. When open PK deduplication against txn's workspace, should skip mo_tables, mo_columns, mo_database .
  2. When open PK deduplication or not , make sure it can pass BVT test.

1.  When open PK deduplication against txn's workspace, should  skip mo_tables, mo_columns, mo_database .
2.  When open PK deduplication  or not ,  make sure it can pass BVT test.

Approved by: @XuPeng-SH, @heni02
@matrix-meow matrix-meow added the size/S Denotes a PR that changes [10,99] lines label May 21, 2024
@mergify mergify bot requested a review from sukki37 May 21, 2024 04:04
@matrix-meow
Copy link
Contributor

@triump2020 Thanks for your contributions!

Pull Request Review:

Title:

The title of the pull request is clear and concise, indicating that it is fixing an issue related to "mo cloud-issue-3298."

Body:

The body of the pull request provides information on the type of PR, the specific issue being addressed, and the purpose of the changes made in the PR. It mentions that the changes are related to PK deduplication and passing BVT tests.

Changes:

  1. In txn.go:

    • The changes made in txn.go involve modifying the checkDup function to skip certain conditions related to mo_tables, mo_columns, and mo_database. This change is aimed at improving the behavior when open PK deduplication against txn's workspace.
    • The condition e.isCatalog() is introduced to skip certain operations based on the type of event.
  2. In load_data.result and auto_increment_2.csv:

    • The changes in these files seem to be related to test data modifications, specifically altering values in the test data files to ensure they pass the BVT test.

Feedback and Suggestions for Improvement:

  1. Security Issue:

    • The changes made in the txn.go file seem to be related to skipping certain operations based on specific conditions. It's crucial to ensure that skipping these operations does not introduce any security vulnerabilities or unexpected behavior. Consider thoroughly testing these changes to prevent any potential security risks.
  2. Code Optimization:

    • In the txn.go file, the condition e.isCatalog() is introduced to skip certain operations. It might be beneficial to provide a clear comment explaining the purpose of this condition to improve code readability and maintainability.
  3. Test Data Modifications:

    • While modifying test data for BVT tests is essential, ensure that the changes made in the test data files accurately reflect the expected behavior and do not introduce inconsistencies that could affect the test results.
  4. Consistency in Changes:

    • Ensure consistency in the changes across different files. For example, in the load_data.result file, there are changes related to dropping tables and modifying data. Make sure these changes align with the overall goal of the PR and do not introduce unnecessary modifications.
  5. Documentation:

    • Consider adding comments or updating documentation to explain the rationale behind the changes made in the PR. This can help other developers understand the purpose of the modifications and facilitate future maintenance.
  6. End of File Changes:

    • In the load_data.result and auto_increment_2.csv files, there are changes related to the end of the file (e.g., removing or adding new lines). Ensure that these changes do not impact the readability or functionality of the test data files.
  7. Review and Testing:

    • Before merging the PR, conduct thorough code reviews and testing to ensure that the changes do not introduce regressions or unexpected behavior. Consider involving team members for additional feedback and validation.

By addressing the security concerns, optimizing the code changes, ensuring consistency, and documenting the modifications, the quality and maintainability of the codebase can be improved.

@mergify mergify bot merged commit 472c844 into matrixorigin:1.2-dev May 21, 2024
18 of 19 checks passed
@aylei aylei mentioned this pull request Jun 5, 2024
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.

5 participants