V3.0.0 5e4210029 hotfix 20260605#24857
Open
LeftHandCold wants to merge 399 commits into
Open
Conversation
Fix sdk bugs Approved by: @LeftHandCold
fix load for partition table Approved by: @ouyuanning
…xorigin#22703) set AP query for tables with large row size and vector indexes Approved by: @ouyuanning, @aunjgr
…xorigin#22710) ### Problem Negative values in DECIMAL64/DECIMAL128 columns were incorrectly converted to huge positive numbers in statistics (e.g., `-123.45` → `18446744073514074000`), causing `min > max` errors and suboptimal query plans. ### Root Cause Direct `float64()` cast on decoded decimals treats two's complement representation as positive numbers. ### Solution - Use `Decimal64ToFloat64()` and `Decimal128ToFloat64()` with proper scale handling - Add decimal support in `getMinMaxValueByFloat64()` for ShuffleRange ### Changes - `pkg/sql/plan/stats.go` - Fixed `UpdateStatsInfo()` - `pkg/vm/engine/disttae/stats.go` - Added decimal support - Added 11 unit tests covering all scenarios Approved by: @ouyuanning, @gouhongshen
fix TestCDCCases Approved by: @zhangxu19830126
### **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 matrixorigin#16235 ## What this PR does / why we need it: Fix misuse of unsafe get string at
Implement the data branch. Approved by: @heni02, @zhangxu19830126, @aunjgr, @iamlinjunhong, @daviszhen, @XuPeng-SH
…STAMP, align with MySQL 8.0, add comprehensive tests (matrixorigin#22708) ### How it fixes: Add and use TruncateToScale for TIME; ensure Datetime/Timestamp/Time all apply scale consistently. Update CAST paths: Add timeToTime and datetimeToDatetime with TruncateToScale. Modify datetimeToTimestamp to accept targetScale and apply TruncateToScale. Ensure timestampToTimestamp/timestampToDatetime also apply target scale. Built-ins: apply TruncateToScale in current_timestamp/sysdate (and NOW family) per explicit scale; default behaves as scale 0. Type inference: restore scale=6 for string sources so literals keep full precision before truncation to target scale (MySQL 8.0 behavior). Tests: fix incorrect expectations in unit tests; rework flaky NOW tests; update plan_cache to use TIMESTAMP(6); add comprehensive BVT suites for TIME/DATETIME/TIMESTAMP covering rounding, boundaries, negatives, casts, arithmetic, comparisons, and functions; remove PASS/FAIL strings in results and assert via value outputs. ### Notes/impact: Breaking change: TIMESTAMP(0)/DATETIME(0)/TIME(0) now truly store/compare at seconds resolution. Workloads relying on microsecond differences at scale 0 must switch to scale 6. Aligns with MySQL 8.0 behavior; improves determinism; eliminates test flakiness. Approved by: @heni02, @ouyuanning
fix 22599 Approved by: @ouyuanning
add benchmark for write Approved by: @fengttt
…trixorigin#22714) 1. (u)int8/16/32/64, float32/64 with and without dictionary. 2. decimal, date, datetime, timestamp with and without dictionary. 3. The nested data type will be supported in the following PR. Approved by: @aunjgr, @XuPeng-SH, @zhangxu19830126
add case Approved by: @heni02
Rollback cannot delete the cloned shared files. Approved by: @XuPeng-SH, @ouyuanning
…aggregate (matrixorigin#22735) This PR implements overflow detection for integer arithmetic operations in SELECT queries and SUM aggregate functions, matching MySQL 8.0 strict mode behavior. Previously, integer overflow would silently wrap around, returning incorrect results. Now it properly throws an error. Approved by: @ouyuanning, @heni02
Data loss in distributed queries
```go
if res.err != nil && retErr != nil { // BUG: condition is always false
retErr = errors.Wrapf(res.err, "failed to get result from %s", res.nodeAddr)
}
```
**Issue**:
- `retErr` is initialized to `nil`
- Condition `retErr != nil` is always false on first error
- First error is silently ignored
- Function returns `nil` even when nodes fail
Approved by: @fengttt
…mation (matrixorigin#22741) Problem: - Decimal literals used internal scaled values (e.g., 500) instead of actual values (e.g., 5.0) in selectivity calculation - Caused TPC-H Q19 performance regression (dop 10→1, 1.5s→12.8s) Solution: - Add calcSelectivityByMinMaxForDecimal() for decimal-specific handling - Add getDecimalLiteralValue() to convert literals using proper scale - Use zm.GetScale() consistently in UpdateStatsInfo for scale consistency - Add boundary checks for out-of-range values Result: - Q19 performance restored - Decimal statistics accurate - Selectivity estimation correct - Comprehensive unit tests added Approved by: @ouyuanning
…CI environments due to platform-specific filesystem issues. (matrixorigin#22743) ### Problem The test was failing with high probability in CI with errors like: FSEventStreamStart errors on macOS Path mismatches due to symlink resolution (/var vs /private/var) panic: index out of range [0] when filesystem watching failed ### Root Causes Symlink path inconsistency: macOS resolves /var to /private/var, causing path comparison failures Filesystem watching failures: notify.Watch() can fail on macOS, Docker containers, and restricted CI environments Temporary directory paths: os.MkdirTemp() may return paths containing symlinks that need normalization ### Changes Normalize root path with filepath.EvalSymlinks() at test start Make filesystem watching failures non-fatal (changed from assert to warning log) Fix symlink comparison by using EvalSymlinks on both sides Handle nil event channel gracefully Approved by: @ouyuanning
### Root Cause In `pkg/container/types/decimal.go`, the `Parse64` function only handled the minus sign (`-`) in scientific notation exponents but not the plus sign (`+`). The `Parse128` function already handled both cases correctly. Approved by: @heni02
…atrixorigin#22742) This PR adds a comprehensive test suite to evaluate the robustness of MatrixOne's charset and collation implementation. The test suite contains **614 test cases** covering multilingual support, collation behaviors, string functions, and edge cases, achieving a **100% pass rate**. Approved by: @heni02
### Root Cause The issue occurs in the type checking logic of the IF() function (iffCheck in operatorSet.go): 1. When IF() receives mixed types (string and numeric), it tries to unify them to a single return type 2. The function iterates through retOperatorIffSupports array to find the best type match 3. Previously, numeric types (int8, int16, int32, etc.) were listed before string types (varchar, char, etc.) 4. This caused the system to attempt converting 'All years' (string) to int type 5. While the type system allows varchar→int implicit cast (for numeric strings like '123'), the actual cast of 'All years' to int fails at runtime ### Solution Added special handling logic in iffCheck function in /pkg/sql/plan/function/operatorSet.go to detect mixed string/numeric types and prioritize string type as the return type. Approved by: @heni02, @ouyuanning
…trixorigin#22749) This PR adds automatic type conversion support when loading Parquet files, enabling flexible data ingestion without manual preprocessing. ### Type Conversions Added **Integer Conversions (10)** - INT32 → INT64, UINT32 → UINT64 (widening) - STRING → INT8/16/32/64, UINT8/16/32/64 **Decimal Conversions (2)** - STRING → DECIMAL64, DECIMAL128 **Float Conversions (3)** - STRING → FLOAT32, FLOAT64 - FLOAT32 → FLOAT64 (widening) **Date/Time Conversions (3)** - STRING → DATE, TIME, TIMESTAMP Approved by: @ouyuanning, @heni02, @aunjgr
Fixes panic `index out of range [29] with length 29` in `INSERT IGNORE` with duplicate primary keys. **Root cause**: When `Batches.Shrink()` removes duplicate rows (35→23), `InputBatchRowCount` was not updated, causing bitmap initialization with wrong size. **Test**: Added test case with 35 rows (12 duplicates) to verify deduplication works correctly. Approved by: @ouyuanning, @heni02
When NewLocalETLFS was called with a rootPath that contained or ended with a symlink directory (e.g., /tmp/ABC/a/b/d/ where d is a symlink to c), the function only used filepath.Abs() which doesn't follow symlinks. This caused path resolution issues later when constructing file paths. Approved by: @reusee
fix bvt Approved by: @heni02
) ### **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 matrixorigin#22750 matrixorigin#22727 ## What this PR does / why we need it: ### Problem When a remote CN becomes unavailable (`ReceiverDone=true`), data destined for that CN was silently dropped without error in SendToAll/Shuffle scenarios, leading to incomplete query results. ### Solution Introduce `receiverFailureMode` to distinguish between scenarios: - **FailureModeStrict** (SendToAll/Shuffle): Return error on receiver failure - **FailureModeTolerant** (SendToAny): Allow failover to other receivers ___ ### **PR Type** Bug fix, Tests ___ ### **Description** - Prevent silent data loss when remote CN becomes unavailable in SendToAll/Shuffle scenarios - Introduce `receiverFailureMode` to distinguish strict vs tolerant failure handling - Add comprehensive integration tests documenting the data loss bug and fix - Return explicit errors in strict mode when receiver is done, protecting data integrity ___ ### Diagram Walkthrough ```mermaid flowchart LR A["Remote CN Unavailable<br/>ReceiverDone=true"] --> B{"Failure Mode?"} B -->|FailureModeStrict| C["SendToAll/Shuffle<br/>Return Error"] B -->|FailureModeTolerant| D["SendToAny<br/>Failover to Other"] C --> E["Query Fails<br/>Data Protected"] D --> F["Try Next Receiver<br/>Graceful Degradation"] ``` <details> <summary><h3> File Walkthrough</h3></summary> <table><thead><tr><th></th><th align="left">Relevant files</th></tr></thead><tbody><tr><td><strong>Tests</strong></td><td><table> <tr> <td> <details> <summary><strong>data_loss_integration_test.go</strong><dd><code>Integration tests for silent data loss prevention</code> </dd></summary> <hr> pkg/sql/colexec/dispatch/data_loss_integration_test.go <ul><li>New comprehensive integration test file documenting the silent data <br>loss bug<br> <li> Tests cover SendToAll, Shuffle, and real-world scenarios (full table <br>scan, shuffle join, GROUP BY aggregation)<br> <li> Demonstrates before/after behavior with clear error messages and data <br>integrity protection<br> <li> Includes multiple failure scenarios and comparison tables showing the <br>fix effectiveness</ul> </details> </td> <td><a href="https://github.com/matrixorigin/matrixone/pull/22751/files#diff-7f4a067e0a934a50f16c750f9d5927ad80b0a423c28211bee23489b6c2d8db1b">+353/-0</a> </td> </tr> <tr> <td> <details> <summary><strong>dispatch_test.go</strong><dd><code>Unit tests for failure mode handling and data loss prevention</code></dd></summary> <hr> pkg/sql/colexec/dispatch/dispatch_test.go <ul><li>Renamed <code>TestReceiverDone</code> to <code>TestReceiverDone_OldBehavior</code> for clarity<br> <li> Updated test expectations to verify strict mode returns errors when <br>receiver is done<br> <li> Added new unit tests for <code>sendBatchToClientSession</code> with both strict and <br>tolerant modes<br> <li> Added tests for SendToAll, SendToAny, and Shuffle scenarios with <br>receiver failures<br> <li> Added comparison table test documenting behavior differences before <br>and after fix</ul> </details> </td> <td><a href="https://github.com/matrixorigin/matrixone/pull/22751/files#diff-fdb9ccbe27daa2c248e9af9747e131fffee413882bb667458e4e0c0af84d7e83">+185/-3</a> </td> </tr> </table></td></tr><tr><td><strong>Bug fix</strong></td><td><table> <tr> <td> <details> <summary><strong>sendfunc.go</strong><dd><code>Implement failure mode handling and data loss prevention logic</code></dd></summary> <hr> pkg/sql/colexec/dispatch/sendfunc.go <ul><li>Introduced <code>receiverFailureMode</code> enum with <code>FailureModeStrict</code> and <br><code>FailureModeTolerant</code> constants<br> <li> Modified <code>sendBatchToClientSession</code> signature to accept <code>failureMode</code> and <br><code>receiverID</code> parameters<br> <li> Implemented critical fix: when <code>ReceiverDone=true</code>, return error in <br>strict mode to prevent silent data loss<br> <li> Updated <code>sendToAllRemoteFunc</code> to use <code>FailureModeStrict</code> for data <br>completeness<br> <li> Updated <code>sendBatToIndex</code> and <code>sendBatToMultiMatchedReg</code> to use <br><code>FailureModeStrict</code> for shuffle scenarios<br> <li> Updated <code>sendToAnyRemoteFunc</code> to use <code>FailureModeTolerant</code> allowing <br>failover to other receivers<br> <li> Added retry logic and improved error messages with receiver <br>identifiers</ul> </details> </td> <td><a href="https://github.com/matrixorigin/matrixone/pull/22751/files#diff-6bbcbe692f0f11ca8a189d32e18e4334f74c139121bebb9d2cad719dea586d54">+124/-27</a></td> </tr> </table></td></tr></tr></tbody></table> </details> ___
by doing cast for distance function operands before applying optimizer rules Approved by: @fengttt, @ouyuanning, @heni02
… for symlink handling and concurrent scenarios (matrixorigin#22762) ### Problem File system operations occasionally fail in concurrent test environments due to: Race conditions when resolving symlinks Strict error handling that doesn't account for transient states Missing validation that could lead to panics ### Solution Implement graceful degradation and defensive programming: Symlink Resolution - Fall back to original path if resolution fails Directory Detection - Unified error handling for all symlink errors Data Validation - Add length mismatch check to prevent panics Test Stability - Retry mechanism for transient file system issues Approved by: @reusee, @ouyuanning
This PR fixes a visibility issue when creating CDC tasks in multi-CN environments. Problem: When using mysqlstore transactions, CDC tasks created on one CN may not be immediately visible to the user session due to commit timestamp lag. Solution: Added commit timestamp synchronization after CDC task creation: New syncCommitTimestamp() method queries all CN nodes for their current commit timestamps Finds the maximum timestamp across all CNs Updates the session's transaction client with this timestamp Called automatically after CreateTask() completes Approved by: @daviszhen
## 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 matrixorigin#23553 ## What this PR does / why we need it: Try to find a reasonable spill memory setting.
## 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 matrixorigin#23562 ## What this PR does / why we need it: A series of bugs discovered by a different query plan.
…gin#23560) Cache page iterators instead of recreating them on each batch, fixing O(n²) file traversal that caused a 723MB parquet load to take 1+ hours. Now completes in ~1 minute. Approved by: @aunjgr
implement IN_RANGE/PREFIX_IN_RANGE functions, which will be pushed down to Reader in my next PR Approved by: @ouyuanning, @heni02
…23550) Add support for reading Parquet files with nested types (List, Struct, Map) by converting them to JSON/TEXT format. ### Supported scenarios: - Simple List: `[85, 90, 78]` - Simple Struct: `{"city": "Beijing", "zip": "100000"}` - Simple Map: `{"key1": "value1"}` - Empty List: `[]` - List of Struct: `[{"name": "apple", "price": 10}]` - Map of List: `{"a": [1, 2], "b": [3, 4, 5]}` - NULL nested values - List with NULL elements: `[1, null, 3]` - Struct with NULL fields - Map with NULL values - Struct nested Struct - Struct nested List: `{"name": "order", "items": [1, 2, 3]}` ### Known limitations (to be addressed in follow-up PRs): - Lossless export to Parquet format with nested types ### Usage When loading Parquet files with nested types, the target columns must be defined as `JSON` or `TEXT`: ```sql -- Parquet file has: id(int), scores(list), address(struct) CREATE TABLE t(id INT, scores JSON, address TEXT); LOAD DATA INFILE {'filepath'='file.parquet', 'format'='parquet'} INTO TABLE t; Approved by: @heni02, @ouyuanning
…#23567) Fix DECIMAL loading from PyArrow-generated Parquet files. Now checks LogicalType to distinguish binary DECIMAL (big-endian two's complement) from string format in ByteArray/FixedLenByteArray columns. Approved by: @ouyuanning, @aunjgr, @heni02
This reverts commit 27fec1d.
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
|
flypiggy seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
There was a problem hiding this comment.
Pull request overview
This PR adds a disaster-recovery path for LogService bootstrap by replaying extracted WAL entries into the Log shard, while delaying “HAKeeperRunning” until replay completes. It also makes the “wait for HAKeeper running” timeout configurable to accommodate long recovery times.
Changes:
- Add WAL recovery file parsing and replay logic during LogService bootstrap.
- Gate HAKeeper “running” state while WAL recovery is in progress to prevent TN from starting too early.
- Add configurable
hakeeper-running-timeoutand propagate it to embed/operator and mo-service launcher wait loops.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/logservice/wal_recovery.go | New WAL recovery reader/replayer and recovery completion tag handling. |
| pkg/logservice/store.go | Add walRecoveryInProgress flag to store state. |
| pkg/logservice/store_hakeeper_check.go | Skip setting HAKeeperRunning while WAL recovery flag is set. |
| pkg/logservice/service_bootstrap.go | Set/clear recovery-in-progress flag and invoke WAL recovery during bootstrap. |
| pkg/logservice/config.go | Add wal-data-path and configurable hakeeper-running-timeout with defaulting helper. |
| pkg/embed/operator.go | Use configured HAKeeper running timeout when waiting in embedded operator. |
| cmd/mo-service/launch.go | Thread configured timeout into HAKeeper wait logic and log it. |
| Makefile | Improve err-check and exclude WithTimeoutCause/WithDeadlineCause from grep matches. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+145
to
+156
| // Read entry count | ||
| countBuf := make([]byte, 4) | ||
| if _, err := io.ReadFull(f, countBuf); err != nil { | ||
| return nil, moerr.NewInternalErrorf(ctx, "failed to read entry count: %v", err) | ||
| } | ||
| count := binary.LittleEndian.Uint32(countBuf) | ||
|
|
||
| logger.Info("WAL recovery: reading entries from file", | ||
| zap.Uint32("count", count), | ||
| zap.String("file", filePath)) | ||
|
|
||
| entries := make([]WALEntry, 0, count) |
Comment on lines
+174
to
+178
| dataLen := binary.LittleEndian.Uint32(headerBuf[36:40]) | ||
|
|
||
| // Read raw data | ||
| entry.RawData = make([]byte, dataLen) | ||
| if _, err := io.ReadFull(f, entry.RawData); err != nil { |
Comment on lines
+199
to
+203
| cmd := make([]byte, headerSize+8+len(payload)) | ||
| binaryEnc.PutUint32(cmd[0:4], uint32(pb.UserEntryUpdate)) | ||
| binaryEnc.PutUint64(cmd[4:12], leaseHolderID) | ||
| copy(cmd[12:], payload) | ||
| return cmd |
Comment on lines
+110
to
+114
| if cfg.BootstrapConfig.Restore.WALDataPath != "" { | ||
| s.store.walRecoveryInProgress.Store(true) | ||
| s.runtime.SubLogger(runtime.SystemInit).Info("WAL recovery: set walRecoveryInProgress flag", | ||
| zap.String("wal_data_path", cfg.BootstrapConfig.Restore.WALDataPath)) | ||
| } |
Comment on lines
+130
to
+135
| // readWALDataFile reads WAL entries from the binary data file | ||
| // File format: | ||
| // [count:4][entry1_header:40][entry1_data]...[entryN_header:40][entryN_data] | ||
| // Entry header format: | ||
| // [DSN:8][SafeDSN:8][RaftIndex:8][RaftTerm:8][EntryCount:4][DataLen:4] | ||
| func (s *Service) readWALDataFile(ctx context.Context, filePath string) (*WALRecoveryData, error) { |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What type of PR is this?
Which issue(s) this PR fixes:
issue #
What this PR does / why we need it: