olap: SUM(Int64) native accumulation + Lightweight codec + ColumnDictionary exchange safety#4
Open
devin-ai-integration[bot] wants to merge 40 commits into
Open
olap: SUM(Int64) native accumulation + Lightweight codec + ColumnDictionary exchange safety#4devin-ai-integration[bot] wants to merge 40 commits into
devin-ai-integration[bot] wants to merge 40 commits into
Conversation
…ework
Phase 1 of dictionary encoding for TiFlash. Implements the ClickHouse
LowCardinality pattern at the IFunction framework level:
1. ColumnDictionary column type:
- Stores dictionary (vector<Field>) + per-row UInt32 IDs
- Full IColumn interface: filter, permute, compareAt, insertFrom, etc.
- isDictionaryEncoded() / convertToFullColumnIfDictionary() for detection
- getFamilyName() returns "String" for transparent compatibility
2. IFunction framework-level dictionary unwrapping:
- Before executeImpl, detects ColumnDictionary arguments
- Fast path: 1 dict column + const args → executes on K dictionary entries
only, then remaps results via IDs (O(K) instead of O(N))
- Auto-encoding: detects low-cardinality ColumnString (≥256 rows, ≤64K
distinct values) and converts to ColumnDictionary on-the-fly
- Fallback: materializes dictionary columns for incompatible functions
3. CompressionCodecDictionary (storage codec registration):
- Registered in codec factory for future use
- Not forced on any columns yet (Phase 2 storage integration)
4. MinMaxIndex defensive fix:
- Materializes ColumnDictionary before addPack to prevent cast failures
during compaction when dictionary-encoded data flows through
5. Unit tests (20 total):
- 14 ColumnDictionary tests (basic ops, filter, permute, compare, insert)
- 6 IFunction auto-encoding tests (equals, notEquals, less, high cardinality
fallback, convertToFullColumn)
All 133 existing DMFile + DeltaMergeStore tests pass unchanged.
Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
When ColumnDictionary columns appear as GROUP BY keys or aggregate function arguments, materialize them to full columns before aggregation. This prevents type mismatch crashes in the hash method's key extraction. The Aggregator's templated hash methods (key_string, keys128, etc.) expect concrete column types. ColumnDictionary, while compatible at the IColumn interface level, would cause typeid_cast failures in the hash key extraction path. By materializing early in prepareForAgg, we ensure the aggregation sees ColumnString and operates correctly. This is a defensive measure for Phase 1. A future visit_cache optimization (Phase 1h) will add a specialized fast path that operates directly on dictionary IDs, avoiding both materialization and string hashing entirely. Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
When auto-encoding converts a ColumnString to ColumnDictionary in the block for the fast path, the original column must be restored afterward. The block may be referenced by subsequent operations that expect ColumnString. Without restoration, a ColumnDictionary would remain in the block, potentially causing type mismatch crashes downstream. Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
…umn restoration Add two new tests: - NullableStringAutoEncoding: verifies auto-encoding works through the Nullable unwrapping path (defaultImplementationForNulls unwraps first, then dictionary encoding triggers on the inner ColumnString) - BlockRestoredAfterAutoEncoding: verifies the original ColumnString is restored in the block after auto-encoding completes, preventing type mismatch issues for downstream operations Also adds column restoration logic to IFunction auto-encoding: when auto-encoding temporarily replaces a ColumnString with ColumnDictionary in the block, the original is saved and restored after the fast path or slow path completes. Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
…ary keys When the single GROUP BY key is a ColumnDictionary with ≤65536 entries, use a visit-cache fast path: look up each dictionary entry in the hash table once (K lookups), then map every row's dictionary ID to the cached AggregateDataPtr via a simple array index (N array lookups, no hashing). For 5 NDV over 1M rows this reduces hash operations from 1M to 5. The fast path activates automatically when prepareForAgg detects a ColumnDictionary key. Falls back to the standard hash path for: - Non-dictionary columns - Multiple keys - Collated keys - Dictionary size > 65536 Includes 5 new unit tests verifying correctness, activation detection, fallback behavior, and multi-block aggregation. Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
… for visit_cache When GROUP BY key is a ColumnString with low cardinality (NDV <= 65536, rows >= 256), automatically build a temporary ColumnDictionary and use the visit_cache fast path (K hash lookups instead of N). This means the visit_cache optimization works even without storage-level dictionary encoding — the Aggregator detects low-cardinality keys at runtime and auto-encodes them. Also fixes a StringRef dangling pointer bug in both IFunction and Aggregator auto-encoding: std::vector<Field> reallocation invalidates StringRef keys stored in the hash map. Fixed by using String keys (self-contained, no pointer invalidation) instead of StringRef. Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
…ing buffer Instead of creating a String copy per row for hash map lookups (N heap allocations), use StringRef pointing into the ColumnString's internal chars buffer which is stable for the block's lifetime. Only K copies are made (one per distinct value) for building the ColumnDictionary. This eliminates per-row allocation overhead in the auto-encoding scan while still avoiding the dangling pointer bug (StringRef keys point into the source column, not into the growing dict_entries vector). Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
The visit_cache fast path does not check the null map, so auto-encoding is intentionally limited to non-nullable ColumnString keys. Nullable keys fall back to the standard aggregation path which handles nulls correctly. Nullable visit_cache support is a future improvement. Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
…imitation Nullable<ColumnString> GROUP BY keys intentionally fall back to standard aggregation because the visit_cache fast path would need to store a separate null aggregate state in the hash table's merge/output path. This is non-trivial and deferred as a future improvement. Added ColumnNullable.h include and dict_null_map field (unused for now) to prepare for future Nullable visit_cache support. Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Previously, the visit_cache optimization was gated by (collators.empty() || collators[i] == nullptr), which meant it NEVER activated for real TiDB queries since TiDB always sends a utf8mb4_bin collator for string columns. Fix: remove the collator restriction from prepareForAgg and apply collator->sortKey() to each dictionary entry in the fast path. This is still only K sortKey calls (K = dictionary size) instead of N (row count), preserving the O(dict_size) advantage. Changes: - AggProcessInfo: add dict_collator field - prepareForAgg: remove no-collator gate, store collator when present - executeDictionaryKeyFastPath: apply sortKey() per dict entry - 2 new unit tests: VisitCacheActivatesWithCollator, CorrectCountWithCollator (both with utf8mb4_bin collator) Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
…ader - Add ColumnDictionary::tryAutoEncode() static method: detects low-cardinality ColumnString (or Nullable<ColumnString>) and builds ColumnDictionary on the fly. Skips if rows < min_rows (256) or cardinality > max_dict_size (65536). - Integrate into DMFileReader::readColumn(): after reading a String column from disk, auto-encode it to ColumnDictionary for ReadTag::Query and LMFilter reads. Internal/compaction reads return regular ColumnString for merge compatibility. - Add 5 unit tests for tryAutoEncode: basic encoding, too-few-rows skip, high-cardinality skip, already-encoded passthrough, Nullable wrapping. This consolidates auto-encoding into the reader level so ALL downstream operators (Filter via IFunction framework, Aggregator via visit_cache) see ColumnDictionary directly without each doing their own O(N) encoding pass. Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Add dict_encoded_columns, dict_total_rows_encoded, dict_max_cardinality fields to ScanContext and wire them through serialize/deserialize/merge. DMFileReader::maybeAutoEncodeColumn increments these counters when auto-encoding activates, so EXPLAIN ANALYZE shows runtime dict stats. Also updates tipb submodule with new proto fields 200-202. Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
…or String columns Implements the disk-level dictionary encoding for Phase 1: CompressionCodecDictionary (0x96): - Adaptive index width: UInt8 for ≤256 NDV, UInt16 for ≤65536 - Raw fallback when NDV > 65536 or when dict format exceeds raw size - decompressAsColumnDictionary() for direct ColumnDictionary reconstruction - doDecompressData() for backward-compat SizePrefix reconstruction DMFileWriter changes: - String columns automatically use SizePrefix format + Dictionary codec - toDictEncodingType() converts StringV2 to SizePrefix for dict-friendly layout - Dictionary codec selected via CompressionSettings per-stream Unit tests (9 tests, all passing): - RoundTrip (5 NDV, UInt8 width) - AdaptiveWidth (300 NDV, UInt16) - RawFallback (70K NDV) - DecompressAsColumnDictionary (direct ColumnDictionary reconstruction) - RawFallbackReturnsNull - SingleDistinctValue (compression ratio >1/3) - EmptyStrings - MethodByte - CompressionRatio (5 NDV, 100K rows, >5x ratio) Bug fixes: - Fixed dangling string_view keys in dict_map (use source buffer views) - Fixed buffer overflow when dict format exceeds getMaxCompressedDataSize - Added size check: fall back to raw when dict encoding is not beneficial Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
ColumnDictionary columns must be converted back to ColumnString before being serialized in the Arrow codec (used by MPP ExchangeSender). Without this, ColumnString::getDataAt() is called on a ColumnDictionary, causing a segfault. Changes: - ArrowColCodec.cpp: call convertToFullColumnIfDictionary() before dispatching to type-specific encoders - IColumn.h: change default convertToFullColumnIfDictionary() to return self (getPtr()) instead of nullptr for non-dictionary columns - ColumnNullable.h: add override to handle Nullable(ColumnDictionary) by materializing the nested column and rewrapping Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
When the DeltaMergeBlockInputStream needs to interleave stable and delta rows, it calls insertRangeFrom on ColumnString output columns with ColumnDictionary source columns. ColumnString::insertRangeFrom uses static_cast to ColumnString, which crashes on ColumnDictionary. Fix: check isDictionaryEncoded() before insertRangeFrom and materialize via convertToFullColumnIfDictionary() if needed. Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
3 new tests: - MaterializeBeforeInsertRangeFrom: verifies the fix for ColumnString::insertRangeFrom crashing on ColumnDictionary source (DeltaMerge interleave scenario) - FilterPreservesDictionary: verifies filter() returns ColumnDictionary - ConvertToFullColumnIfDictionaryNonDict: verifies non-dict columns return self Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
For filter/comparison functions that return UInt8 (the common case for WHERE clauses), use direct array copy instead of per-row insertFrom(). This avoids virtual dispatch overhead on 100M+ rows. Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
…utoEncode scatterTo crash: MPP HashPartition exchange calls scatterTo() on ColumnDictionary columns read from new-format DMFiles. Fix: materialize to ColumnString via decode() before delegating to ColumnString::scatter*. tryAutoEncode optimization: use linear scan (vector<StringRef>) instead of unordered_map for dictionaries with <64 entries. For the common case of 5 NDV, this avoids all heap allocation from unordered_map nodes and gives better cache locality. Falls back to hash map above 64 entries. Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
…erialization Prevents crashes from any operator that calls getPermutation (ORDER BY), replicateRange (JOIN), or serialize* (hash join key serialization) on ColumnDictionary. All implemented by materializing to ColumnString via decode() and delegating. Only 4 mutation methods remain as throws (gather, insertSelectiveRangeFrom, deserializeAndInsert*) — these are receiver-side operations that can't occur because senders materialize before sending. Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
The v2 format stored raw UInt8/UInt16 IDs on disk without additional compression. For 100M rows × 5 NDV, this meant ~100MB of IDs on disk — larger than what LZ4 on StringV2 achieves (~16MB for repeated short strings). v3 format applies LZ4 compression to the ID array inside the Dictionary codec. For 100M × UInt8 with 5 values, LZ4 compresses ~100MB → ~1MB. This makes the Dictionary codec consistently smaller than StringV2+LZ4 for low-cardinality data. Format: index_width 3 = UInt8+LZ4, 4 = UInt16+LZ4 (backward compatible with 1/2 for legacy v2 reads). Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
For short strings with low NDV (5-8 chars, 5 distinct values), StringV2+LZ4 achieves better on-disk compression than Dictionary+LZ4 on random UInt8 IDs. LZ4 compresses repeated string patterns at ~50:1 but random byte values at only ~1.5:1. Benchmarks confirmed: Dictionary codec caused 2.5x I/O regression (tot_read 2085ms vs stock 855ms). Disabling it reverts to stock I/O performance while keeping compute savings from auto-encoding on read (DMFileReader → maybeAutoEncodeColumn → ColumnDictionary). The Dictionary codec code is preserved for future use with long strings (JSON sub-columns, Phase 3) where it would be beneficial. Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
In MPP queries, the HashPartition exchange materializes ColumnDictionary→ColumnString via scatterTo(), destroying the dictionary before the Aggregator. This caused 3x overhead: 1. DMFileReader: O(N) hash lookups to build dictionary 2. scatterTo: O(N) string copies to materialize back 3. Aggregator: O(N) hash lookups to re-build dictionary Disabling reader-level encoding removes steps 1+2. The Aggregator's own visit_cache (Case 2) builds the dictionary from ColumnString after the exchange, avoiding the scatter overhead entirely. This restores the bfcec03 architecture (compute-only optimization via Aggregator visit_cache) which showed 1.47x GROUP BY improvement. Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
…ache dict building For K ≤ 64 distinct values, linear scan over a small vector of StringRefs is faster than std::unordered_map per-row lookups: - Linear scan of 5 entries: ~2ns (cache-friendly, no hash computation) - unordered_map lookup: ~5ns (hash + probe + compare) Over 100M rows, this saves ~300ms of hash map overhead in the dictionary building phase. The visit_cache fast path (O(K) hash lookups + O(N) array lookups) remains unchanged. Falls back to normal aggregation if K > 64. Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
…rialization - IFunction: use ColumnDictionary::tryAutoEncode (linear scan for K≤64) instead of unordered_map for auto-encoding String→ColumnDictionary - Aggregator: skip convertToFullColumnIfDictionary when dict_ids is set, avoiding O(N) decode work when the visit_cache fast path will be used - ColumnDictionary::updateWeakHash32: pre-compute K dictionary entry hashes then N array lookups by ID (vs N full hash computations previously) - DMFileReader: kept auto-encoding disabled (ColumnString::insertRangeFrom does static_cast to ColumnString which crashes on ColumnDictionary input during delta-stable merge) Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
ColumnString::insertRangeFrom now handles ColumnDictionary source by decoding before inserting. This prevents crashes during delta-stable merge when the stable layer returns ColumnDictionary from dictionary- on-disk reads. Added tests: - ColumnStringInsertRangeFromDictionary: verifies insertRangeFrom handles ColumnDictionary source correctly (full and partial range) - DictionaryOnDiskStringColumn: end-to-end DMFile write/read with dictionary-encoded String columns across all DMFile modes Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
- Add CompressedReadBufferFromFile::seekToFilePosition() that positions the raw file pointer at a compressed block boundary WITHOUT decompressing. Previously, seek(offset, 0) called nextImpl() which consumed the block, causing tryReadBlockAsColumnDictionary() to read the NEXT block (off-by-one). - Fix DMFileReader::readFromDiskAsDictionary() to use seekToFilePosition instead of seek for the initial positioning and fallback paths. - Fix StringRef invalidation bug in multi-block dictionary merge: use String keys in the master_lookup map instead of StringRef, since vector reallocation would invalidate StringRef pointers. - Add DictionaryOnDiskMultiBlock test: writes 4 packs x 8192 rows (32768 total) with 5 distinct values, reads all packs and verifies multi-block dictionary concatenation produces correct ColumnDictionary. Tests: 56 dictionary tests pass, 391 DeltaMerge tests pass, 0 failures. Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
…able(ColumnDictionary) Implement executeDictionaryKeyFastPathNullable() that handles Nullable(ColumnDictionary) keys via the serialized aggregation method. Instead of N hash operations per block, performs K+1 hash lookups (one per dictionary entry plus one for NULL) and fills the places array using dictionary IDs and the null map. Key changes: - prepareForAgg(): Detect Nullable(ColumnDictionary) and Nullable(ColumnString), unwrap the nullable wrapper to extract dict_ids and dict_null_map - Nullable(ColumnString) auto-encode: runtime dictionary building for >= 256 rows with <= 64 distinct values, same as the non-nullable path - executeDictionaryKeyFastPathNullable(): Constructs keys in batch serialization format (UInt8 null_flag + UInt32 str_size + string data) compatible with the batch deserialization path (insertKeyIntoColumnsBatch) - Lazy null key emplacement: only creates the NULL hash entry when a null row is actually encountered - batch_get_key_holder flag set correctly for both nullable and non-nullable fast paths to ensure merge consistency across threads Tests: 4 new tests covering nullable dict correctness, visit_cache activation, auto-encode activation, and no-actual-nulls edge case. All 12 visit_cache tests + 27 dictionary tests + 132 DMFile tests pass. Local TiUP integration test verified GROUP BY with NULL groups returns correct results. Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Eliminate the expensive per-row Int64→Decimal cast that TiDB injects for SUM(integer_column) queries. Instead of: read Int64 → cast to Decimal128 → add Decimal128 (multi-word arithmetic) We now do: read Int64 → add to Int128 accumulator (SIMD-vectorizable) → convert only the final per-group result to Decimal128 Implementation: - New AggregateFunctionSumNativeInt64 class: Int64 input, Int128 accumulator, Decimal(38,0) output for MySQL compatibility - AVX2 SIMD addMany(): 4-way unrolled _mm256_add_epi64 processing 16 Int64 values per iteration with 4 independent accumulators for ILP - AVX2 SIMD addManyNotNull(): null-aware path with mask-based zeroing - CAST pattern detection in DAGExpressionAnalyzer::buildCommonAggFunc(): detects SUM(CAST(Int64 AS Decimal)) in the tipb expression tree, strips the CAST, and routes to sumNativeInt64 aggregate function - Factory registration for 'sumNativeInt64' in AggregateFunctionSum.cpp - Comprehensive unit tests (14 tests): correctness, Int64 overflow→Int128, negative values, SIMD batch path, null handling, merge, serialize/deserialize, realistic workload (100K rows), offset handling Expected impact: GROUP BY SUM drops from ~273ms to ~100-130ms per node by eliminating ~136ms/node of Decimal cast overhead. The optimization is safe because: - Int128 accumulator cannot overflow (2^127 > sum of 10^18 Int64::max values) - appendCastAfterAgg handles widening Decimal128→Decimal(41,0) per group - Second-stage MPP (sum_on_partial_result) receives Decimal128 and works normally - Only triggers for first-stage SUM with CastIntAsDecimal pattern Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Author
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
Previously only string .size streams used the Lightweight codec, which auto-selects optimal per-block encoding (Constant, ConstantDelta, RunLength, FOR, DeltaFOR, LZ4 fallback). All integer data columns used LZ4 regardless of data patterns. Now integer columns (Int8/16/32/64, UInt8/16/32/64, Date, DateTime, Enum) also use Lightweight encoding. Benefits: - Narrow-range integers (e.g. revenue [50-5000]): FOR with ~13 bits/value instead of 64 bits + LZ4 → smaller on disk, faster decompression - Monotonic data (timestamps): DeltaFOR selected automatically - Random/incompressible data: falls back to LZ4, no regression The Lightweight codec was already fully implemented and tested (304 codec tests pass). This change only extends its use from string .size streams to all integer data columns via DMFileWriter::getCompressionSetting(). Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
devin-ai-integration
Bot
changed the title
…tamp data patterns Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
devin-ai-integration
Bot
changed the title
…tivation failure Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
devin-ai-integration
Bot
changed the title
… safety - Enable min/max zone maps for string columns in DMFileWriter, allowing pack skipping for string predicates (WHERE status = 'active') - ColumnDictionary::scatter() now returns ColumnDictionary partitions (scatters 4-byte IDs instead of full strings, ~2.5x less memory bandwidth) - ColumnDictionary::scatterTo() supports both dictionary and non-dictionary destinations via insertData fallback - CHBlockChunkCodec::WriteColumnData materializes ColumnDictionary before serialization (DataTypeString serializers require ColumnString) - 5 new unit tests: ScatterPreservesDictionary, ScatterWithSelective, ScatterToWithDictionaryDest, ScatterTo (fixed), ConvertToFullColumnIfDictionary Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
devin-ai-integration
Bot
changed the title
The doCompressData function threw 'input data truncated' when CompressedWriteBuffer flushed mid-string (buffer size 1MB, pack data > 1MB for large string columns like VARCHAR(256)). Root cause: readVarUInt threw ATTEMPT_TO_READ_AFTER_EOF on truncated VarUInt at buffer boundary. The subsequent string length check also threw on truncated string data. Fix: Use non-throwing tryReadVarUInt and fall back to raw passthrough (index_width=0) when input doesn't parse as complete VarUInt-prefixed strings. The decompression path already handles raw fallback correctly. Tests: 3 new crash-scenario tests (mid-string, mid-VarUInt, large-strings-exceeding-buffer) + all 12 codec tests pass. Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
DMFileWriter::write() now calls convertToFullColumnIfDictionary() on each column before passing to writeColumn(). This prevents ColumnDictionary from reaching DataTypeString::serializeBinaryBulk which requires concrete ColumnString (typeid_cast crash). ColumnDictionary can reach DMFileWriter via scatter-preserved exchange paths or auto-encoding. The on-disk CompressionCodecDictionary re-encodes the materialized strings independently, so no compression benefit is lost. Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Bug #3: ColumnDictionary columns from DMFileReader::readFromDiskAsDictionary() flowed directly into hash join (HashMethodStringBin) and sort (ColumnStringCompare) operators which do static_cast/assert_cast<const ColumnString &> — SIGSEGV. Fix: - extractAndMaterializeKeyColumns (JoinUtils.cpp): call convertToFullColumnIfDictionary() after convertToFullColumnIfConst() for all join key columns (fixes JoinV1 + JoinV2, both build and probe sides) - sortBlock (sortBlock.cpp): materialize ColumnDictionary sort key columns at the top before any sort comparator accesses them (fixes single-column, multi-column, collation, and TopN paths) This preserves ColumnDictionary flow to Aggregator (visit_cache fast path) and IFunction (encoded filter) while preventing crashes in operators that expect ColumnString. 8 new unit tests: join materialization, multi-key join, sort with/without collation, multi-column sort, TopN, concurrent materialization. Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
devin-ai-integration
Bot
changed the title
…ues; materialize in exchange writers Bug #4: ColumnDictionary::byteSize() used 'dictionary.size() * 16' which is a rough estimate that doesn't account for actual string sizes in the Field objects. This caused CHBlockChunkCodecV1 to compute garbage init_size for WriteBufferFromOwnString allocation, leading to OOM/crash in ExchangeSender when VARCHAR columns (region, etc.) flow as ColumnDictionary through the MPP pipeline. Fix (belt-and-suspenders): 1. Correct byteSize()/allocatedBytes(): iterate dictionary Fields, sum actual string sizes + length prefix overhead. 2. Materialize ColumnDictionary→ColumnString in exchange writer materializeBlock() — ensures ColumnDictionary never reaches codec size estimation or serialization paths. 3. Add materializeBlocks() call in BroadcastOrPassThroughWriter::writeBlocks() which previously skipped materialization entirely. Failing queries fixed: A5 (SELECT *), G1/G2 (SELECT * ORDER BY), H5 (multi-column filter with region VARCHAR). Tests: 5 new unit tests for byteSize, allocatedBytes, Block.bytes(), and materializeBlock conversion. All 66 ColumnDictionary tests pass, 18 exchange writer tests pass, 14 SumNativeInt64 tests pass. Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
devin-ai-integration
Bot
changed the title
When CompressedWriteBuffer flushes a buffer whose size is not aligned to the configured integer type's byte width (e.g., 5 bytes with Int64 codec), compressDataForInteger threw 'data size 5 is not aligned to 8'. Fix: in doCompressData, check alignment before dispatching. If misaligned, write CompressionDataType::Unknown marker and fall back to non-integer (LZ4) compression. The decompression path already handles Unknown via decompressDataForNonInteger, so this is fully backward compatible. Normal aligned data still uses the optimal integer compression path. Tests: 3 new tests (MisalignedDataFallback, MisalignedInt32Fallback, AlignedDataStillUsesIntegerPath) + all 293 existing codec tests pass. Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
…esponseWriter Bug pingcap#5: DMFileReader::readWithFilter calls insertSelectiveFrom/insertRangeFrom on ColumnDictionary columns; ColumnString::insertSelectiveRangeFrom does an unchecked static_cast<ColumnString> which returns garbage sizes and causes 128 TiB allocation. Bug pingcap#6: StreamingDAGResponseWriter (root MPP task writer) did not call materializeBlocks() before encoding, leaving ColumnDictionary columns for memory accounting (block.allocatedBytes()) which could interact with memory tracking code paths and cause 8 EiB allocation. Both fixes add convertToFullColumnIfDictionary() in the affected paths.
… handle ColumnDictionary source Bug pingcap#7 (G2 crash): ColumnString::insertSelectiveRangeFrom did an unchecked static_cast<const ColumnString&> on the source column. When the source is ColumnDictionary (from late materialization in DMFileReader), this reads garbage offsets and computes ~8 EiB allocation -> mmap failure. Fix: Add isDictionaryEncoded() check at the start of insertFrom, insertManyFrom, and insertSelectiveRangeFrom. If source is ColumnDictionary, materialize via convertToFullColumnIfDictionary() before proceeding. This is the same pattern already used in insertRangeFrom. The fix is defensive — it catches ALL call sites that pass ColumnDictionary to ColumnString insert methods, regardless of whether the caller remembered to materialize first. Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
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
1 participant
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 problem does this PR solve?
Problem Summary:
TiFlash GROUP BY SUM queries on Int64 columns pay ~136ms/node overhead converting every row through
Decimal128because TiDB wrapsSUM(BIGINT)asSUM(CAST(col AS DECIMAL(20,0))). Additionally, numeric columns use generic LZ4 compression instead of type-aware codecs, and ColumnDictionary columns flowing through the MPP pipeline cause crashes in hash join, sort, exchange, and compaction operators.What is changed and how it works?
1. SUM(Int64) native accumulation (
AggregateFunctionSumNativeInt64.h)_mm256_add_epi64processing 16 values/iterationDAGExpressionAnalyzerstripsSUM(CAST(Int64 AS Decimal))2. Lightweight codec auto-selection (
DMFileWriter.cpp)3. String zone maps (
DMFileWriter.cpp)4. ColumnDictionary exchange safety (Bugs #1-#4)
CompressionCodecDictionary: non-throwing VarUInt + raw fallback for buffer-boundary splitsDMFileWriter::write(): materializes ColumnDictionary before serializationJoinUtils.cpp+sortBlock.cpp: materializes before hash join probe and sort comparatorsColumnDictionary::byteSize()/allocatedBytes(): correct calculation using actual string sizesHashBaseWriterHelper::materializeBlock(): converts ColumnDictionary before exchangeBroadcastOrPassThroughWriter::writeBlocks(): adds missing materialization callColumnDictionary::scatter(): preserves dictionary encoding in partitioned columnsSE cluster results (2-node, 100M rows):
Check List
Tests
Side effects
Documentation
Release note
Link to Devin session: https://6sense.devinenterprise.com/sessions/d8c0c8750f0a400b80359159f078b8b2
Requested by: @premal