Skip to content

Fix Inconsistent SQL Query Results for the MO System Version. #22176

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

Merged
merged 5 commits into from
Jul 17, 2025

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