Skip to content

Commit 493be2e

Browse files
jacktenggjacktengg
committed
[fix](be) Fix missing decimal precision and scale for legacy decimal data (#65419)
Issue Number: close #xxx Related PR: #26572, #63569 Problem Summary: Segments written before decimal ColumnMetaPB precision and scale were persisted can miss precision and scale for decimal types in metadata . Regular columns, complex decimal children may then build decimal DataType instances without the logical precision/scale from TabletSchema, and result in error "Wrong precision 0, min: 1, max: 76". This change centralizes decimal metadata recovery from TabletSchema, applies it before ColumnReader/DataType creation. Fix reading legacy decimal whose segment metadata is missing decimal precision and scale. - Test <!-- At least one of them must be included. --> - [ ] Regression test - [ ] Unit Test - [ ] Manual test (add detailed scripts or steps below) - [ ] No need to test or manual test. Explain why: - [ ] This is a refactor/code format and no logic has been changed. - [ ] Previous test can cover this change. - [ ] No code files have been changed. - [ ] Other reason <!-- Add your reason? --> - Behavior changed: - [ ] No. - [ ] Yes. <!-- Explain the behavior change --> - Does this need documentation? - [ ] No. - [ ] Yes. <!-- Add document PR link here. eg: apache/doris-website#1214 --> - [ ] Confirm the release note - [ ] Confirm test cases - [ ] Confirm document - [ ] Add branch pick label <!-- Add branch pick label that this PR should merge into -->
1 parent 009bcf4 commit 493be2e

10 files changed

Lines changed: 311 additions & 220 deletions

File tree

‎be/src/core/data_type/data_type_decimal.h‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -124,6 +124,10 @@ class DataTypeDecimal final : public IDataType {
124124

125125
DataTypeDecimal(UInt32 precision = max_decimal_precision<T>(),
126126
UInt32 scale = default_decimal_scale<T>(),
127+
// For decimalv2 only, record the original(schema) precision and scale.
128+
// UINT32_MAX means original precision and scale are unknown.
129+
// Decimalv2 is converted to Decimal(27, 9) in memory for calculations,
130+
// but string casts should retain the precision and scale from the schema.
127131
UInt32 arg_original_precision = UINT32_MAX,
128132
UInt32 arg_original_scale = UINT32_MAX)
129133
: precision(precision),

‎be/src/core/data_type/data_type_factory.cpp‎

Lines changed: 2 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -380,23 +380,8 @@ DataTypePtr DataTypeFactory::create_data_type(const segment_v2::ColumnMetaPB& pc
380380
nested = std::make_shared<DataTypeStruct>(dataTypes, names);
381381
} else {
382382
// TODO add precision and frac
383-
auto meta_precision = pcolumn.precision();
384-
auto meta_scale = pcolumn.frac();
385-
if (pcolumn.type() == static_cast<int>(FieldType::OLAP_FIELD_TYPE_DECIMAL)) {
386-
// Segments written by Doris < 2.1.0 (before #26572) do not persist
387-
// precision/frac in ColumnMetaPB, so they default to 0 when read back.
388-
// Pass UINT32_MAX to DataTypeDecimalV2 to signal that the original
389-
// precision/scale are unknown; otherwise check_type_precision(0) throws
390-
// "meet invalid precision: real_precision=0".
391-
UInt32 orig_precision =
392-
meta_precision > 0 ? static_cast<UInt32>(meta_precision) : UINT32_MAX;
393-
UInt32 orig_scale = meta_precision > 0 ? static_cast<UInt32>(meta_scale) : UINT32_MAX;
394-
nested = _create_primitive_data_type(static_cast<FieldType>(pcolumn.type()),
395-
orig_precision, orig_scale, -1);
396-
} else {
397-
nested = _create_primitive_data_type(static_cast<FieldType>(pcolumn.type()),
398-
meta_precision, meta_scale, -1);
399-
}
383+
nested = _create_primitive_data_type(static_cast<FieldType>(pcolumn.type()),
384+
pcolumn.precision(), pcolumn.frac(), -1);
400385
}
401386

402387
if (pcolumn.is_nullable() && nested) {

‎be/src/storage/index/inverted/inverted_index_iterator.cpp‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -273,7 +273,7 @@ Result<InvertedIndexReaderPtr> InvertedIndexIterator::select_best_reader(
273273
return select_for_text(match, query_type, normalized_key);
274274
}
275275

276-
if (is_numeric_type(field_type)) {
276+
if (field_is_numeric_type(field_type)) {
277277
return select_for_numeric(match, query_type);
278278
}
279279

‎be/src/storage/olap_common.h‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -195,6 +195,14 @@ constexpr bool field_is_slice_type(const FieldType& field_type) {
195195
field_type == FieldType::OLAP_FIELD_TYPE_STRING;
196196
}
197197

198+
constexpr bool field_is_decimal_type(const FieldType& field_type) {
199+
return field_type == FieldType::OLAP_FIELD_TYPE_DECIMAL ||
200+
field_type == FieldType::OLAP_FIELD_TYPE_DECIMAL32 ||
201+
field_type == FieldType::OLAP_FIELD_TYPE_DECIMAL64 ||
202+
field_type == FieldType::OLAP_FIELD_TYPE_DECIMAL128I ||
203+
field_type == FieldType::OLAP_FIELD_TYPE_DECIMAL256;
204+
}
205+
198206
constexpr bool field_is_numeric_type(const FieldType& field_type) {
199207
return field_type == FieldType::OLAP_FIELD_TYPE_INT ||
200208
field_type == FieldType::OLAP_FIELD_TYPE_UNSIGNED_INT ||

‎be/src/storage/segment/column_reader.cpp‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -692,7 +692,7 @@ Status ColumnReader::_load_index(const std::shared_ptr<IndexFileReader>& index_f
692692
"create StringTypeInvertedIndexReader error: {}", e.what());
693693
}
694694
}
695-
} else if (is_numeric_type(type)) {
695+
} else if (field_is_numeric_type(type)) {
696696
try {
697697
index_reader = BkdIndexReader::create_shared(index_meta, index_file_reader);
698698
} catch (const CLuceneError& e) {

‎be/src/storage/segment/segment.cpp‎

Lines changed: 48 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@
2323
#include <gen_cpp/olap_file.pb.h>
2424
#include <gen_cpp/segment_v2.pb.h>
2525

26+
#include <algorithm>
2627
#include <cstring>
2728
#include <memory>
2829
#include <set>
@@ -144,6 +145,49 @@ Status build_segment_zonemap_context(Segment* segment, const Schema& schema,
144145
return Status::OK();
145146
}
146147

148+
void fill_missing_decimal_precision(const TabletColumn& column, ColumnMetaPB* meta) {
149+
auto meta_type = static_cast<FieldType>(meta->type());
150+
if (meta_type != column.type()) {
151+
return;
152+
}
153+
154+
if (field_is_decimal_type(meta_type)) {
155+
if ((!meta->has_precision() || meta->precision() <= 0) && column.precision() > 0) {
156+
meta->set_precision(column.precision());
157+
}
158+
if ((!meta->has_frac() || meta->frac() < 0) && column.frac() >= 0) {
159+
meta->set_frac(column.frac());
160+
}
161+
}
162+
163+
// Complex column meta may also include storage helper children, such as
164+
// array offsets. Only schema children have matching TabletColumn subtypes.
165+
int child_count =
166+
std::min(meta->children_columns_size(), static_cast<int>(column.get_subtype_count()));
167+
for (int i = 0; i < child_count; ++i) {
168+
fill_missing_decimal_precision(column.get_sub_column(i), meta->mutable_children_columns(i));
169+
}
170+
}
171+
172+
void fill_missing_decimal_precision_from_schema(const TabletSchemaSPtr& tablet_schema,
173+
ColumnMetaPB* meta) {
174+
if (!meta->has_unique_id()) {
175+
return;
176+
}
177+
int32_t col_idx = tablet_schema->field_index(static_cast<int32_t>(meta->unique_id()));
178+
if (col_idx < 0) {
179+
return;
180+
}
181+
fill_missing_decimal_precision(tablet_schema->column(col_idx), meta);
182+
}
183+
184+
void fill_footer_missing_decimal_precision(const TabletSchemaSPtr& tablet_schema,
185+
SegmentFooterPB* footer) {
186+
for (int i = 0; i < footer->columns_size(); ++i) {
187+
fill_missing_decimal_precision_from_schema(tablet_schema, footer->mutable_columns(i));
188+
}
189+
}
190+
147191
} // namespace
148192

149193
Status Segment::open(io::FileSystemSPtr fs, const std::string& path, int64_t tablet_id,
@@ -553,6 +597,10 @@ Status Segment::_parse_footer(std::shared_ptr<SegmentFooterPB>& footer,
553597
_file_reader->path().native(), file_size,
554598
file_cache_key_str(_file_reader->path().native()));
555599
}
600+
// Segments written before #26572 do not persist decimal precision/frac in
601+
// ColumnMetaPB, so recover the logical p/s from TabletSchema before
602+
// ColumnReader builds DataTypeDecimal.
603+
fill_footer_missing_decimal_precision(_tablet_schema, footer.get());
556604

557605
VLOG_DEBUG << fmt::format("Loading segment footer from {} finished",
558606
_file_reader->path().native());

‎be/src/storage/utils.h‎

Lines changed: 0 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -182,32 +182,6 @@ constexpr bool is_string_type(const FieldType& field_type) {
182182
field_type == FieldType::OLAP_FIELD_TYPE_STRING;
183183
}
184184

185-
constexpr bool is_numeric_type(const FieldType& field_type) {
186-
return field_type == FieldType::OLAP_FIELD_TYPE_INT ||
187-
field_type == FieldType::OLAP_FIELD_TYPE_UNSIGNED_INT ||
188-
field_type == FieldType::OLAP_FIELD_TYPE_BIGINT ||
189-
field_type == FieldType::OLAP_FIELD_TYPE_SMALLINT ||
190-
field_type == FieldType::OLAP_FIELD_TYPE_UNSIGNED_TINYINT ||
191-
field_type == FieldType::OLAP_FIELD_TYPE_UNSIGNED_SMALLINT ||
192-
field_type == FieldType::OLAP_FIELD_TYPE_TINYINT ||
193-
field_type == FieldType::OLAP_FIELD_TYPE_DOUBLE ||
194-
field_type == FieldType::OLAP_FIELD_TYPE_FLOAT ||
195-
field_type == FieldType::OLAP_FIELD_TYPE_DATE ||
196-
field_type == FieldType::OLAP_FIELD_TYPE_DATEV2 ||
197-
field_type == FieldType::OLAP_FIELD_TYPE_DATETIME ||
198-
field_type == FieldType::OLAP_FIELD_TYPE_DATETIMEV2 ||
199-
field_type == FieldType::OLAP_FIELD_TYPE_TIMESTAMPTZ ||
200-
field_type == FieldType::OLAP_FIELD_TYPE_LARGEINT ||
201-
field_type == FieldType::OLAP_FIELD_TYPE_DECIMAL ||
202-
field_type == FieldType::OLAP_FIELD_TYPE_DECIMAL32 ||
203-
field_type == FieldType::OLAP_FIELD_TYPE_DECIMAL64 ||
204-
field_type == FieldType::OLAP_FIELD_TYPE_DECIMAL128I ||
205-
field_type == FieldType::OLAP_FIELD_TYPE_DECIMAL256 ||
206-
field_type == FieldType::OLAP_FIELD_TYPE_BOOL ||
207-
field_type == FieldType::OLAP_FIELD_TYPE_IPV4 ||
208-
field_type == FieldType::OLAP_FIELD_TYPE_IPV6;
209-
}
210-
211185
// Util used to get string name of thrift enum item
212186
#define EnumToString(enum_type, index, out) \
213187
do { \

‎be/test/core/data_type/data_type_decimal_test.cpp‎

Lines changed: 0 additions & 170 deletions
Original file line numberDiff line numberDiff line change
@@ -482,176 +482,6 @@ TEST_F(DataTypeDecimalTest, ser_deser) {
482482
test_func(dt_decimal256_3, *column_decimal256_3, USE_CONST_SERDE);
483483
test_func(dt_decimal256_3, *column_decimal256_3, AGGREGATION_2_1_VERSION);
484484
}
485-
// Regression for legacy DecimalV2 segments written by Doris < 2.1.0 (before
486-
// #26572): ColumnMetaPB.precision/frac are absent and default to 0 when read
487-
// back. DataTypeFactory must not pass 0 as the original precision/scale to
488-
// DataTypeDecimalV2, otherwise check_type_precision throws
489-
// "meet invalid precision: real_precision=0".
490-
TEST_F(DataTypeDecimalTest, create_decimalv2_from_legacy_tablet_column) {
491-
// Case 1: legacy segment — precision/frac missing (default 0)
492-
{
493-
TabletColumn col;
494-
col.set_type(FieldType::OLAP_FIELD_TYPE_DECIMAL);
495-
// intentionally do not set precision/frac to mimic old segments
496-
DataTypePtr dt;
497-
EXPECT_NO_THROW(dt = DataTypeFactory::instance().create_data_type(col, false));
498-
ASSERT_NE(dt, nullptr);
499-
EXPECT_EQ(dt->get_primitive_type(), TYPE_DECIMALV2);
500-
const auto* decv2 = assert_cast<const DataTypeDecimalV2*>(dt.get());
501-
EXPECT_EQ(decv2->get_precision(), BeConsts::MAX_DECIMALV2_PRECISION);
502-
EXPECT_EQ(decv2->get_scale(), BeConsts::MAX_DECIMALV2_SCALE);
503-
// When original precision/scale are unknown, get_original_* should fall
504-
// back to the in-memory (27, 9) representation.
505-
EXPECT_EQ(decv2->get_original_precision(), BeConsts::MAX_DECIMALV2_PRECISION);
506-
EXPECT_EQ(decv2->get_original_scale(), BeConsts::MAX_DECIMALV2_SCALE);
507-
}
508-
// Case 2: new segment — precision/frac are persisted (e.g. decimal(26,6))
509-
{
510-
TabletColumn col;
511-
col.set_type(FieldType::OLAP_FIELD_TYPE_DECIMAL);
512-
col.set_precision(26);
513-
col.set_frac(6);
514-
DataTypePtr dt;
515-
EXPECT_NO_THROW(dt = DataTypeFactory::instance().create_data_type(col, false));
516-
ASSERT_NE(dt, nullptr);
517-
EXPECT_EQ(dt->get_primitive_type(), TYPE_DECIMALV2);
518-
const auto* decv2 = assert_cast<const DataTypeDecimalV2*>(dt.get());
519-
EXPECT_EQ(decv2->get_precision(), BeConsts::MAX_DECIMALV2_PRECISION);
520-
EXPECT_EQ(decv2->get_scale(), BeConsts::MAX_DECIMALV2_SCALE);
521-
EXPECT_EQ(decv2->get_original_precision(), 26U);
522-
EXPECT_EQ(decv2->get_original_scale(), 6U);
523-
}
524-
// Case 3: same regression via the segment-read path that consumes
525-
// ColumnMetaPB directly (this is the exact code path that broke after
526-
// PR #35222 when reading old DecimalV2 segments).
527-
{
528-
segment_v2::ColumnMetaPB meta;
529-
meta.set_type(static_cast<int>(FieldType::OLAP_FIELD_TYPE_DECIMAL));
530-
// precision/frac intentionally unset -> default 0
531-
DataTypePtr dt;
532-
EXPECT_NO_THROW(dt = DataTypeFactory::instance().create_data_type(meta));
533-
ASSERT_NE(dt, nullptr);
534-
EXPECT_EQ(dt->get_primitive_type(), TYPE_DECIMALV2);
535-
const auto* decv2 = assert_cast<const DataTypeDecimalV2*>(dt.get());
536-
EXPECT_EQ(decv2->get_precision(), BeConsts::MAX_DECIMALV2_PRECISION);
537-
EXPECT_EQ(decv2->get_scale(), BeConsts::MAX_DECIMALV2_SCALE);
538-
EXPECT_EQ(decv2->get_original_precision(), BeConsts::MAX_DECIMALV2_PRECISION);
539-
EXPECT_EQ(decv2->get_original_scale(), BeConsts::MAX_DECIMALV2_SCALE);
540-
}
541-
}
542-
543-
// Regression for complex types wrapping legacy DecimalV2 children whose
544-
// segment ColumnMetaPB.precision/frac default to 0. The fix in
545-
// DataTypeFactory::create_data_type(segment_v2::ColumnMetaPB&) must
546-
// propagate through Array / Map / Struct / AggState recursively.
547-
TEST_F(DataTypeDecimalTest, create_complex_types_with_legacy_decimalv2) {
548-
auto make_legacy_decv2_meta = []() {
549-
segment_v2::ColumnMetaPB child;
550-
child.set_type(static_cast<int>(FieldType::OLAP_FIELD_TYPE_DECIMAL));
551-
// precision/frac intentionally unset -> default 0
552-
return child;
553-
};
554-
auto unwrap_nullable = [](const DataTypePtr& dt) -> DataTypePtr {
555-
if (dt && dt->is_nullable()) {
556-
return assert_cast<const DataTypeNullable*>(dt.get())->get_nested_type();
557-
}
558-
return dt;
559-
};
560-
561-
// Array<DecimalV2>
562-
{
563-
segment_v2::ColumnMetaPB meta;
564-
meta.set_type(static_cast<int>(FieldType::OLAP_FIELD_TYPE_ARRAY));
565-
*meta.add_children_columns() = make_legacy_decv2_meta();
566-
DataTypePtr dt;
567-
EXPECT_NO_THROW(dt = DataTypeFactory::instance().create_data_type(meta));
568-
ASSERT_NE(dt, nullptr);
569-
EXPECT_EQ(dt->get_primitive_type(), TYPE_ARRAY);
570-
const auto* arr = assert_cast<const DataTypeArray*>(dt.get());
571-
auto elem = unwrap_nullable(arr->get_nested_type());
572-
EXPECT_EQ(elem->get_primitive_type(), TYPE_DECIMALV2);
573-
const auto* decv2 = assert_cast<const DataTypeDecimalV2*>(elem.get());
574-
EXPECT_EQ(decv2->get_original_precision(), BeConsts::MAX_DECIMALV2_PRECISION);
575-
EXPECT_EQ(decv2->get_original_scale(), BeConsts::MAX_DECIMALV2_SCALE);
576-
}
577-
578-
// Map<Int, DecimalV2>
579-
{
580-
segment_v2::ColumnMetaPB meta;
581-
meta.set_type(static_cast<int>(FieldType::OLAP_FIELD_TYPE_MAP));
582-
auto* k = meta.add_children_columns();
583-
k->set_type(static_cast<int>(FieldType::OLAP_FIELD_TYPE_INT));
584-
*meta.add_children_columns() = make_legacy_decv2_meta();
585-
DataTypePtr dt;
586-
EXPECT_NO_THROW(dt = DataTypeFactory::instance().create_data_type(meta));
587-
ASSERT_NE(dt, nullptr);
588-
EXPECT_EQ(dt->get_primitive_type(), TYPE_MAP);
589-
const auto* m = assert_cast<const DataTypeMap*>(dt.get());
590-
EXPECT_EQ(unwrap_nullable(m->get_key_type())->get_primitive_type(), TYPE_INT);
591-
auto v = unwrap_nullable(m->get_value_type());
592-
EXPECT_EQ(v->get_primitive_type(), TYPE_DECIMALV2);
593-
const auto* decv2 = assert_cast<const DataTypeDecimalV2*>(v.get());
594-
EXPECT_EQ(decv2->get_original_precision(), BeConsts::MAX_DECIMALV2_PRECISION);
595-
}
596-
597-
// Struct<DecimalV2, Int>
598-
{
599-
segment_v2::ColumnMetaPB meta;
600-
meta.set_type(static_cast<int>(FieldType::OLAP_FIELD_TYPE_STRUCT));
601-
*meta.add_children_columns() = make_legacy_decv2_meta();
602-
auto* second = meta.add_children_columns();
603-
second->set_type(static_cast<int>(FieldType::OLAP_FIELD_TYPE_INT));
604-
DataTypePtr dt;
605-
EXPECT_NO_THROW(dt = DataTypeFactory::instance().create_data_type(meta));
606-
ASSERT_NE(dt, nullptr);
607-
EXPECT_EQ(dt->get_primitive_type(), TYPE_STRUCT);
608-
const auto* s = assert_cast<const DataTypeStruct*>(dt.get());
609-
ASSERT_EQ(s->get_elements().size(), 2u);
610-
EXPECT_EQ(unwrap_nullable(s->get_element(0))->get_primitive_type(), TYPE_DECIMALV2);
611-
EXPECT_EQ(unwrap_nullable(s->get_element(1))->get_primitive_type(), TYPE_INT);
612-
}
613-
614-
// Nested: Array<Map<Int, DecimalV2>>
615-
{
616-
segment_v2::ColumnMetaPB meta;
617-
meta.set_type(static_cast<int>(FieldType::OLAP_FIELD_TYPE_ARRAY));
618-
auto* map_child = meta.add_children_columns();
619-
map_child->set_type(static_cast<int>(FieldType::OLAP_FIELD_TYPE_MAP));
620-
auto* k = map_child->add_children_columns();
621-
k->set_type(static_cast<int>(FieldType::OLAP_FIELD_TYPE_INT));
622-
*map_child->add_children_columns() = make_legacy_decv2_meta();
623-
DataTypePtr dt;
624-
EXPECT_NO_THROW(dt = DataTypeFactory::instance().create_data_type(meta));
625-
ASSERT_NE(dt, nullptr);
626-
EXPECT_EQ(dt->get_primitive_type(), TYPE_ARRAY);
627-
auto inner =
628-
unwrap_nullable(assert_cast<const DataTypeArray*>(dt.get())->get_nested_type());
629-
EXPECT_EQ(inner->get_primitive_type(), TYPE_MAP);
630-
auto v = unwrap_nullable(assert_cast<const DataTypeMap*>(inner.get())->get_value_type());
631-
EXPECT_EQ(v->get_primitive_type(), TYPE_DECIMALV2);
632-
}
633-
634-
// AggState (count) with legacy DecimalV2 sub-type
635-
{
636-
segment_v2::ColumnMetaPB meta;
637-
meta.set_type(static_cast<int>(FieldType::OLAP_FIELD_TYPE_AGG_STATE));
638-
meta.set_function_name("count");
639-
meta.set_result_is_nullable(false);
640-
meta.set_be_exec_version(BeExecVersionManager::get_newest_version());
641-
*meta.add_children_columns() = make_legacy_decv2_meta();
642-
DataTypePtr dt;
643-
EXPECT_NO_THROW(dt = DataTypeFactory::instance().create_data_type(meta));
644-
ASSERT_NE(dt, nullptr);
645-
EXPECT_EQ(dt->get_primitive_type(), TYPE_AGG_STATE);
646-
const auto* agg = assert_cast<const DataTypeAggState*>(dt.get());
647-
ASSERT_EQ(agg->get_sub_types().size(), 1u);
648-
auto sub = unwrap_nullable(agg->get_sub_types()[0]);
649-
EXPECT_EQ(sub->get_primitive_type(), TYPE_DECIMALV2);
650-
const auto* decv2 = assert_cast<const DataTypeDecimalV2*>(sub.get());
651-
EXPECT_EQ(decv2->get_original_precision(), BeConsts::MAX_DECIMALV2_PRECISION);
652-
EXPECT_EQ(decv2->get_original_scale(), BeConsts::MAX_DECIMALV2_SCALE);
653-
}
654-
}
655485

656486
TEST_F(DataTypeDecimalTest, to_pb_column_meta) {
657487
auto test_func = [](auto dt, PGenericType_TypeId expected_type) {

0 commit comments

Comments
 (0)