matrixone icon indicating copy to clipboard operation
matrixone copied to clipboard
verified publisher icon matrixorigin

Add WEEK() function mode parameter support and configurable CTE max recursion depth

Open flypiggyyoyoyo opened this issue 4 months ago • 3 comments

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 #23265

What this PR does / why we need it:

This PR adds two enhancements:

  1. WEEK() function mode parameter support: Adds optional second parameter mode (0-7) for WEEK(date, mode) and WEEK(datetime, mode) to control week numbering behavior, making it compatible with MySQL's WEEK() function specification.
  2. Configurable CTE max recursion depth: Allows users to configure CTE recursion limit via SET cte_max_recursion_depth = N instead of using a hardcoded value. This enables deeper recursion for complex hierarchical queries when needed.

PR Type

Enhancement, Bug fix

Description

  • Add optional mode parameter (0-7) to WEEK() function for date/datetime types

    • Supports MySQL-compatible week numbering modes
    • Handles mode validation and clamping to valid range

  • Make CTE max recursion depth configurable via SET cte_max_recursion_depth variable

    • Replaces hardcoded recursion limit with dynamic resolution
    • Enables deeper recursion for complex hierarchical queries

  • Add comprehensive test coverage for WEEK() mode parameter and CTE recursion depth

Should we remain consistent with MySQL？

  1. Prerequisites (Create Database/Table)

MySQL:

CREATE DATABASE IF NOT EXISTS test;
USE test;

MO:

CREATE DATABASE IF NOT EXISTS test;
USE test;
  1. WEEK Function Tests

Normal Cases (MySQL and MO results are identical ✅)

SQL MySQL MO
SELECT week('2023-01-01', 0); 1 1
SELECT week('2023-01-01', 1); 0 0
SELECT week('2023-01-02', 0); 1 1
SELECT week('2023-01-02', 1); 1 1
SELECT week('2023-12-31', 0); 53 53
SELECT week('2023-12-31', 1); 52 52
SELECT week('2023-01-01', -1); 52 52
SELECT week('2023-01-01', 8); 1 1
SELECT week('2023-01-01', 100); 1 1
SELECT week(null, 0); NULL NULL
SELECT week('2023-01-01', null); 1 1

Invalid Cases (MySQL and MO behavior differs ⚠️)

SQL MySQL MO Note
SELECT week('0000-00-00', 0); NULL Error: invalid argument parsedate MO is stricter, rejects invalid date
SELECT week('2023-02-30', 0); NULL Error: invalid argument parsedate MO is stricter, rejects non-existent date
SELECT week('abc', 0); NULL Error: invalid argument parsedate MO is stricter, rejects invalid format
SELECT week('', 0); NULL NULL Identical
  1. CTE Recursion Depth Tests

Prerequisites

  CREATE DATABASE IF NOT EXISTS test;
  USE test;
  DROP TABLE IF EXISTS t_cte;
  CREATE TABLE t_cte(a int);
  INSERT INTO t_cte VALUES (1);

Normal Cases (MySQL and MO results are identical ✅)

SQL MySQL MO
SET cte_max_recursion_depth = 200;WITH RECURSIVE c AS (SELECT a FROM t_cte UNION ALL SELECT a+1 FROM c WHERE a < 150) SELECT count(*) FROM c; 150 150
SET cte_max_recursion_depth = 50;WITH RECURSIVE c AS (SELECT a FROM t_cte UNION ALL SELECT a+1 FROM c WHERE a < 100) SELECT count(*) FROM c; Error: Recursive query aborted after 51 iterations Error: recursive level out of range

Invalid Cases (MySQL and MO behavior differs ⚠️)

SQL MySQL MO Note
SET cte_max_recursion_depth = -1; Success, silently converts to 0 Error: convert to the system variable int type failed MO is stricter, rejects negative values
SET cte_max_recursion_depth = -100; Success, silently converts to 0 Error: convert to the system variable int type failed MO is stricter, rejects negative values
SET cte_max_recursion_depth = 0; Success, value is 0 Success, value is 0 Identical ✅
SET cte_max_recursion_depth = 4294967295; Success, value is 4294967295 Success Identical ✅
SET cte_max_recursion_depth = 4294967296; Success, silently truncates to 4294967295 Error: convert to the system variable int type failed MO is stricter, rejects overflow
SET cte_max_recursion_depth = 'abc'; Error: Incorrect argument type Error: convert to the system variable int type failed Both error ✅

