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

add collate utf_8 case(1.1-dev) #15223

Merged
merged 3 commits into from
Mar 29, 2024

Conversation

Ariznawlll
Copy link
Contributor

@Ariznawlll Ariznawlll commented Mar 28, 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/2666

What this PR does / why we need it:

add collate utf8_general_ci case

@matrix-meow matrix-meow added the size/XS Denotes a PR that changes [1, 9] lines label Mar 28, 2024
@mergify mergify bot requested a review from sukki37 March 28, 2024 09:11
@matrix-meow
Copy link
Contributor

@Ariznawlll Thanks for your contributions!

Pull Request Review:

Title:

The title of the pull request is clear and indicates that the changes are related to adding collation for UTF-8 in a specific case.

Body:

The body of the pull request includes information about the type of PR, the related issue, and the purpose of the changes. It would be helpful to have more context on why this specific collation is being added and how it improves the codebase.

Changes:

  1. Two new files are added:

    • information_schema.result
    • information_schema.sql
  2. The changes involve adding a SQL query in the information_schema.sql file that uses collation for utf8_general_ci in a specific query condition.

Feedback and Suggestions:

  1. Context and Explanation:

    • It would be beneficial to provide more context in the body of the pull request explaining why this specific collation is being added and how it benefits the codebase or resolves the issue fix UpdateOffsets #2666.
  2. Security Concerns:

    • Ensure that adding collation does not introduce any security vulnerabilities like SQL injection. Sanitize user inputs and use parameterized queries to prevent such risks.
  3. Code Optimization:

    • Consider refactoring the SQL query to make it more readable and maintainable. Break it down into smaller parts if necessary for better understanding.
  4. Consistency:

    • Ensure consistency in the naming conventions and formatting of the new files added to maintain a uniform codebase structure.
  5. Testing:

    • It would be beneficial to include test cases for the added functionality to ensure it works as expected and does not introduce regressions.
  6. Documentation:

    • If this change impacts how developers interact with the codebase, update the documentation to reflect the new collation usage.
  7. Review SQL Query:

    • Review the SQL query added to ensure it aligns with best practices and does not have any performance implications.

By addressing the above points, the pull request can be enhanced in terms of clarity, security, and overall code quality.

@Ariznawlll Ariznawlll changed the title add collate utf 8 case(1.1-dev) add collate utf_8 case(1.1-dev) Mar 28, 2024
@sukki37 sukki37 merged commit aa8651f into matrixorigin:1.1-dev Mar 29, 2024
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size/XS Denotes a PR that changes [1, 9] lines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants