Skip to content

Conversation

XuPeng-SH
Copy link
Contributor

@XuPeng-SH XuPeng-SH commented Jul 12, 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 #22151

What this PR does / why we need it:

Revert 22092 changes in 2.2-dev due to oom


PR Type

Bug fix


Description


Changes diagram

flowchart LR
  A["Import types package"] --> B["Check column types"]
  B --> C["Block vector column DML"]
  C --> D["Prevent OOM issues"]
Loading

Changes walkthrough 📝

Relevant files
Bug fix
dml_context.go
Add vector column DML restriction                                               

pkg/sql/plan/dml_context.go

  • Add import for types package
  • Add validation loop to check column types
  • Return error for vector/array columns in DML operations
  • +7/-0     

    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

    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

    Error Message

    The error message "vector column" is inconsistent with the check logic which uses IsArrayRelate() method. This could confuse users since the method checks for array-related types but reports "vector column" error.

    	return moerr.NewUnsupportedDML(ctx.GetContext(), "vector column")
    }
    Logic Placement

    The vector/array column check is placed after foreign key validation but before cluster table checks. Consider if this is the optimal placement for performance and logical flow.

    for _, col := range tableDef.Cols {
    	if types.T(col.Typ.Id).IsArrayRelate() {
    		return moerr.NewUnsupportedDML(ctx.GetContext(), "vector column")
    	}
    }

    @matrix-meow matrix-meow added the size/XS Denotes a PR that changes [1, 9] lines label Jul 12, 2025
    @mergify mergify bot added the kind/bug Something isn't working label Jul 12, 2025
    Copy link

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    General
    Fix misleading error message

    The error message "vector column" is misleading since the check uses
    IsArrayRelate() which covers both vector and array types. Update the error
    message to accurately reflect what types are being rejected.

    pkg/sql/plan/dml_context.go [233-237]

     for _, col := range tableDef.Cols {
     	if types.T(col.Typ.Id).IsArrayRelate() {
    -		return moerr.NewUnsupportedDML(ctx.GetContext(), "vector column")
    +		return moerr.NewUnsupportedDML(ctx.GetContext(), "vector/array column")
     	}
     }
    • Apply / Chat
    Suggestion importance[1-10]: 6

    __

    Why: The suggestion correctly identifies that the error message "vector column" is misleading because the check IsArrayRelate() covers both vector and array types, so the proposed change improves clarity.

    Low
    • More

    aunjgr added a commit to aunjgr/matrixone that referenced this pull request Jul 25, 2025
    aunjgr added a commit to aunjgr/matrixone that referenced this pull request Aug 6, 2025
    aunjgr added a commit to aunjgr/matrixone that referenced this pull request Aug 29, 2025
    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/XS Denotes a PR that changes [1, 9] lines
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    3 participants