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-1.1-dev] fix load data stmt affect rows count calculation incorrectly #15435

Merged
merged 3 commits into from Apr 10, 2024

Conversation

noorall
Copy link
Contributor

@noorall noorall commented Apr 10, 2024

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

What this PR does / why we need it:

fix load data stmt affect rows count calculation incorrectly

noorall and others added 2 commits April 10, 2024 15:32
…xorigin#14364)

Fix load data stmt affect rows count calculation incorrectly.

Approved by: @m-schen, @nnsgmsone

(cherry picked from commit 959a08c)
@matrix-meow matrix-meow added the size/S Denotes a PR that changes [10,99] lines label Apr 10, 2024
@mergify mergify bot requested a review from sukki37 April 10, 2024 07:37
@mergify mergify bot added the kind/bug Something isn't working label Apr 10, 2024
@matrix-meow
Copy link
Contributor

@noorall Thanks for your contributions!

Pull Request Review:

Title:

The title of the pull request is clear and concise, indicating that the PR is related to fixing an issue with the calculation of affected rows in the load data statement.

Body:

The body of the pull request provides relevant information about the type of PR (BUG), the specific issue it fixes, and why the PR is needed. It references the related GitHub issue for context.

Changes:

  1. In mergeblock_test.go:

    • Added AddAffectedRows: true to two test cases.
  2. In types.go:

    • Added a new boolean field AddAffectedRows to the Argument struct.
    • Modified the Split function to conditionally update arg.affectedRows based on the value of AddAffectedRows.
  3. In compile.go:

    • Added AddAffectedRows field to the Argument struct when compiling the plan scope.

Feedback and Suggestions:

  1. Code Optimization:

    • Instead of adding a new boolean field AddAffectedRows to the Argument struct, consider using a default value or refactoring the logic to avoid the need for this additional field. This can help simplify the code and reduce complexity.
  2. Conditional Logic:

    • The conditional checks in the Split function could be consolidated to improve readability. Consider refactoring the logic to make it more concise and easier to follow.
  3. Consistency:

    • Ensure consistency in variable naming and coding style across the changes made in different files to maintain code readability and maintainability.
  4. Security Concern:

    • When introducing conditional logic based on the AddAffectedRows flag, ensure that this does not introduce any vulnerabilities related to data integrity or unexpected behavior. Perform thorough testing to validate the changes.
  5. Documentation:

    • Consider adding comments or updating existing documentation to explain the purpose of the AddAffectedRows field and its impact on the affected rows calculation for better code understanding.
  6. Testing:

    • Verify that the changes do not introduce regressions and that the affected rows calculation is accurate in all scenarios, including edge cases.
  7. Review and Approval:

    • Request a review from a team member to validate the changes and ensure they align with the project's coding standards and best practices.

By addressing the optimization suggestions and ensuring the security implications are considered, the pull request can be enhanced for better code quality and maintainability.

@mergify mergify bot merged commit 6688544 into matrixorigin:1.1-dev Apr 10, 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
kind/bug Something isn't working size/S Denotes a PR that changes [10,99] lines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants