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

Revert "cherry-pick sys variables" #15560

Merged
merged 1 commit into from
Apr 16, 2024
Merged

Conversation

sukki37
Copy link
Contributor

@sukki37 sukki37 commented Apr 16, 2024

Reverts #15330

@matrix-meow matrix-meow added the size/S Denotes a PR that changes [10,99] lines label Apr 16, 2024
@matrix-meow
Copy link
Contributor

@sukki37 Thanks for your contributions!

Pull Request Review:

Title:

The title of the pull request indicates that it is reverting a previous change related to system variables.

Body:

The body of the pull request simply states that it reverts a specific commit.

Changes:

  1. In pkg/config/configuration.go:

    • The removal of the dafaultSqlMode variable which was a typo for defaultSqlMode.
    • The removal of the SqlMode field from the FrontendParameters struct and related code in the SetDefaultValues function.
  2. In pkg/frontend/session.go:

    • Removal of the getUpdateVariableSqlsByToml function which was used to generate SQL update statements for system variables.
    • Removal of the code that executed SQL update statements for system variables in the InitGlobalSystemVariables function.

Feedback and Suggestions:

  1. Typo Fix:

    • The removal of dafaultSqlMode was correct as it was a typo. However, it should be replaced with the correct variable name defaultSqlMode if needed elsewhere in the codebase.
  2. SqlMode Field Removal:

    • The removal of the SqlMode field from the FrontendParameters struct and related code in the SetDefaultValues function seems intentional. If this field is no longer needed, the removal is appropriate.
  3. System Variable Handling:

    • The removal of the getUpdateVariableSqlsByToml function and related code in session.go may have been done for a reason. If system variables are no longer updated in this manner, it's fine. However, if this functionality is still required, it should be refactored rather than completely removed.
  4. Code Optimization:

    • Consider optimizing the code further by ensuring that all unused or redundant code is removed. This helps in maintaining a clean and efficient codebase.
  5. Security Concerns:

    • While the changes in this pull request do not introduce security vulnerabilities directly, it's important to ensure that any system variable handling is done securely to prevent potential SQL injection attacks. If system variables are being set dynamically, proper validation and sanitization should be applied.
  6. Documentation:

    • Ensure that any significant changes like the removal of variables or functions are documented properly to aid in understanding the codebase for future developers.
  7. Testing:

    • After making these changes, it's crucial to run tests to verify that the functionality of the application has not been affected adversely.

By addressing the above points, the codebase can be improved in terms of clarity, efficiency, and security.

@sukki37 sukki37 merged commit 14c4496 into 1.1-dev Apr 16, 2024
15 of 16 checks passed
@sukki37 sukki37 deleted the revert-15330-fix-var-1.1-dev branch April 16, 2024 15:08
ck89119 pushed a commit to ck89119/matrixone that referenced this pull request Apr 19, 2024
mergify bot pushed a commit that referenced this pull request Apr 22, 2024
Revert "cherry-pick sys variables" (#15560)

Approved by: @qingxinhome, @daviszhen
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.

2 participants