Diagram Walkthrough

flowchart LR
  A["WEEK Function"] -->|"Add mode parameter"| B["DateToWeek/DatetimeToWeek"]
  B -->|"Support modes 0-7"| C["Week calculation"]
  D["CTE Recursion"] -->|"Read variable"| E["cte_max_recursion_depth"]
  E -->|"Dynamic limit"| F["MergeCTE execution"]
  G["Test Cases"] -->|"Validate"| B
  G -->|"Validate"| F

File Walkthrough
Relevant files
Enhancement
func_unary.go
Add mode parameter support to WEEK functions                         

pkg/sql/plan/function/func_unary.go

  • Refactored DateToWeek() to support optional mode parameter with
    validation
  • Refactored DatetimeToWeek() to support optional mode parameter with
    validation
  • Replaced simple WeekOfYear2() calls with Week(mode) method calls
  • Added manual loop-based implementation to handle mode parameter
    extraction and null handling
 +72/-6   
list_builtIn.go
Register WEEK function overloads with mode parameter         

pkg/sql/plan/function/list_builtIn.go

  • Added two new function overloads for WEEK() with mode parameter
  • Overload 2: WEEK(date, int64) returning uint8
  • Overload 3: WEEK(datetime, int64) returning uint8
  • Maintains backward compatibility with existing single-parameter
    overloads
 +20/-0   
mergecte.go
Make CTE recursion limit configurable via variable             

pkg/sql/colexec/mergecte/mergecte.go

  • Replaced hardcoded moDefaultRecursionMax with dynamic variable
    resolution
  • Added proc.GetResolveVariableFunc() call to fetch
    cte_max_recursion_depth variable
  • Implemented fallback to default value if variable resolution fails
  • Added type checking and validation for resolved variable value
 +9/-1     
Tests
func_date.sql
Add WEEK function mode parameter test cases                           

test/distributed/cases/function/func_date.sql

  • Added test cases for WEEK() function with mode parameter
  • Tests cover modes 0, 1, 2, 3 with both date and datetime types
  • Tests include edge cases like year boundaries (2023-01-01, 2023-12-31)
  • Validates behavior with NULL values
 +11/-0   
func_date.result
Update WEEK function test results with mode parameter       

test/distributed/cases/function/func_date.result

  • Updated expected results for WEEK() function tests with various modes
  • Fixed whitespace formatting in existing test results (tabs to spaces)
  • Added comprehensive output for WEEK() mode parameter test cases
  • Validates correct week numbers for different modes and dates
 +109/-20
recursive_cte.sql
Add CTE recursion depth configuration test cases                 

test/distributed/cases/recursive_cte/recursive_cte.sql

  • Added test cases for configurable cte_max_recursion_depth variable
  • Tests SET command with different recursion depth values (200, 50, 100)
  • Validates that queries succeed/fail based on configured depth limits
  • Tests cleanup with table drop statements
 +11/-0   
recursive_cte.result
Update recursive CTE test results with configurable depth

test/distributed/cases/recursive_cte/recursive_cte.result

  • Updated expected results for recursive CTE tests with configurable
    depth
  • Changed previous error result to successful query output for depth=200
    case
  • Added expected output for CTE recursion depth variable tests
  • Validates that depth limit of 50 correctly rejects queries exceeding
    it
  • Fixed column name casing in Person table test results
 +609/-3 

flypiggyyoyoyo avatar Dec 12 '25 03:12 flypiggyyoyoyo

CLA assistant check
All committers have signed the CLA.

CLAassistant avatar Dec 12 '25 03:12 CLAassistant

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

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
🟢
No security concerns identified No security vulnerabilities detected by AI analysis. Human verification advised for critical code.
Ticket Compliance
⚪
🎫 No ticket provided
  • [ ] Create ticket/issue <!-- /create_ticket --create_ticket=true -->
Codebase Duplication Compliance
⚪
Codebase context is not defined

Follow the guide to enable codebase context checks.

Custom Compliance
🟢
Generic: Meaningful Naming and Self-Documenting Code

Objective: Ensure all identifiers clearly express their purpose and intent, making code
self-documenting

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Secure Error Handling

Objective: To prevent the leakage of sensitive system information through error messages while
providing sufficient detail for internal debugging.

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Secure Logging Practices

Objective: To ensure logs are useful for debugging and auditing without exposing sensitive
information like PII, PHI, or cardholder data.

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Security-First Input Validation and Data Handling

