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 non reserverd keyword last #14014

Merged
merged 1 commit into from
Jan 4, 2024
Merged

Conversation

iamlinjunhong
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 #2096 (mo-cloud)

What this PR does / why we need it:

fix non reserverd keyword last

@matrix-meow matrix-meow added the size/XXL Denotes a PR that changes 2000+ lines label Jan 4, 2024
@mergify mergify bot requested a review from sukki37 January 4, 2024 08:57
@matrix-meow
Copy link
Contributor

@iamlinjunhong Thanks for your contributions!

Here are review comments for file pkg/sql/parsers/dialect/mysql/mysql_sql.go:
Based on the provided information, it seems that the pull request is titled "fix non reserved keyword last" and it aims to address issue #2096 related to "mo-cloud". However, there are no changes provided in the pull request diff for the file pkg/sql/parsers/dialect/mysql/mysql_sql.go.

Since there are no changes to review, it is not possible to provide any feedback or suggestions for improvement. It is recommended to either update the pull request with the relevant changes or provide more information about the intended changes in the body of the pull request.

Here are review comments for file pkg/sql/parsers/dialect/mysql/mysql_sql.y:
Review:

Title: fix non reserverd keyword last

Body: The pull request aims to fix an issue related to the non-reserved keyword "last". It is not clear what the specific problem is or why this fix is necessary.

Changes made in the pull request:

  • In the file pkg/sql/parsers/dialect/mysql/mysql_sql.y, a new non-reserved keyword "LAST" has been added.

Suggestions for improvement:

  1. Provide a more detailed explanation in the pull request body about the issue being fixed and why it is necessary. This will help reviewers understand the context and importance of the fix.

Security concerns:
No security concerns are apparent based on the provided information.

Optimization suggestions:
No optimization suggestions can be made based on the provided information.

Here are review comments for file pkg/sql/parsers/dialect/mysql/mysql_sql_test.go:
Review:

Title: fix non reserved keyword last

Body: The pull request is fixing an issue with a non-reserved keyword "last" in the MySQL dialect parser.

Changes made in the file pkg/sql/parsers/dialect/mysql/mysql_sql_test.go:

  • Line 30: The input query has been changed from "select reference from t1" to "select last from t1".
  • Line 31: The expected output has been changed from "select reference from t1" to "select last from t1".

Suggestions:

  1. The title of the pull request could be more descriptive. Instead of "fix non reserved keyword last", it could be "Fix parsing issue with non-reserved keyword 'last' in MySQL dialect parser".
  2. The body of the pull request is incomplete. It should provide more context about the issue and why the change is necessary.
  3. It would be helpful to mention any relevant tests that have been added or modified to ensure the fix is working correctly.
  4. Consider adding a link to the issue [Feature Request]: Mathematical Built-in function crc32()  #2096 (mo-cloud) in the body of the pull request for better traceability.

Security Concerns:
There don't appear to be any security concerns in this pull request.

Optimization:
The changes made in the pull request are minimal and seem to be focused on fixing a specific issue. It would be beneficial to review the entire codebase and identify any other potential issues or optimizations that can be addressed in the same pull request. This will help improve the overall quality and efficiency of the codebase.

Here are review comments for file test/distributed/cases/util/keywords.result:
Title: fix non reserved keyword last

Body: This pull request fixes issue #2096 (mo-cloud). It addresses a problem with a non-reserved keyword "last".

Changes in file test/distributed/cases/util/keywords.result:

  • Added a drop table statement for table "tt2" before creating it.
  • Added a drop table statement for table "tt2" after creating it.
  • Added a drop table statement for table "tt3" before creating it.
  • Added a create table statement for table "tt3" with a column "last".
  • Added a drop table statement for table "tt3" after creating it.

Review:

  1. The title of the pull request is clear and concise, indicating that it fixes a problem related to a non-reserved keyword "last".

  2. The body of the pull request provides information about the type of PR and the issue it fixes. It also mentions the reason for the PR, which is to fix the non-reserved keyword "last".

  3. The changes made in the file "test/distributed/cases/util/keywords.result" are as follows:

    • Added a drop table statement for table "tt2" before creating it. This ensures that the table is dropped if it already exists before creating it again.
    • Added a drop table statement for table "tt2" after creating it. This ensures that the table is dropped after it is no longer needed.
    • Added a drop table statement for table "tt3" before creating it. This ensures that the table is dropped if it already exists before creating it again.
    • Added a create table statement for table "tt3" with a column "last". This creates a new table "tt3" with a single column "last".
    • Added a drop table statement for table "tt3" after creating it. This ensures that the table is dropped after it is no longer needed.

Suggestions:

  1. It is recommended to provide more detailed information in the body of the pull request, such as the specific problem with the non-reserved keyword "last" and how the changes in the file address that problem. This will help reviewers understand the context and purpose of the changes more effectively.

  2. It is good practice to include test cases or test results in the pull request to ensure that the changes are properly tested and do not introduce any regressions.

  3. It is advisable to follow a consistent coding style throughout the file. In this case, the file "test/distributed/cases/util/keywords.result" has inconsistent line endings (some lines have a newline at the end, while others do not). It is recommended to have a consistent line ending style, either with or without a newline at the end of the file.

  4. It is important to ensure that the changes made in the file do not introduce any security vulnerabilities. In this case, the changes seem to be related to table creation and deletion, which may not have direct security implications. However, it is always a good practice to review the changes for any potential security issues, such as SQL injection vulnerabilities or improper access control.

Here are review comments for file test/distributed/cases/util/keywords.sql:
Review:

Title:
The title of the pull request is "fix non reserved keyword last". The title is concise and provides a clear indication of the purpose of the pull request.

Body:
The body of the pull request does not provide any additional information or explanation about the changes made. It only includes a checklist for the type of PR and a reference to the issue being fixed. It would be helpful to include a brief description of the problem being addressed and why the changes are necessary.

Changes:
The changes made in the pull request are in the file "test/distributed/cases/util/keywords.sql". The changes include the following:

  1. Added a drop table statement for table "tt2" before creating it. This ensures that the table is dropped if it already exists before creating a new one.

  2. Added a drop table statement for table "tt2" after creating it. This ensures that the table is dropped after it has been used.

  3. Added a drop table statement for table "tt3" before creating it. This ensures that the table is dropped if it already exists before creating a new one.

  4. Added a drop table statement for table "tt3" after creating it. This ensures that the table is dropped after it has been used.

Suggestions:

  1. Provide a more detailed description in the pull request body, explaining the problem being addressed and why the changes are necessary. This will help reviewers understand the context and importance of the changes.

  2. Consider using a more descriptive commit message for each change made in the file. This will make it easier to understand the purpose of each change when reviewing the commit history.

  3. Instead of dropping and creating the "tt2" table multiple times, consider using a single drop table statement at the beginning and end of the file. This will make the code more concise and reduce redundancy.

  4. Instead of dropping and creating the "tt3" table multiple times, consider using a single drop table statement at the beginning and end of the file. This will make the code more concise and reduce redundancy.

Security Issues:
There are no apparent security issues in the changes made in the pull request.

@mergify mergify bot merged commit c3ee517 into matrixorigin:1.1-dev Jan 4, 2024
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size/XXL Denotes a PR that changes 2000+ lines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants