Skip to content

Commit 2d3495d

Browse files
XuPeng-SHXuPeng-SH
authored
do revert 22711 (#22740)
### **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 #22523 ## What this PR does / why we need it: revert due to TPCH q19 performance degradation ___ ### **PR Type** Bug fix ___ ### **Description** - Revert decimal64/decimal128 conversion fixes causing TPCH q19 performance degradation - Remove proper scale handling in stats.go decimal conversions - Remove decimal type cases from shuffle range type checks - Remove comprehensive decimal conversion test coverage ___ ### Diagram Walkthrough ```mermaid flowchart LR A["Decimal Conversion Fixes"] -->|Revert| B["Direct float64 Cast"] C["Scale-aware Conversion"] -->|Remove| D["Hardcoded Scale 0"] E["Decimal Type Handling"] -->|Remove| F["Shuffle Range Logic"] G["Test Coverage"] -->|Delete| H["Decimal Test Cases"] ``` <details> <summary><h3> File Walkthrough</h3></summary> <table><thead><tr><th></th><th align="left">Relevant files</th></tr></thead><tbody><tr><td><strong>Bug fix</strong></td><td><table> <tr> <td> <details> <summary><strong>stats.go</strong><dd><code>Revert decimal conversion to direct float64 cast</code>&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; </dd></summary> <hr> pkg/sql/plan/stats.go <ul><li>Reverted decimal64 conversion from scale-aware <code>Decimal64ToFloat64</code> to <br>direct <code>float64</code> cast<br> <li> Reverted decimal128 conversion from scale-aware <code>Decimal128ToFloat64</code> to <br>hardcoded scale 0<br> <li> Removed intermediate variable assignments for decimal decoding<br> <li> Restored simpler but less accurate decimal-to-float conversion logic</ul> </details> </td> <td><a href="https://github.com/matrixorigin/matrixone/pull/22740/files#diff-3b3d55fa9884dcf8980f90043a05b26a04d0153ae89fcf032cec998752c0cafa">+6/-13</a>&nbsp; &nbsp; </td> </tr> <tr> <td> <details> <summary><strong>stats.go</strong><dd><code>Remove decimal type handling from stats conversion</code>&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; </dd></summary> <hr> pkg/vm/engine/disttae/stats.go <ul><li>Removed decimal64 and decimal128 cases from <code>getMinMaxValueByFloat64</code> <br>function<br> <li> Removed scale-aware conversion using <code>Decimal64ToFloat64</code> and <br><code>Decimal128ToFloat64</code><br> <li> Removed decimal types from shuffle range type switch cases in two <br>locations<br> <li> Simplified type handling by excluding decimal types from numeric range <br>processing</ul> </details> </td> <td><a href="https://github.com/matrixorigin/matrixone/pull/22740/files#diff-d0f8ce84135a062e5992dcb3d1175993ee396beae48de126969255cd9240d02b">+2/-10</a>&nbsp; &nbsp; </td> </tr> </table></td></tr><tr><td><strong>Tests</strong></td><td><table> <tr> <td> <details> <summary><strong>stats_test.go</strong><dd><code>Remove decimal conversion test coverage</code>&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; </dd></summary> <hr> pkg/sql/plan/stats_test.go <ul><li>Removed all three decimal conversion test functions<br> <li> Deleted <code>TestUpdateStatsInfo_Decimal64_NegativeValues</code> test<br> <li> Deleted <code>TestUpdateStatsInfo_Decimal128_NegativeValues</code> test<br> <li> Deleted <code>TestUpdateStatsInfo_Decimal_DifferentScales</code> test<br> <li> Removed test imports for types, pb, and testify/require</ul> </details> </td> <td><a href="https://github.com/matrixorigin/matrixone/pull/22740/files#diff-99727172c3e9bb9536487e724b5b68d8d02bfbf514d145e062222952cdde78da">+0/-246</a>&nbsp; </td> </tr> <tr> <td> <details> <summary><strong>stats_test.go</strong><dd><code>Remove decimal conversion test function</code>&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; </dd></summary> <hr> pkg/vm/engine/disttae/stats_test.go <ul><li>Removed entire <code>TestGetMinMaxValueByFloat64_Decimal</code> test function<br> <li> Deleted comprehensive test coverage for decimal64 and decimal128 <br>conversions<br> <li> Removed tests for positive/negative values, different scales, and <br>min/max relationships<br> <li> Removed test cases validating correct handling of two's complement <br>representation</ul> </details> </td> <td><a href="https://github.com/matrixorigin/matrixone/pull/22740/files#diff-5a180d6cc7bed3d03a8f45d2ceb0a95a41bcb192162481f0dd52d4db5f119896">+0/-120</a>&nbsp; </td> </tr> </table></td></tr></tr></tbody></table> </details> ___
1 parent 22e88af commit 2d3495d

File tree

4 files changed

+8
-389
lines changed

4 files changed

+8
-389
lines changed

‎pkg/sql/plan/stats.go‎

Lines changed: 6 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -350,20 +350,13 @@ func UpdateStatsInfo(info *InfoFromZoneMap, tableDef *plan.TableDef, s *pb.Stats
350350
s.MinValMap[colName] = float64(ByteSliceToUint64(info.ColumnZMs[i].GetMinBuf()))
351351
s.MaxValMap[colName] = float64(ByteSliceToUint64(info.ColumnZMs[i].GetMaxBuf()))
352352
case types.T_decimal64:
353-
// Fix: Use Decimal64ToFloat64 with proper scale to handle negative values correctly
354-
// Direct cast to float64 treats negative values (stored as two's complement) as large positive numbers
355-
scale := coldef.Typ.Scale
356-
minDec := types.DecodeDecimal64(info.ColumnZMs[i].GetMinBuf())
357-
maxDec := types.DecodeDecimal64(info.ColumnZMs[i].GetMaxBuf())
358-
s.MinValMap[colName] = types.Decimal64ToFloat64(minDec, scale)
359-
s.MaxValMap[colName] = types.Decimal64ToFloat64(maxDec, scale)
353+
s.MinValMap[colName] = float64(types.DecodeDecimal64(info.ColumnZMs[i].GetMinBuf()))
354+
s.MaxValMap[colName] = float64(types.DecodeDecimal64(info.ColumnZMs[i].GetMaxBuf()))
360355
case types.T_decimal128:
361-
// Fix: Use actual scale from column definition instead of hardcoded 0
362-
scale := coldef.Typ.Scale
363-
minDec := types.DecodeDecimal128(info.ColumnZMs[i].GetMinBuf())
364-
maxDec := types.DecodeDecimal128(info.ColumnZMs[i].GetMaxBuf())
365-
s.MinValMap[colName] = types.Decimal128ToFloat64(minDec, scale)
366-
s.MaxValMap[colName] = types.Decimal128ToFloat64(maxDec, scale)
356+
val := types.DecodeDecimal128(info.ColumnZMs[i].GetMinBuf())
357+
s.MinValMap[colName] = float64(types.Decimal128ToFloat64(val, 0))
358+
val = types.DecodeDecimal128(info.ColumnZMs[i].GetMaxBuf())
359+
s.MaxValMap[colName] = float64(types.Decimal128ToFloat64(val, 0))
367360
}
368361

369362
if info.ShuffleRanges[i] != nil {

‎pkg/sql/plan/stats_test.go‎

Lines changed: 0 additions & 246 deletions
Original file line numberDiff line numberDiff line change
@@ -18,255 +18,9 @@ import (
1818
"testing"
1919

2020
"github.com/matrixorigin/matrixone/pkg/catalog"
21-
"github.com/matrixorigin/matrixone/pkg/container/types"
2221
"github.com/matrixorigin/matrixone/pkg/pb/plan"
23-
pb "github.com/matrixorigin/matrixone/pkg/pb/statsinfo"
24-
index2 "github.com/matrixorigin/matrixone/pkg/vm/engine/tae/index"
25-
"github.com/stretchr/testify/require"
2622
)
2723

28-
// TestUpdateStatsInfo_Decimal64_NegativeValues tests that negative decimal64 values
29-
// are correctly converted to float64 for statistics
30-
func TestUpdateStatsInfo_Decimal64_NegativeValues(t *testing.T) {
31-
// Test case: DECIMAL(10, 2) with negative values
32-
// Example: -123.45 and 456.78
33-
scale := int32(2)
34-
35-
// Create negative decimal: -123.45
36-
negativeValue, err := types.Decimal64FromFloat64(-123.45, 10, scale)
37-
require.NoError(t, err)
38-
39-
// Create positive decimal: 456.78
40-
positiveValue, err := types.Decimal64FromFloat64(456.78, 10, scale)
41-
require.NoError(t, err)
42-
43-
// Create zonemap with negative min and positive max
44-
zm := index2.NewZM(types.T_decimal64, scale)
45-
minBuf := types.EncodeDecimal64(&negativeValue)
46-
maxBuf := types.EncodeDecimal64(&positiveValue)
47-
index2.UpdateZM(zm, minBuf)
48-
index2.UpdateZM(zm, maxBuf)
49-
50-
// Create table definition with decimal64 column
51-
tableDef := &plan.TableDef{
52-
Name: "test_table",
53-
Cols: []*plan.ColDef{
54-
{
55-
Name: "balance",
56-
Typ: plan.Type{
57-
Id: int32(types.T_decimal64),
58-
Scale: scale,
59-
Width: 10,
60-
},
61-
},
62-
{
63-
Name: catalog.Row_ID,
64-
},
65-
},
66-
}
67-
68-
// Create InfoFromZoneMap
69-
info := &InfoFromZoneMap{
70-
ColumnZMs: []index2.ZM{zm},
71-
DataTypes: []types.Type{types.New(types.T_decimal64, 10, scale)},
72-
ColumnNDVs: []float64{2},
73-
NullCnts: []int64{0},
74-
ColumnSize: []int64{8},
75-
ShuffleRanges: []*pb.ShuffleRange{nil},
76-
}
77-
78-
// Create StatsInfo
79-
statsInfo := &pb.StatsInfo{
80-
MinValMap: make(map[string]float64),
81-
MaxValMap: make(map[string]float64),
82-
NdvMap: make(map[string]float64),
83-
DataTypeMap: make(map[string]uint64),
84-
NullCntMap: make(map[string]uint64),
85-
SizeMap: make(map[string]uint64),
86-
ShuffleRangeMap: make(map[string]*pb.ShuffleRange),
87-
}
88-
89-
// Call UpdateStatsInfo
90-
UpdateStatsInfo(info, tableDef, statsInfo)
91-
92-
// Verify results
93-
minVal := statsInfo.MinValMap["balance"]
94-
maxVal := statsInfo.MaxValMap["balance"]
95-
96-
// The key assertion: min should be less than max
97-
require.Less(t, minVal, maxVal, "Min value should be less than max value")
98-
99-
// Verify approximate values (allowing for floating point precision)
100-
require.InDelta(t, -123.45, minVal, 0.01, "Min value should be approximately -123.45")
101-
require.InDelta(t, 456.78, maxVal, 0.01, "Max value should be approximately 456.78")
102-
103-
// Before the fix, minVal would have been a huge positive number like 18446744073514074000
104-
// This check ensures that didn't happen
105-
require.Greater(t, minVal, -1000.0, "Min value should not be an extremely large number")
106-
require.Less(t, minVal, 0.0, "Min value should be negative")
107-
}
108-
109-
// TestUpdateStatsInfo_Decimal128_NegativeValues tests that negative decimal128 values
110-
// are correctly converted with proper scale
111-
func TestUpdateStatsInfo_Decimal128_NegativeValues(t *testing.T) {
112-
// Test case: DECIMAL(20, 4) with negative values
113-
scale := int32(4)
114-
115-
// Create negative decimal: -9876543210.1234
116-
negativeValue, err := types.Decimal128FromFloat64(-9876543210.1234, 20, scale)
117-
require.NoError(t, err)
118-
119-
// Create positive decimal: 1234567890.5678
120-
positiveValue, err := types.Decimal128FromFloat64(1234567890.5678, 20, scale)
121-
require.NoError(t, err)
122-
123-
// Create zonemap
124-
zm := index2.NewZM(types.T_decimal128, scale)
125-
minBuf := types.EncodeDecimal128(&negativeValue)
126-
maxBuf := types.EncodeDecimal128(&positiveValue)
127-
index2.UpdateZM(zm, minBuf)
128-
index2.UpdateZM(zm, maxBuf)
129-
130-
// Create table definition
131-
tableDef := &plan.TableDef{
132-
Name: "test_table",
133-
Cols: []*plan.ColDef{
134-
{
135-
Name: "amount",
136-
Typ: plan.Type{
137-
Id: int32(types.T_decimal128),
138-
Scale: scale,
139-
Width: 20,
140-
},
141-
},
142-
{
143-
Name: catalog.Row_ID,
144-
},
145-
},
146-
}
147-
148-
// Create InfoFromZoneMap
149-
info := &InfoFromZoneMap{
150-
ColumnZMs: []index2.ZM{zm},
151-
DataTypes: []types.Type{types.New(types.T_decimal128, 20, scale)},
152-
ColumnNDVs: []float64{2},
153-
NullCnts: []int64{0},
154-
ColumnSize: []int64{16},
155-
ShuffleRanges: []*pb.ShuffleRange{nil},
156-
}
157-
158-
// Create StatsInfo
159-
statsInfo := &pb.StatsInfo{
160-
MinValMap: make(map[string]float64),
161-
MaxValMap: make(map[string]float64),
162-
NdvMap: make(map[string]float64),
163-
DataTypeMap: make(map[string]uint64),
164-
NullCntMap: make(map[string]uint64),
165-
SizeMap: make(map[string]uint64),
166-
ShuffleRangeMap: make(map[string]*pb.ShuffleRange),
167-
}
168-
169-
// Call UpdateStatsInfo
170-
UpdateStatsInfo(info, tableDef, statsInfo)
171-
172-
// Verify results
173-
minVal := statsInfo.MinValMap["amount"]
174-
maxVal := statsInfo.MaxValMap["amount"]
175-
176-
// The key assertion: min should be less than max
177-
require.Less(t, minVal, maxVal, "Min value should be less than max value")
178-
179-
// Verify approximate values
180-
require.InDelta(t, -9876543210.1234, minVal, 0.01, "Min value should be approximately -9876543210.1234")
181-
require.InDelta(t, 1234567890.5678, maxVal, 0.01, "Max value should be approximately 1234567890.5678")
182-
183-
// Ensure min is negative and within reasonable range
184-
require.Less(t, minVal, 0.0, "Min value should be negative")
185-
}
186-
187-
// TestUpdateStatsInfo_Decimal_DifferentScales tests decimal conversion with various scales
188-
func TestUpdateStatsInfo_Decimal_DifferentScales(t *testing.T) {
189-
testCases := []struct {
190-
name string
191-
scale int32
192-
minFloat float64
193-
maxFloat float64
194-
}{
195-
{"scale_0", 0, -100.0, 200.0},
196-
{"scale_2", 2, -99.99, 199.99},
197-
{"scale_4", 4, -1234.5678, 5678.1234},
198-
{"scale_6", 6, -0.123456, 0.987654},
199-
}
200-
201-
for _, tc := range testCases {
202-
t.Run(tc.name, func(t *testing.T) {
203-
// Create decimal values
204-
minDec, err := types.Decimal64FromFloat64(tc.minFloat, 18, tc.scale)
205-
require.NoError(t, err)
206-
207-
maxDec, err := types.Decimal64FromFloat64(tc.maxFloat, 18, tc.scale)
208-
require.NoError(t, err)
209-
210-
// Create zonemap
211-
zm := index2.NewZM(types.T_decimal64, tc.scale)
212-
minBuf := types.EncodeDecimal64(&minDec)
213-
maxBuf := types.EncodeDecimal64(&maxDec)
214-
index2.UpdateZM(zm, minBuf)
215-
index2.UpdateZM(zm, maxBuf)
216-
217-
// Create table definition
218-
tableDef := &plan.TableDef{
219-
Name: "test_table",
220-
Cols: []*plan.ColDef{
221-
{
222-
Name: "value",
223-
Typ: plan.Type{
224-
Id: int32(types.T_decimal64),
225-
Scale: tc.scale,
226-
Width: 18,
227-
},
228-
},
229-
{
230-
Name: catalog.Row_ID,
231-
},
232-
},
233-
}
234-
235-
// Create InfoFromZoneMap
236-
info := &InfoFromZoneMap{
237-
ColumnZMs: []index2.ZM{zm},
238-
DataTypes: []types.Type{types.New(types.T_decimal64, 18, tc.scale)},
239-
ColumnNDVs: []float64{2},
240-
NullCnts: []int64{0},
241-
ColumnSize: []int64{8},
242-
ShuffleRanges: []*pb.ShuffleRange{nil},
243-
}
244-
245-
// Create StatsInfo
246-
statsInfo := &pb.StatsInfo{
247-
MinValMap: make(map[string]float64),
248-
MaxValMap: make(map[string]float64),
249-
NdvMap: make(map[string]float64),
250-
DataTypeMap: make(map[string]uint64),
251-
NullCntMap: make(map[string]uint64),
252-
SizeMap: make(map[string]uint64),
253-
ShuffleRangeMap: make(map[string]*pb.ShuffleRange),
254-
}
255-
256-
// Call UpdateStatsInfo
257-
UpdateStatsInfo(info, tableDef, statsInfo)
258-
259-
// Verify results
260-
minVal := statsInfo.MinValMap["value"]
261-
maxVal := statsInfo.MaxValMap["value"]
262-
263-
require.Less(t, minVal, maxVal, "Min value should be less than max value")
264-
require.InDelta(t, tc.minFloat, minVal, 0.01, "Min value mismatch")
265-
require.InDelta(t, tc.maxFloat, maxVal, 0.01, "Max value mismatch")
266-
})
267-
}
268-
}
269-
27024
func makeQueryWithScan(tableType string, rowsize float64, blockNum int32) *plan.Query {
27125
n := &plan.Node{
27226
NodeType: plan.Node_TABLE_SCAN,

‎pkg/vm/engine/disttae/stats.go‎

Lines changed: 2 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -609,14 +609,6 @@ func getMinMaxValueByFloat64(typ types.Type, buf []byte) float64 {
609609
return float64(types.DecodeTimestamp(buf))
610610
case types.T_datetime:
611611
return float64(types.DecodeDatetime(buf))
612-
case types.T_decimal64:
613-
// Fix: Use Decimal64ToFloat64 to handle negative values correctly
614-
dec := types.DecodeDecimal64(buf)
615-
return types.Decimal64ToFloat64(dec, typ.Scale)
616-
case types.T_decimal128:
617-
// Fix: Use Decimal128ToFloat64 to handle negative values correctly
618-
dec := types.DecodeDecimal128(buf)
619-
return types.Decimal128ToFloat64(dec, typ.Scale)
620612
//case types.T_char, types.T_varchar, types.T_text:
621613
//return float64(plan2.ByteSliceToUint64(buf)), true
622614
default:
@@ -671,7 +663,7 @@ func updateInfoFromZoneMap(
671663
meta.BlockHeader().BFExtent().Length() + objColMeta.Location().Length())
672664
if info.ColumnNDVs[idx] > 100 || info.ColumnNDVs[idx] > 0.1*float64(meta.BlockHeader().Rows()) {
673665
switch info.DataTypes[idx].Oid {
674-
case types.T_int64, types.T_int32, types.T_int16, types.T_uint64, types.T_uint32, types.T_uint16, types.T_time, types.T_timestamp, types.T_date, types.T_datetime, types.T_decimal64, types.T_decimal128:
666+
case types.T_int64, types.T_int32, types.T_int16, types.T_uint64, types.T_uint32, types.T_uint16, types.T_time, types.T_timestamp, types.T_date, types.T_datetime:
675667
info.ShuffleRanges[idx] = plan2.NewShuffleRange(false)
676668
if info.ColumnZMs[idx].IsInited() {
677669
minvalue := getMinMaxValueByFloat64(info.DataTypes[idx], info.ColumnZMs[idx].GetMinBuf())
@@ -717,7 +709,7 @@ func updateInfoFromZoneMap(
717709
info.ColumnSize[idx] += int64(objColMeta.Location().Length())
718710
if info.ShuffleRanges[idx] != nil {
719711
switch info.DataTypes[idx].Oid {
720-
case types.T_int64, types.T_int32, types.T_int16, types.T_uint64, types.T_uint32, types.T_uint16, types.T_time, types.T_timestamp, types.T_date, types.T_datetime, types.T_decimal64, types.T_decimal128:
712+
case types.T_int64, types.T_int32, types.T_int16, types.T_uint64, types.T_uint32, types.T_uint16, types.T_time, types.T_timestamp, types.T_date, types.T_datetime:
721713
minvalue := getMinMaxValueByFloat64(info.DataTypes[idx], zm.GetMinBuf())
722714
maxvalue := getMinMaxValueByFloat64(info.DataTypes[idx], zm.GetMaxBuf())
723715
info.ShuffleRanges[idx].Update(minvalue, maxvalue, int64(meta.BlockHeader().Rows()), int64(objColMeta.NullCnt()))

0 commit comments

Comments
 (0)