Skip to content

Commit de7ed87

Browse files
committed
fix(shredding): address shared-shredding review comments
1 parent 1c40f3d commit de7ed87

4 files changed

Lines changed: 20 additions & 14 deletions

File tree

src/paimon/common/data/shredding/lru_map_shared_shredding_column_allocator.cpp

Lines changed: 3 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -31,18 +31,11 @@ int32_t LruMapSharedShreddingColumnAllocator::SelectColumn(
3131
const std::vector<int32_t>& planned_col_to_field) const {
3232
int32_t selected_col = candidates.front();
3333
int64_t selected_last_used = std::numeric_limits<int64_t>::max();
34-
bool selected_empty = false;
3534
for (int32_t col : candidates) {
36-
bool is_empty = planned_col_to_field[col] == -1;
37-
if (is_empty) {
38-
if (!selected_empty || col < selected_col) {
39-
selected_empty = true;
40-
selected_col = col;
41-
}
42-
continue;
35+
if (planned_col_to_field[col] == -1) {
36+
return col;
4337
}
44-
if (!selected_empty && (last_used_[col] < selected_last_used ||
45-
(last_used_[col] == selected_last_used && col < selected_col))) {
38+
if (last_used_[col] < selected_last_used) {
4639
selected_col = col;
4740
selected_last_used = last_used_[col];
4841
}

src/paimon/common/data/shredding/lru_map_shared_shredding_column_allocator.h

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,12 @@
2323

2424
namespace paimon {
2525

26-
/// Allocator that keeps cross-row column state and evicts least-recently-used columns.
26+
/// Allocator that keeps current column assignments across rows and evicts least-recently-used
27+
/// columns.
28+
///
29+
/// Keys that are still assigned to physical columns keep those columns when they appear again.
30+
/// New keys, including keys that were previously evicted, use empty columns first; if no empty
31+
/// column is available, the allocator replaces the least-recently-used physical column.
2732
class LruMapSharedShreddingColumnAllocator : public MapSharedShreddingColumnAllocator {
2833
public:
2934
/// @param num_columns Number of available physical columns.

src/paimon/common/data/shredding/map_shared_shredding_batch_converter_test.cpp

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -445,6 +445,13 @@ TEST_F(MapSharedShreddingBatchConverterTest, SequentialPlacementUsesSmallestColu
445445
.ValueOrDie();
446446

447447
AssertArrayEquals(expected, actual);
448+
449+
MapSharedShreddingFieldMeta expected_meta;
450+
expected_meta.name_to_id = {{"a", 0}, {"b", 1}, {"c", 2}};
451+
expected_meta.field_to_columns = {{0, {0}}, {1, {1}}, {2, {2}}};
452+
expected_meta.num_columns = 3;
453+
expected_meta.max_row_width = 3;
454+
ASSERT_EQ(expected_meta, converter->BuildFieldMeta("tags").value());
448455
}
449456

450457
TEST_F(MapSharedShreddingBatchConverterTest, LruPlacementPreservesResidentColumns) {

src/paimon/core/options/map_shared_shredding_column_placement_policy.h

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -20,11 +20,12 @@ namespace paimon {
2020

2121
/// Specifies how shared-shredding MAP fields choose physical columns.
2222
enum class MapSharedShreddingColumnPlacementPolicy {
23-
/// Keep input field order and place fields into columns 0..K-1.
23+
/// Keep the key order from each input MAP row and place the first K keys into columns 0..K-1.
2424
PLAIN = 0,
25-
/// Sort field IDs and place fields into columns 0..K-1.
25+
/// Use a stable key order before placing the first K keys into columns 0..K-1.
2626
SEQUENTIAL = 1,
27-
/// Choose empty columns first, then the least-recently-used physical column.
27+
/// Reuse columns for recently seen keys when possible; otherwise choose an empty column first,
28+
/// then the least-recently-used physical column.
2829
LRU = 2
2930
};
3031

0 commit comments

Comments
 (0)