Skip to content

Conversation

aunjgr
Copy link
Contributor

@aunjgr aunjgr commented Jun 23, 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/5330

What this PR does / why we need it:

Previously runtime filter on non-PK column only take effect in single-concurrency queries (i.e., tpcc). This PR makes it work as expected.


PR Type

Enhancement


Description

  • Enable runtime filters on non-primary key columns

  • Refactor filter operator to separate runtime and regular filters

  • Update protobuf schema to support runtime filter expressions

  • Fix full-text index deletion with runtime filter integration


Changes walkthrough 📝

Relevant files
Enhancement
7 files
build_dml_util.go
Enable runtime filters for full-text index deletion           
+37/-34 
types.go
Separate runtime and regular filter expressions                   
+8/-6     
filter.go
Update filter preparation for runtime expressions               
+8/-3     
remoterun.go
Add runtime filter serialization support                                 
+4/-2     
operator.go
Update operator duplication for runtime filters                   
+3/-2     
scope.go
Simplify runtime filter handling in scope                               
+1/-4     
pipeline.proto
Add runtime_filters field to pipeline instruction               
+9/-8     
Tests
1 files
filter_test.go
Update test cases for new filter structure                             
+2/-2     
Formatting
1 files
types_mock_test.go
Fix whitespace formatting                                                               
+1/-1     
Additional files
1 files
pipeline.pb.go +462/-399

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: 4 🔵🔵🔵🔵⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Breaking Change

    The Filter struct has been significantly refactored, changing from a single expression field E to separate FilterExprs and RuntimeFilterExprs fields. This is a breaking change that affects the public API and requires careful validation that all existing code paths have been updated correctly.

    type Filter struct {
    	ctr                container
    	FilterExprs        *plan.Expr
    	RuntimeFilterExprs []*plan.Expr
    	IsEnd              bool
    
    	vm.OperatorBase
    }
    Method Signature

    The method SetRuntimeExpr has been renamed to SetRuntimeFilterExprs and its implementation has been simplified to just assign the expressions without creating executors. This change in behavior needs validation to ensure runtime filter functionality still works correctly.

    func (filter *Filter) SetRuntimeFilterExprs(proc *process.Process, exes []*plan.Expr) (err error) {
    	filter.RuntimeFilterExprs = exes
    	return
    }
    Protocol Breaking

    The protobuf field numbering has been changed, with new runtime_filters field added at position 23 and existing fields renumbered. This is a breaking change in the protocol that requires careful coordination with all components using this schema.

    ExternalScan external_scan = 15;
    Insert insert = 16;
    OnDuplicateKey on_duplicate_key = 17;
    PreInsert pre_insert               = 18;
    PreInsertUnique pre_insert_unique  = 19;
    repeated plan.OrderBySpec order_by = 20;
    repeated plan.Expr    project_list = 21;
    plan.Expr    filter = 22;
    repeated plan.Expr runtime_filters = 23;
    

    Copy link

    qodo-merge-pro bot commented Jun 23, 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 error handling for runtime executors
    Suggestion Impact:The suggestion was implemented - error handling was added for both the RuntimeFilterExprs case (lines 22-24) and also for the FilterExprs case (lines 15-17). The commit added the exact error checking pattern suggested.

    code diff:

    +		if err != nil {
    +			return
    +		}
     	}
     
     	if len(filter.RuntimeFilterExprs) > 0 {
     		filter.ctr.runtimeExecutors, err = colexec.NewExpressionExecutorsFromPlanExpressions(proc, filter.RuntimeFilterExprs)
    +		if err != nil {
    +			return
    +		}

    The error returned from NewExpressionExecutorsFromPlanExpressions is not being
    checked. If this function fails, the error should be handled to prevent
    potential runtime issues.

    pkg/sql/colexec/filter/filter.go [52-54]

     if len(filter.RuntimeFilterExprs) > 0 {
     	filter.ctr.runtimeExecutors, err = colexec.NewExpressionExecutorsFromPlanExpressions(proc, filter.RuntimeFilterExprs)
    +	if err != nil {
    +		return err
    +	}
     }

    [Suggestion processed]

    Suggestion importance[1-10]: 8

    __

    Why: The suggestion correctly identifies that the error returned by colexec.NewExpressionExecutorsFromPlanExpressions is not handled. This is a bug, as an error during executor creation could be overwritten or ignored, leading to potential panics or incorrect behavior later in the execution.

    Medium
    Add validation for runtime filter assignment

    Direct assignment to RuntimeFilterExprs bypasses validation and error handling
    that was previously performed by SetRuntimeExpr. This could lead to runtime
    issues if nonPkFilters contains invalid expressions or if the assignment fails
    silently.

    pkg/sql/compile/scope.go [723]

    -arg.RuntimeFilterExprs = nonPkFilters
    +if err := arg.SetRuntimeFilterExprs(nonPkFilters); err != nil {
    +    return nil, err
    +}
    • Apply / Chat
    Suggestion importance[1-10]: 8

    __

    Why: The suggestion correctly identifies that the PR removes error handling by replacing the arg.SetRuntimeExpr function call with a direct assignment. This change bypasses potential validation, which could introduce runtime bugs if nonPkFilters contains invalid data. Restoring the error handling is a crucial improvement for code robustness.

    Medium
    • Update

    @mergify mergify bot merged commit 52844ed into matrixorigin:main Jun 23, 2025
    19 checks passed
    @aunjgr aunjgr deleted the fix_rf branch June 23, 2025 10:22
    aunjgr added a commit to aunjgr/matrixone that referenced this pull request Jul 7, 2025
    Previously runtime filter on non-PK column only take effect in single-concurrency queries (i.e., tpcc). This PR makes it work as expected.
    
    Approved by: @ouyuanning
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    kind/enhancement Review effort 4/5 size/M Denotes a PR that changes [100,499] lines
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    4 participants