Skip to content

Conversation

aunjgr
Copy link
Contributor

@aunjgr aunjgr commented Jul 3, 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 #22020

What this PR does / why we need it:

refactored DML has better support for REPLACE INTO SELECT


PR Type

Enhancement


Description

  • Remove vector column restriction from DML operations

  • Enable new DML plan for tables with vector columns


Changes diagram

flowchart LR
  A["DML Context"] --> B["Table Resolution"]
  B --> C["Vector Column Check"]
  C --> D["Remove Restriction"]
  D --> E["Enable New DML Plan"]
Loading

Changes walkthrough 📝

Relevant files
Enhancement
dml_context.go
Remove vector column DML restriction                                         

pkg/sql/plan/dml_context.go

  • Remove import of types package
  • Delete vector column validation loop in ResolveSingleTable
  • Remove error return for vector column unsupported DML
  • +0/-7     

    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

    qodo-merge-pro bot commented Jul 3, 2025

    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

    Missing Validation

    The removal of vector column validation may enable DML operations on vector columns without proper testing or verification that the new DML plan actually supports these operations correctly. This could lead to runtime errors or data corruption if the underlying implementation is not ready.

    if checkFK && (len(tableDef.Fkeys) > 0 || len(tableDef.RefChildTbls) > 0) {
    	return moerr.NewUnsupportedDML(ctx.GetContext(), "foreign key constraint")
    }
    
    isClusterTable := util.TableIsClusterTable(tableDef.GetTableType())
    accountId, err := ctx.GetAccountId()
    if err != nil {
    	return err

    Copy link

    qodo-merge-pro bot commented Jul 3, 2025

    PR Code Suggestions ✨

    No code suggestions found for the PR.

    @matrix-meow matrix-meow added the size/XS Denotes a PR that changes [1, 9] lines label Jul 3, 2025
    @mergify mergify bot added the kind/feature label Jul 3, 2025
    @mergify mergify bot merged commit b752e35 into matrixorigin:main Jul 3, 2025
    18 of 19 checks passed
    @aunjgr aunjgr deleted the replace_pk branch July 3, 2025 09:11
    Cabbage74 pushed a commit to Cabbage74/matrixone that referenced this pull request Jul 7, 2025
    refactored DML has better support for REPLACE INTO SELECT
    
    Approved by: @ouyuanning
    aunjgr added a commit to aunjgr/matrixone that referenced this pull request Jul 7, 2025
    refactored DML has better support for REPLACE INTO SELECT
    
    Approved by: @ouyuanning
    XuPeng-SH pushed a commit to XuPeng-SH/matrixone that referenced this pull request Jul 11, 2025
    refactored DML has better support for REPLACE INTO SELECT
    
    Approved by: @ouyuanning
    XuPeng-SH added a commit that referenced this pull request Jul 12, 2025
    ### **User description**
    ## What type of PR is this?
    
    - [ ] API-change
    - [x] 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**
    - Revert changes from PR #22092 causing OOM issues
    
    - Add vector column DML operation restriction
    
    - Import types package for array type checking
    
    
    ___
    
    ### **Changes diagram**
    
    ```mermaid
    flowchart LR
      A["Import types package"] --> B["Check column types"]
      B --> C["Block vector column DML"]
      C --> D["Prevent OOM issues"]
    ```
    
    
    ___
    
    
    
    ### **Changes walkthrough** 📝
    <table><thead><tr><th></th><th align="left">Relevant
    files</th></tr></thead><tbody><tr><td><strong>Bug
    fix</strong></td><td><table>
    <tr>
      <td>
        <details>
    <summary><strong>dml_context.go</strong><dd><code>Add vector column DML
    restriction</code>&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp;
    &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp;
    &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; </dd></summary>
    <hr>
    
    pkg/sql/plan/dml_context.go
    
    <li>Add import for <code>types</code> package<br> <li> Add validation
    loop to check column types<br> <li> Return error for vector/array
    columns in DML operations
    
    
    </details>
    
    
      </td>
    <td><a
    href="https://github.com/matrixorigin/matrixone/pull/22157/files#diff-6e952c10166b807c3a9ef06cde3d549ebbee6e29eb2ffd1aeceb9eab8590fd3e">+7/-0</a>&nbsp;
    &nbsp; &nbsp; </td>
    
    </tr>
    </table></td></tr></tr></tbody></table>
    
    ___
    
    > <details> <summary> Need help?</summary><li>Type <code>/help how to
    ...</code> in the comments thread for any questions about Qodo Merge
    usage.</li><li>Check out the <a
    href="https://qodo-merge-docs.qodo.ai/usage-guide/">documentation</a>
    for more information.</li></details>
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    kind/feature 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