Objective: Ensure all data inputs are validated, sanitized, and handled securely to prevent
vulnerabilities

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

⚪
Generic: Comprehensive Audit Trails

Objective: To create a detailed and reliable record of critical system actions for security analysis
and compliance.

Status:
Missing Audit: The new logic that reads and enforces cte_max_recursion_depth performs a critical control
decision without any explicit auditing/logging of the configured value or enforcement
outcome in the added lines.

Referred Code 
maxRecursion := moDefaultRecursionMax
if resolveFunc := proc.GetResolveVariableFunc(); resolveFunc != nil {
	if val, err := resolveFunc("cte_max_recursion_depth", true, false); err == nil {
		if v, ok := val.(int64); ok {
			maxRecursion = int(v)
		}
	}
}
if mergeCTE.ctr.recursiveLevel > maxRecursion {
	result.Status = vm.ExecStop
	return result, moerr.NewCheckRecursiveLevel(proc.Ctx)
}

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Robust Error Handling and Edge Case Management

Objective: Ensure comprehensive error handling that provides meaningful context and graceful
degradation

Status:
Silent Fallback: When resolving cte_max_recursion_depth, errors or non-int64 values are silently ignored
and default is used without recording context, which may hinder debugging and hides
misconfiguration.

Referred Code 
maxRecursion := moDefaultRecursionMax
if resolveFunc := proc.GetResolveVariableFunc(); resolveFunc != nil {
	if val, err := resolveFunc("cte_max_recursion_depth", true, false); err == nil {
		if v, ok := val.(int64); ok {
			maxRecursion = int(v)
		}
	}
}
if mergeCTE.ctr.recursiveLevel > maxRecursion {
	result.Status = vm.ExecStop

Learn more about managing compliance generic rules or creating your own custom rules

  • [ ] Update <!-- /compliance --update_compliance=true -->
Compliance status legend 🟢 - Fully Compliant
🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label

qodo-code-review[bot] avatar Dec 12 '25 03:12 qodo-code-review[bot]

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
Use a more flexible overload check

Replace the checkFn for the WEEK function from fixedTypeMatch to generalCheck to
correctly support its new overloads with a variable number of arguments.

pkg/sql/plan/function/list_builtIn.go [7254-7283]

 {
-    overloadId: 1,
-    args:       []types.T{types.T_datetime},
-    retType: func(parameters []types.Type) types.Type {
-        return types.T_uint8.ToType()
-    },
-    newOp: func() executeLogicOfOverload {
-        return DatetimeToWeek
-    },
-},
-{
-    overloadId: 2,
-    args:       []types.T{types.T_date, types.T_int64},
-    retType: func(parameters []types.Type) types.Type {
-        return types.T_uint8.ToType()
-    },
-    newOp: func() executeLogicOfOverload {
-        return DateToWeek
-    },
-},
-{
-    overloadId: 3,
-    args:       []types.T{types.T_datetime, types.T_int64},
-    retType: func(parameters []types.Type) types.Type {
-        return types.T_uint8.ToType()
-    },
-    newOp: func() executeLogicOfOverload {
-        return DatetimeToWeek
+    functionId: WEEK,
+    class:      plan.Function_STRICT,
+    layout:     STANDARD_FUNCTION,
+    checkFn:    generalCheck,
+
+    Overloads: []overload{
+        {
+            overloadId: 0,
+            args:       []types.T{types.T_date},
+            retType: func(parameters []types.Type) types.Type {
+                return types.T_uint8.ToType()
+            },
+            newOp: func() executeLogicOfOverload {
+                return DateToWeek
+            },
+        },
+        {
+            overloadId: 1,
+            args:       []types.T{types.T_datetime},
+            retType: func(parameters []types.Type) types.Type {
+                return types.T_uint8.ToType()
+            },
+            newOp: func() executeLogicOfOverload {
+                return DatetimeToWeek
+            },
+        },
+        {
+            overloadId: 2,
+            args:       []types.T{types.T_date, types.T_int64},
+            retType: func(parameters []types.Type) types.Type {
+                return types.T_uint8.ToType()
+            },
+            newOp: func() executeLogicOfOverload {
+                return DateToWeek
+            },
+        },
+        {
+            overloadId: 3,
+            args:       []types.T{types.T_datetime, types.T_int64},
+            retType: func(parameters []types.Type) types.Type {
+                return types.T_uint8.ToType()
+            },
+            newOp: func() executeLogicOfOverload {
+                return DatetimeToWeek
+            },
+        },
     },
 },

[To ensure code accuracy, apply this suggestion manually]

Suggestion importance[1-10]: 8

__

Why: The suggestion correctly identifies that fixedTypeMatch is too restrictive for the overloaded WEEK function and proposes using generalCheck, which is essential for the function to work correctly with its different argument variations.

Medium
Validate that mode argument is constant

Add a validation check to ensure the mode argument for the WEEK function is a
constant value, returning an error if it is not.

pkg/sql/plan/function/func_unary.go [3037-3046]

 // Get mode (default 0 if not provided)
 mode := 0
-if len(ivecs) > 1 && !ivecs[1].IsConstNull() {
-    mode = int(vector.MustFixedColWithTypeCheck[int64](ivecs[1])[0])
-    if mode < 0 {
-        mode = 0
-    } else if mode > 7 {
-        mode = 7
+if len(ivecs) > 1 {
+    if !ivecs[1].IsConst() {
+        return moerr.NewInvalidInput(proc.Ctx, "week function mode argument must be a constant")
+    }
+    if !ivecs[1].IsConstNull() {
+        mode = int(vector.MustFixedColWithTypeCheck[int64](ivecs[1])[0])
+        if mode < 0 {
+            mode = 0
+        } else if mode > 7 {
+            mode = 7
+        }
     }
 }
  • [ ] Apply / Chat <!-- /improve --apply_suggestion=1 -->
Suggestion importance[1-10]: 7

__

Why: The suggestion correctly identifies that the mode argument should be a constant, and adds a necessary validation check to prevent incorrect behavior if a column is passed as mode.

Medium
High-level
Resolve CTE recursion depth once

To improve performance, fetch the cte_max_recursion_depth variable once in the
Prepare method or at the start of Call, instead of on every recursion level.

Examples:

pkg/sql/colexec/mergecte/mergecte.go [124-132] 
					maxRecursion := moDefaultRecursionMax
					if resolveFunc := proc.GetResolveVariableFunc(); resolveFunc != nil {
						if val, err := resolveFunc("cte_max_recursion_depth", true, false); err == nil {
							if v, ok := val.(int64); ok {
								maxRecursion = int(v)
							}
						}
					}
					if mergeCTE.ctr.recursiveLevel > maxRecursion {

Solution Walkthrough:

Before:

func (mergeCTE *MergeCTE) Call(proc *process.Process) (vm.CallResult, error) {
  // ...
  switch ctr.status {
  // ...
  case sendRecursive:
    // ...
    if result.Batch.Last() {
      mergeCTE.ctr.recursiveLevel++
      maxRecursion := moDefaultRecursionMax
      // Variable is resolved on every recursive step
      if resolveFunc := proc.GetResolveVariableFunc(); resolveFunc != nil {
        if val, err := resolveFunc("cte_max_recursion_depth", true, false); err == nil {
          // ...
        }
      }
      if mergeCTE.ctr.recursiveLevel > maxRecursion {
        return result, moerr.NewCheckRecursiveLevel(proc.Ctx)
      }
      // ...
    }
  }
  return result, nil
}

After:

func (mergeCTE *MergeCTE) Prepare(proc *process.Process) error {
  // ...
  // Resolve variable once during preparation
  maxRecursion := moDefaultRecursionMax
  if resolveFunc := proc.GetResolveVariableFunc(); resolveFunc != nil {
    if val, err := resolveFunc("cte_max_recursion_depth", true, false); err == nil {
      if v, ok := val.(int64); ok {
        maxRecursion = int(v)
      }
    }
  }
  mergeCTE.maxRecursion = maxRecursion // store it in the struct
  return nil
}

func (mergeCTE *MergeCTE) Call(proc *process.Process) (vm.CallResult, error) {
  // ...
  if result.Batch.Last() {
    mergeCTE.ctr.recursiveLevel++
    // Use the pre-resolved value
    if mergeCTE.ctr.recursiveLevel > mergeCTE.maxRecursion {
      return result, moerr.NewCheckRecursiveLevel(proc.Ctx)
    }
  }
  // ...
}
Suggestion importance[1-10]: 7

__

Why: The suggestion correctly identifies a performance issue where cte_max_recursion_depth is resolved in a loop, and moving the resolution to the Prepare method is a significant optimization for deep recursions.

Medium
  • [ ] Update <!-- /improve_multi --more_suggestions=true -->

qodo-code-review[bot] avatar Dec 12 '25 03:12 qodo-code-review[bot]

Merge Queue Status

✅ The pull request has been merged at 57050c55807edfe8c813c2b1eaa011b1fcaace58

This pull request spent 7 seconds in the queue, with no time running CI. The checks were run in-place.

Required conditions to merge
  • #approved-reviews-by >= 1 [🛡 GitHub branch protection]
    • [X] #23268
  • #changes-requested-reviews-by = 0 [🛡 GitHub branch protection]
    • [X] #23268
  • #review-threads-unresolved = 0 [🛡 GitHub branch protection]
    • [X] #23268
  • branch-protection-review-decision = APPROVED [🛡 GitHub branch protection]
    • [X] #23268
  • [X] any of [🛡 GitHub branch protection]:
    • [X] check-success = Matrixone Compose CI / multi cn e2e bvt test docker compose(PESSIMISTIC)
    • [ ] check-neutral = Matrixone Compose CI / multi cn e2e bvt test docker compose(PESSIMISTIC)
    • [ ] check-skipped = Matrixone Compose CI / multi cn e2e bvt test docker compose(PESSIMISTIC)
  • [X] any of [🛡 GitHub branch protection]:
    • [X] check-success = Matrixone Standlone CI / Multi-CN e2e BVT Test on Linux/x64(LAUNCH, PROXY)
    • [ ] check-neutral = Matrixone Standlone CI / Multi-CN e2e BVT Test on Linux/x64(LAUNCH, PROXY)
    • [ ] check-skipped = Matrixone Standlone CI / Multi-CN e2e BVT Test on Linux/x64(LAUNCH, PROXY)
  • [X] any of [🛡 GitHub branch protection]:
    • [X] check-success = Matrixone Standlone CI / e2e BVT Test on Linux/x64(LAUNCH, PESSIMISTIC)
    • [ ] check-neutral = Matrixone Standlone CI / e2e BVT Test on Linux/x64(LAUNCH, PESSIMISTIC)
    • [ ] check-skipped = Matrixone Standlone CI / e2e BVT Test on Linux/x64(LAUNCH, PESSIMISTIC)
  • [X] any of [🛡 GitHub branch protection]:
    • [X] check-success = Matrixone CI / SCA Test on Ubuntu/x86
    • [ ] check-neutral = Matrixone CI / SCA Test on Ubuntu/x86
    • [ ] check-skipped = Matrixone CI / SCA Test on Ubuntu/x86
  • [X] any of [🛡 GitHub branch protection]:
    • [X] check-success = Matrixone CI / UT Test on Ubuntu/x86
    • [ ] check-neutral = Matrixone CI / UT Test on Ubuntu/x86
    • [ ] check-skipped = Matrixone CI / UT Test on Ubuntu/x86
  • [X] any of [🛡 GitHub branch protection]:
    • [X] check-success = Matrixone Compose CI / multi cn e2e bvt test docker compose(Optimistic/PUSH)
    • [ ] check-neutral = Matrixone Compose CI / multi cn e2e bvt test docker compose(Optimistic/PUSH)
    • [ ] check-skipped = Matrixone Compose CI / multi cn e2e bvt test docker compose(Optimistic/PUSH)
  • [X] any of [🛡 GitHub branch protection]:
    • [X] check-success = Matrixone Standlone CI / e2e BVT Test on Linux/x64(LAUNCH,Optimistic)
    • [ ] check-neutral = Matrixone Standlone CI / e2e BVT Test on Linux/x64(LAUNCH,Optimistic)
    • [ ] check-skipped = Matrixone Standlone CI / e2e BVT Test on Linux/x64(LAUNCH,Optimistic)
  • [X] any of [🛡 GitHub branch protection]:
    • [X] check-success = Matrixone Upgrade CI / Compatibility Test With Target on Linux/x64(LAUNCH)
    • [ ] check-neutral = Matrixone Upgrade CI / Compatibility Test With Target on Linux/x64(LAUNCH)
    • [ ] check-skipped = Matrixone Upgrade CI / Compatibility Test With Target on Linux/x64(LAUNCH)
  • [X] any of [🛡 GitHub branch protection]:
    • [X] check-skipped = Matrixone Utils CI / Coverage
    • [ ] check-neutral = Matrixone Utils CI / Coverage
    • [ ] check-success = Matrixone Utils CI / Coverage

mergify[bot] avatar Dec 17 '25 13:12 mergify[bot]