Skip to content

Conversation

gouhongshen
Copy link
Contributor

@gouhongshen gouhongshen commented Jul 16, 2025

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

What this PR does / why we need it:

Fix Inconsistent SQL Query Results for the MO System Version.

Three ways to query the version:

  1. New session connection:
    Server version: 8.0.30-MatrixOne-v2.2.1 MatrixOne

  2. Show variables like "%version%":

+-------------------------+-------------------------+
| Variable_name           | Value                   |
+-------------------------+-------------------------+
| version                 | 8.0.30-MatrixOne-v2.2.1 |
+-------------------------+-------------------------+
  1. select version();
+-------------------------+
| version()               |
+-------------------------+
| 8.0.30-MatrixOne-v2.2.1 |
+-------------------------+

They should all be the system version for the MO, not the metadata version.


PR Type

Bug fix


Description

  • Fix inconsistent SQL query results for MO system version

  • Update version() function to use system version instead of metadata version

  • Ensure all three version query methods return consistent results


Changes diagram

flowchart LR
  A["System Variables"] --> B["serverVersion.Load()"]
  C["version() Function"] --> D["GetResolveVariableFunc()"]
  B --> E["Consistent Version Output"]
  D --> E
Loading

Changes walkthrough 📝

Relevant files
Bug fix
variables.go
Update system variable version source                                       

pkg/frontend/variables.go

  • Replace CNServiceConfig.MoVersion with serverVersion.Load().(string)
  • Update system variable version to use runtime server version
  • +1/-1     
    func_unary.go
    Refactor version() function implementation                             

    pkg/sql/plan/function/func_unary.go

  • Refactor Version() function to use variable resolver
  • Replace direct session version access with GetResolveVariableFunc()
  • Add proper error handling for version retrieval
  • +22/-2   

    Need help?
  • Type /help how to ... in the comments thread for any questions about Qodo Merge usage.
  • Check out the documentation for more information.
  • Copy link

    You are nearing your monthly Qodo Merge usage quota. For more information, please visit here.

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Type Assertion

    The code performs an unsafe type assertion versionAny.(string) without checking if the returned value is actually a string. This could cause a panic if the variable resolver returns a different type.

    versionStr = versionAny.(string)
    Type Assertion

    Similar unsafe type assertion serverVersion.Load().(string) without type checking. If serverVersion contains a non-string value, this will panic.

    	sysVarsMp["version"] = CNServiceConfig.ServerVersionPrefix + serverVersion.Load().(string)
    }

    Copy link

    qodo-merge-pro bot commented Jul 16, 2025

    You are nearing your monthly Qodo Merge usage quota. For more information, please visit here.

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    Possible issue
    Add safe type assertion check

    The type assertion versionAny.(string) can panic if the interface doesn't
    contain a string. Add a safe type assertion with an ok check to handle potential
    type mismatches gracefully.

    pkg/sql/plan/function/func_unary.go [1602]

    -versionStr = versionAny.(string)
    +if versionStr, ok := versionAny.(string); !ok {
    +    return fmt.Errorf("version variable is not a string")
    +}
    • Apply / Chat
    Suggestion importance[1-10]: 8

    __

    Why: The suggestion correctly identifies that the type assertion versionAny.(string) can cause a panic if the resolved variable is not a string, and proposes a safe check which is a best practice to improve code robustness.

    Medium
    • Update

    @mergify mergify bot merged commit af55a18 into matrixorigin:main Jul 17, 2025
    19 checks passed
    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 Review effort 2/5 size/S Denotes a PR that changes [10,99] lines
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    5 participants