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 lower case table names (#1.1-dev) #15098

Merged
merged 2 commits into from
Mar 23, 2024

Conversation

YANGGMM
Copy link
Contributor

@YANGGMM YANGGMM commented Mar 22, 2024

fix lower case table names

Approved by: @daviszhen

What type of PR is this?

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

Which issue(s) this PR fixes:

issue ##15031

What this PR does / why we need it:

fix lower case table names

fix lower case table names

Approved by: @daviszhen
@matrix-meow matrix-meow added the size/S Denotes a PR that changes [10,99] lines label Mar 22, 2024
@mergify mergify bot requested a review from sukki37 March 22, 2024 02:22
@mergify mergify bot added the kind/bug Something isn't working label Mar 22, 2024
@matrix-meow
Copy link
Contributor

@YANGGMM Thanks for your contributions!

Pull Request Review:

Title:

The title of the pull request is clear and concise, indicating that it fixes lower case table names.

Body:

The body of the pull request provides additional context by mentioning the issue it fixes and the approval status.

Changes:

  1. In configuration.go:

    • Changed the type of LowerCaseTableNames from int64 to string.
    • Updated the default value assignment for LowerCaseTableNames to use a string constant defaultLowerCaseTableNames.
  2. In routine_manager_test.go:

    • Added a call to SetDefaultValues() method for pu.SV object.
  3. In variables.go:

    • Added a new conversion function cv3 to handle conversion of string values to integers in SystemVariableIntType struct.

Feedback and Suggestions for Improvement:

  1. Type Mismatch in configuration.go:

    • Issue: Changing the type of LowerCaseTableNames from int64 to string might introduce type inconsistencies or unexpected behavior in the codebase.
    • Suggestion: It's recommended to keep the type consistent with the purpose of the variable. If LowerCaseTableNames is meant to represent an integer value, it should remain as int64. If string representation is needed, consider adding a separate field for that purpose.
  2. Default Value Assignment in configuration.go:

    • Issue: Using a hard-coded string value for defaultLowerCaseTableNames may lead to maintenance issues if the default value needs to be changed in the future.
    • Suggestion: Instead of hard-coding the default value, consider using a constant or a configuration setting that can be easily modified without changing the code.
  3. Call to SetDefaultValues() in routine_manager_test.go:

    • Issue: Directly calling SetDefaultValues() without proper context or explanation in the test file may lead to confusion or unexpected behavior during testing.
    • Suggestion: Add a comment or documentation explaining the reason for calling SetDefaultValues() in the test setup to provide clarity to other developers.
  4. New Conversion Function in variables.go:

    • Issue: Introducing a new conversion function cv3 for handling string to integer conversions may add complexity and reduce code readability.
    • Suggestion: Instead of adding a new function, consider refactoring the existing conversion logic to handle string conversions within the existing functions to maintain code simplicity and clarity.
  5. Security Concerns:

    • Ensure that any changes related to table names or configurations do not introduce vulnerabilities like SQL injection or data corruption. Validate user inputs and configurations properly to prevent security risks.
  6. Optimization:

    • Review the necessity of changing the type of LowerCaseTableNames and ensure it aligns with the overall design and requirements of the system to avoid unnecessary complexity.

By addressing the mentioned issues and considering the suggestions provided, the codebase can be improved in terms of consistency, maintainability, and security.

@mergify mergify bot merged commit 5353173 into matrixorigin:1.1-dev Mar 23, 2024
17 of 18 checks passed
@YANGGMM YANGGMM deleted the fix-set-lower-1.1-dev branch May 13, 2024 02:49
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

4 participants