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

[Feature Request]: strcmp function #18483

Open
wants to merge 13 commits into
base: main
Choose a base branch
from

Conversation

leo-livis
Copy link

@leo-livis leo-livis commented Sep 2, 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 #15359

What this PR does / why we need it:

Compatible with MySQL functions


PR Type

Enhancement


Description

  • Introduced a new Strcmp function to perform string comparisons, compatible with MySQL's strcmp.
  • Registered the STRCMP function ID and added it to the function ID map.
  • Included strcmp in the list of supported built-in string functions with proper overload definitions.

Changes walkthrough 📝

Relevant files
Enhancement
func_binary.go
Add Strcmp function for string comparison                               

pkg/sql/plan/function/func_binary.go

  • Added Strcmp function to handle string comparison.
  • Implemented strcmp helper function using strings.Compare.
  • +8/-0     
    function_id.go
    Register STRCMP function ID and mapping                                   

    pkg/sql/plan/function/function_id.go

  • Registered STRCMP constant for function identification.
  • Added strcmp to function ID register map.
  • +2/-0     
    list_builtIn.go
    Add strcmp to supported built-in functions                             

    pkg/sql/plan/function/list_builtIn.go

  • Added strcmp to supported string built-in functions.
  • Defined overload for strcmp with varchar arguments.
  • +21/-0   

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

    @leo-livis leo-livis requested a review from m-schen as a code owner September 2, 2024 11:15
    @CLAassistant
    Copy link

    CLAassistant commented Sep 2, 2024

    CLA assistant check
    All committers have signed the CLA.

    Copy link

    qodo-merge-pro bot commented Sep 2, 2024

    PR Reviewer Guide 🔍

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

    Possible Bug
    The strcmp function uses int8 as return type, which might lead to overflow for large string comparisons.

    Incomplete Implementation
    The strcmp function is only implemented for varchar types, but not for other string-like types such as char or text.

    @matrix-meow matrix-meow added the size/S Denotes a PR that changes [10,99] lines label Sep 2, 2024
    Copy link

    qodo-merge-pro bot commented Sep 2, 2024

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Error handling
    Add error handling for the Strcmp function execution

    Consider adding error handling for the Strcmp function. Currently, there's no error
    handling mechanism in place, which might lead to unexpected behavior if an error
    occurs during execution.

    pkg/sql/plan/function/list_builtIn.go [1534-1536]

     newOp: func() executeLogicOfOverload {
    -    return Strcmp
    +    return func(ivecs []*vector.Vector, result vector.FunctionResultWrapper, proc *process.Process, length int) error {
    +        err := Strcmp(ivecs, result, proc, length)
    +        if err != nil {
    +            return fmt.Errorf("error in Strcmp: %w", err)
    +        }
    +        return nil
    +    }
     },
     
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: Adding error handling to the Strcmp function enhances robustness and prevents unexpected behavior during execution, which is important for maintaining code reliability.

    8
    Performance
    ✅ Implement a more efficient string comparison function that directly returns int8
    Suggestion Impact:The suggestion to implement a custom comparison logic for the strcmp function was directly applied in the commit.

    code diff:

     func strcmp(s1, s2 string) int8 {
    -	return (int8)(strings.Compare(s1, s2))
    +	if s1 == s2 {
    +		return 0
    +	}
    +	if s1 < s2 {
    +		return -1
    +	}
    +	return 1
     }

    Consider using a more efficient comparison method for strcmp function. Instead of
    converting the result of strings.Compare to int8, you can implement a custom
    comparison logic that directly returns int8.

    pkg/sql/plan/function/func_binary.go [2205-2207]

     func strcmp(s1, s2 string) int8 {
    -    return (int8)(strings.Compare(s1, s2))
    +    if s1 == s2 {
    +        return 0
    +    }
    +    if s1 < s2 {
    +        return -1
    +    }
    +    return 1
     }
     
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: The suggestion provides a custom implementation for the strcmp function that avoids the overhead of converting the result of strings.Compare to int8, which can improve performance slightly. However, the performance gain might be minimal, so the improvement is not crucial.

    7
    Documentation
    Add a descriptive comment for the strcmp function

    Consider adding a comment to explain the purpose and behavior of the strcmp
    function, including its return values and how it compares to other string comparison
    functions in the codebase.

    pkg/sql/plan/function/list_builtIn.go [1520-1525]

    -// function `strcmp`
    +// function `strcmp` compares two strings lexicographically
    +// It returns:
    +//   -1 if s1 < s2
    +//    0 if s1 == s2
    +//    1 if s1 > s2
    +// This function is case-sensitive and uses byte-by-byte comparison.
     {
         functionId: STRCMP,
         class:      plan.Function_STRICT,
         layout:     STANDARD_FUNCTION,
         checkFn:    fixedTypeMatch,
     
    • Apply this suggestion
    Suggestion importance[1-10]: 6

    Why: The suggestion to add a comment improves code readability and maintainability by providing clarity on the function's behavior and return values, which is helpful for future developers.

    6

    @mergify mergify bot added the kind/feature label Sep 2, 2024
    @leo-livis leo-livis changed the title [Feature Request]: strcmp function #15359 [Feature Request]: strcmp function Sep 2, 2024
    @m-schen
    Copy link
    Contributor

    m-schen commented Sep 3, 2024

    需要单测,以及函数的定义不对,缺少一个select list参数,代码编译不过的。

    return opBinaryStrStrToFixed[int8](ivecs, result, proc, length, strcmp, nil)
    }

    func strcmp(s1, s2 string) int8 {
    Copy link
    Contributor

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    why use int8 instead of int32? the function signature said int32.

    args: []types.T{types.T_varchar, types.T_varchar},
    retType: func(parameters []types.Type) types.Type {
    return types.T_int32.ToType()
    },
    Copy link
    Contributor

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    int32 here.

    Copy link
    Contributor

    @fengttt fengttt left a comment

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    Please fix type mismatch.

    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    6 participants