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 json_extract(#1.1-dev) #15305

Merged
merged 4 commits into from Apr 3, 2024
Merged

Conversation

YANGGMM
Copy link
Contributor

@YANGGMM YANGGMM commented Apr 3, 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 #14909

What this PR does / why we need it:

fix json_extract

@matrix-meow matrix-meow added the size/XS Denotes a PR that changes [1, 9] lines label Apr 3, 2024
@mergify mergify bot requested a review from sukki37 April 3, 2024 02:37
@mergify mergify bot added the kind/bug Something isn't working label Apr 3, 2024
@matrix-meow
Copy link
Contributor

@YANGGMM Thanks for your contributions!

Pull Request Review:

Title:

The title of the pull request is concise and clear, indicating that it fixes an issue related to json_extract(#1.1-dev).

Body:

The body of the pull request provides relevant information about the type of PR (BUG), the specific issue it fixes (issue #14909), and the reason for the PR ("fix json_extract").

Changes:

The changes made in the pull request involve modifying the logic in the query function of the ByteJson struct in the bytejson.go file. Specifically, the change is in the condition where last is true and idx is less than 0 or equal to cnt. The modification changes the comparison from idx < 0 to cnt <= idx.

Feedback and Suggestions for Improvement:

  1. Boundary Condition Issue:

    • The condition last && idx < 0 || cnt <= idx may have a logical error. If last is true and idx is less than 0, the current logic appends a LiteralNull to the cur slice. However, if last is true and idx is equal to cnt, the logic also appends LiteralNull, which may not be the intended behavior.
    • Suggestion: Review the logic and ensure that the conditions are correctly handling the cases where last is true and idx is less than 0 or equal to cnt. Adjust the logic to handle these cases appropriately.
  2. Code Optimization:

    • The code change itself seems straightforward and focused on fixing a specific issue. However, it's essential to ensure that the modification does not introduce unintended consequences.
    • Suggestion: Consider adding comments to explain the rationale behind the condition change for better code readability and maintainability.
  3. Security Considerations:

    • While the code change itself does not appear to introduce security vulnerabilities, it's always good practice to review any modifications that involve conditional logic to prevent potential security risks.
    • Suggestion: Perform a thorough review of the surrounding code to ensure that the changes do not inadvertently introduce security vulnerabilities.
  4. Testing:

    • It would be beneficial to include test cases that cover the scenarios affected by this change to ensure that the modified logic functions as expected and does not introduce regressions.
    • Suggestion: Add relevant test cases to validate the behavior of the query function under different conditions, including the scenarios addressed by this fix.
  5. Documentation:

    • Consider updating the documentation or adding comments in the code to explain the purpose of the query function and the specific conditions being modified for better code understanding.
    • Suggestion: Document the changes made and any potential implications for future developers working on this codebase.

Overall, the pull request addresses a specific issue related to json_extract and makes a targeted code change. However, it's crucial to ensure that the modified logic functions correctly under all scenarios and does not introduce new issues. Additionally, incorporating testing and documentation improvements can enhance the overall quality and maintainability of the codebase.

@mergify mergify bot merged commit 0d276f1 into matrixorigin:1.1-dev Apr 3, 2024
17 of 18 checks passed
@YANGGMM YANGGMM deleted the fix-json-1.1-dev branch May 13, 2024 02:50
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/XS Denotes a PR that changes [1, 9] lines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants