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_string, json_extract_float64 with multi path param #18770

Merged
merged 6 commits into from
Sep 13, 2024

Conversation

YANGGMM
Copy link
Contributor

@YANGGMM YANGGMM commented Sep 13, 2024

User description

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/4066

What this PR does / why we need it:

fix json_extract_string, json_extract_float64 with multi path param


PR Type

Bug fix, Tests


Description

  • Fixed the json_extract_string and json_extract_float64 functions to handle multiple path parameters correctly.
  • Updated the condition to check for op.simple and ensure paths retrieve a single value.
  • Added new test cases to verify the behavior of JSON extraction functions with multiple path parameters.
  • Ensured that invalid input is correctly handled and error messages are displayed.

Changes walkthrough 📝

Relevant files
Bug fix
func_builtin_json.go
Fix JSON extraction functions to handle multiple paths     

pkg/sql/plan/function/func_builtin_json.go

  • Modified condition to handle multiple JSON path parameters.
  • Added check for op.simple and path length.
  • +2/-2     
    Tests
    func_json_extract.result
    Add test results for JSON extraction with multiple paths 

    test/distributed/cases/function/func_json_extract.result

  • Added test cases for json_extract, json_extract_float64, and
    json_extract_string.
  • Verified behavior with multiple JSON path parameters.
  • +18/-0   
    func_json_extract.test
    Add test cases for JSON extraction with multiple paths     

    test/distributed/cases/function/func_json_extract.test

  • Added new test cases for JSON extraction functions.
  • Tested behavior with multiple JSON path parameters.
  • +11/-0   

    💡 PR-Agent usage:
    Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

    Copy link

    PR-Agent was enabled for this repository. To continue using it, please link your git user with your CodiumAI identity here.

    PR Reviewer Guide 🔍

    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🧪 PR contains tests
    🔒 No security concerns identified
    ⚡ Key issues to review

    Logic Change
    The condition for checking simple paths has been modified. Ensure this change doesn't introduce unintended side effects.

    Consistency
    Verify that the same logic change is applied consistently to both jsonExtractString and jsonExtractFloat64 functions.

    @mergify mergify bot added the kind/bug Something isn't working label Sep 13, 2024
    Copy link

    codiumai-pr-agent-pro bot commented Sep 13, 2024

    PR-Agent was enabled for this repository. To continue using it, please link your git user with your CodiumAI identity here.

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Best practice
    Correct spelling error in error message

    Fix the typo in the error message: "retrives" should be "retrieves".

    pkg/sql/plan/function/func_builtin_json.go [272]

    -return moerr.NewInvalidInput(proc.Ctx, "json_extract_value should use a path that retrives a single value")
    +return moerr.NewInvalidInput(proc.Ctx, "json_extract_value should use a path that retrieves a single value")
     
    • Apply this suggestion
    Suggestion importance[1-10]: 10

    Why: Correcting the typo in the error message is crucial for professionalism and clarity, ensuring that users receive accurate information.

    10
    Extract error message into a constant for better maintainability

    Consider extracting the error message into a constant variable to improve
    maintainability and consistency across the codebase.

    pkg/sql/plan/function/func_builtin_json.go [272]

    -return moerr.NewInvalidInput(proc.Ctx, "json_extract_value should use a path that retrives a single value")
    +const errMsgSingleValuePath = "json_extract_value should use a path that retrieves a single value"
    +return moerr.NewInvalidInput(proc.Ctx, errMsgSingleValuePath)
     
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: Extracting the error message into a constant improves maintainability and consistency, making it easier to update the message in the future if needed.

    7
    Enhancement
    Provide more specific error message with path count information

    Consider adding a more specific error message that includes information about the
    number of paths provided, to help users understand why their input is invalid.

    pkg/sql/plan/function/func_builtin_json.go [272]

    -return moerr.NewInvalidInput(proc.Ctx, "json_extract_value should use a path that retrives a single value")
    +return moerr.NewInvalidInput(proc.Ctx, fmt.Sprintf("json_extract_value should use a path that retrieves a single value, but %d paths were provided", len(op.paths)))
     
    • Apply this suggestion
    Suggestion importance[1-10]: 9

    Why: Adding specific information about the number of paths provided in the error message enhances user understanding and debugging, making it a valuable improvement.

    9
    Simplify the condition by removing redundant check

    Simplify the condition by combining the checks for op.simple and len(op.paths) into
    a single logical expression.

    pkg/sql/plan/function/func_builtin_json.go [271]

    -if !op.simple || op.simple && len(op.paths) > 1 {
    +if !op.simple || len(op.paths) > 1 {
     
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: The suggestion simplifies the condition by removing the redundant check for op.simple, which improves code readability without altering functionality.

    8

    @matrix-meow matrix-meow added the size/S Denotes a PR that changes [10,99] lines label Sep 13, 2024
    @mergify mergify bot merged commit 06297d7 into matrixorigin:main Sep 13, 2024
    18 checks passed
    YANGGMM added a commit to YANGGMM/matrixone that referenced this pull request Sep 19, 2024
    …atrixorigin#18770)
    
    fix json_extract_string, json_extract_float64 with multi path param
    
    Approved by: @m-schen, @heni02
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    Bug fix kind/bug Something isn't working Review effort [1-5]: 2 size/S Denotes a PR that changes [10,99] lines Tests
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    4 participants