Skip to content

Commit 0682bfa

Browse files
wesnelwesnel
authored
fix(bigtable): Allow GC updates on emulated aggregate column family (#11499)
1 parent b89dd8e commit 0682bfa

File tree

2 files changed

+342
-8
lines changed

2 files changed

+342
-8
lines changed

‎bigtable/bttest/inmem.go‎

Lines changed: 31 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -334,20 +334,43 @@ func (s *server) ModifyColumnFamilies(ctx context.Context, req *btapb.ModifyColu
334334
})
335335
} else if modify := mod.GetUpdate(); modify != nil {
336336
newcf := newColumnFamily(req.Name+"/columnFamilies/"+mod.Id, 0, modify)
337+
updateMask := mod.GetUpdateMask()
338+
paths := updateMask.GetPaths()
339+
337340
cf, ok := tbl.families[mod.Id]
338341
if !ok {
339342
return nil, fmt.Errorf("no such family %q", mod.Id)
340343
}
341-
if cf.valueType != nil {
342-
_, isOldAggregateType := cf.valueType.Kind.(*btapb.Type_AggregateType)
343-
if isOldAggregateType && cf.valueType != newcf.valueType {
344-
return nil, status.Errorf(codes.InvalidArgument, "Immutable fields 'value_type.aggregate_type' cannot be updated")
345-
}
344+
345+
var utr *btapb.ColumnFamily
346+
if len(paths) > 0 &&
347+
!updateMask.IsValid(utr) {
348+
return nil, status.Errorf(codes.InvalidArgument,
349+
"incorrect path in UpdateMask; got: %v",
350+
updateMask)
346351
}
347352

348-
// assume that we ALWAYS want to replace by the new setting
349-
// we may need partial update through
350-
tbl.families[mod.Id] = newcf
353+
if len(paths) == 0 {
354+
// Assume that the update is only modifying the GC policy.
355+
cf.gcRule = newcf.gcRule
356+
}
357+
358+
for _, path := range paths {
359+
switch path {
360+
case "value_type":
361+
if cf.valueType != nil &&
362+
cf.valueType.GetAggregateType() != nil {
363+
// The existing column family is an aggregate type, and the update
364+
// is attempting to modify its immutable type.
365+
return nil, status.Errorf(codes.InvalidArgument,
366+
"Immutable fields 'value_type.aggregate_type' cannot be updated")
367+
}
368+
369+
cf.valueType = newcf.valueType
370+
case "gc_rule":
371+
cf.gcRule = newcf.gcRule
372+
}
373+
}
351374
}
352375
}
353376

‎bigtable/bttest/inmem_test.go‎

Lines changed: 311 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -42,6 +42,7 @@ import (
4242
"google.golang.org/protobuf/proto"
4343
"google.golang.org/protobuf/testing/protocmp"
4444
"google.golang.org/protobuf/types/known/durationpb"
45+
"google.golang.org/protobuf/types/known/fieldmaskpb"
4546
"google.golang.org/protobuf/types/known/wrapperspb"
4647
)
4748

@@ -2711,3 +2712,313 @@ func TestAuthorizedViewApis(t *testing.T) {
27112712
t.Fatalf("Failed to error %s", err)
27122713
}
27132714
}
2715+
2716+
func TestUpdateGCPolicyOnAggregateColumnFamily(t *testing.T) {
2717+
ctx := context.Background()
2718+
2719+
s := &server{
2720+
tables: make(map[string]*table),
2721+
}
2722+
2723+
tblInfo, err := s.CreateTable(ctx, &btapb.CreateTableRequest{
2724+
Parent: "cluster",
2725+
TableId: "t",
2726+
Table: &btapb.Table{
2727+
ColumnFamilies: map[string]*btapb.ColumnFamily{
2728+
"sum": {
2729+
ValueType: &btapb.Type{
2730+
Kind: &btapb.Type_AggregateType{
2731+
AggregateType: &btapb.Type_Aggregate{
2732+
InputType: &btapb.Type{
2733+
Kind: &btapb.Type_Int64Type{},
2734+
},
2735+
Aggregator: &btapb.Type_Aggregate_Sum_{
2736+
Sum: &btapb.Type_Aggregate_Sum{},
2737+
},
2738+
},
2739+
},
2740+
},
2741+
},
2742+
},
2743+
},
2744+
})
2745+
if err != nil {
2746+
t.Fatal(err)
2747+
}
2748+
2749+
if tblInfo.ColumnFamilies["sum"].
2750+
GetValueType().
2751+
GetAggregateType().
2752+
GetSum() == nil {
2753+
t.Fatal("Unexpected aggregate column family type at start of test")
2754+
}
2755+
2756+
if tblInfo.ColumnFamilies["sum"].
2757+
GetGcRule().
2758+
GetMaxNumVersions() == 1 {
2759+
t.Fatal("Unexpected GC policy state at start of test")
2760+
}
2761+
2762+
tblInfo, err = s.ModifyColumnFamilies(ctx, &btapb.ModifyColumnFamiliesRequest{
2763+
Name: tblInfo.Name,
2764+
Modifications: []*btapb.ModifyColumnFamiliesRequest_Modification{
2765+
{
2766+
Id: "sum",
2767+
// UpdateMask intentionally left empty, which the server will
2768+
// implicitly interpret as a gc_rule update.
2769+
Mod: &btapb.ModifyColumnFamiliesRequest_Modification_Update{
2770+
Update: &btapb.ColumnFamily{
2771+
GcRule: &btapb.GcRule{
2772+
Rule: &btapb.GcRule_MaxNumVersions{
2773+
MaxNumVersions: 1,
2774+
},
2775+
},
2776+
// HACK: Intentionally include an invalid type
2777+
// update, which should be ignored since it isn't
2778+
// present in the UpdateMask.
2779+
ValueType: &btapb.Type{
2780+
Kind: &btapb.Type_AggregateType{
2781+
AggregateType: &btapb.Type_Aggregate{
2782+
InputType: &btapb.Type{
2783+
Kind: &btapb.Type_Int64Type{},
2784+
},
2785+
Aggregator: &btapb.Type_Aggregate_Max_{
2786+
Max: &btapb.Type_Aggregate_Max{},
2787+
},
2788+
},
2789+
},
2790+
},
2791+
},
2792+
},
2793+
},
2794+
},
2795+
})
2796+
if err != nil {
2797+
t.Fatal(err)
2798+
}
2799+
2800+
if tblInfo.ColumnFamilies["sum"].
2801+
GetValueType().
2802+
GetAggregateType().
2803+
GetSum() == nil {
2804+
t.Fatal("Aggregate type was updated when it should not have been")
2805+
}
2806+
2807+
if tblInfo.ColumnFamilies["sum"].
2808+
GetGcRule().
2809+
GetMaxNumVersions() != 1 {
2810+
t.Fatal("GC policy was not updated when it should have been")
2811+
}
2812+
2813+
tblInfo, err = s.ModifyColumnFamilies(ctx, &btapb.ModifyColumnFamiliesRequest{
2814+
Name: tblInfo.Name,
2815+
Modifications: []*btapb.ModifyColumnFamiliesRequest_Modification{
2816+
{
2817+
Id: "sum",
2818+
// Including UpdateMask in the request this time.
2819+
UpdateMask: &fieldmaskpb.FieldMask{
2820+
Paths: []string{"gc_rule"},
2821+
},
2822+
Mod: &btapb.ModifyColumnFamiliesRequest_Modification_Update{
2823+
Update: &btapb.ColumnFamily{
2824+
GcRule: &btapb.GcRule{
2825+
Rule: &btapb.GcRule_MaxNumVersions{
2826+
MaxNumVersions: 2,
2827+
},
2828+
},
2829+
// HACK: Intentionally including an invalid type
2830+
// update, which should be ignored since it isn't
2831+
// present in the UpdateMask.
2832+
ValueType: &btapb.Type{
2833+
Kind: &btapb.Type_AggregateType{
2834+
AggregateType: &btapb.Type_Aggregate{
2835+
InputType: &btapb.Type{
2836+
Kind: &btapb.Type_Int64Type{},
2837+
},
2838+
Aggregator: &btapb.Type_Aggregate_Max_{
2839+
Max: &btapb.Type_Aggregate_Max{},
2840+
},
2841+
},
2842+
},
2843+
},
2844+
},
2845+
},
2846+
},
2847+
},
2848+
})
2849+
if err != nil {
2850+
t.Fatal(err)
2851+
}
2852+
2853+
if tblInfo.ColumnFamilies["sum"].
2854+
GetValueType().
2855+
GetAggregateType().
2856+
GetSum() == nil {
2857+
t.Fatal("Aggregate type was updated when it should not have been")
2858+
}
2859+
2860+
if tblInfo.ColumnFamilies["sum"].
2861+
GetGcRule().
2862+
GetMaxNumVersions() != 2 {
2863+
t.Fatal("GC policy was not updated when it should have been")
2864+
}
2865+
}
2866+
2867+
func TestCannotUpdateTypeOfAggregateColumnFamily(t *testing.T) {
2868+
ctx := context.Background()
2869+
2870+
s := &server{
2871+
tables: make(map[string]*table),
2872+
}
2873+
2874+
tblInfo, err := s.CreateTable(ctx, &btapb.CreateTableRequest{
2875+
Parent: "cluster",
2876+
TableId: "t",
2877+
Table: &btapb.Table{
2878+
ColumnFamilies: map[string]*btapb.ColumnFamily{
2879+
"sum": {
2880+
ValueType: &btapb.Type{
2881+
Kind: &btapb.Type_AggregateType{
2882+
AggregateType: &btapb.Type_Aggregate{
2883+
InputType: &btapb.Type{
2884+
Kind: &btapb.Type_Int64Type{},
2885+
},
2886+
Aggregator: &btapb.Type_Aggregate_Sum_{
2887+
Sum: &btapb.Type_Aggregate_Sum{},
2888+
},
2889+
},
2890+
},
2891+
},
2892+
},
2893+
},
2894+
},
2895+
})
2896+
if err != nil {
2897+
t.Fatal(err)
2898+
}
2899+
2900+
if tblInfo.ColumnFamilies["sum"].
2901+
GetValueType().
2902+
GetAggregateType().
2903+
GetSum() == nil {
2904+
t.Fatal("Unexpected aggregate column family type at start of test")
2905+
}
2906+
2907+
_, err = s.ModifyColumnFamilies(ctx, &btapb.ModifyColumnFamiliesRequest{
2908+
Name: tblInfo.Name,
2909+
Modifications: []*btapb.ModifyColumnFamiliesRequest_Modification{
2910+
{
2911+
Id: "sum",
2912+
UpdateMask: &fieldmaskpb.FieldMask{
2913+
Paths: []string{"value_type"},
2914+
},
2915+
Mod: &btapb.ModifyColumnFamiliesRequest_Modification_Update{
2916+
Update: &btapb.ColumnFamily{
2917+
ValueType: &btapb.Type{
2918+
Kind: &btapb.Type_AggregateType{
2919+
AggregateType: &btapb.Type_Aggregate{
2920+
InputType: &btapb.Type{
2921+
Kind: &btapb.Type_Int64Type{},
2922+
},
2923+
Aggregator: &btapb.Type_Aggregate_Max_{
2924+
Max: &btapb.Type_Aggregate_Max{},
2925+
},
2926+
},
2927+
},
2928+
},
2929+
},
2930+
},
2931+
},
2932+
},
2933+
})
2934+
if err == nil {
2935+
t.Fatal("ModifyColumnFamilies was supposed to return an error, but it did not")
2936+
}
2937+
2938+
tblInfo, err = s.GetTable(ctx, &btapb.GetTableRequest{Name: tblInfo.Name})
2939+
if err != nil {
2940+
t.Fatal(err)
2941+
}
2942+
2943+
if tblInfo.ColumnFamilies["sum"].
2944+
GetValueType().
2945+
GetAggregateType().
2946+
GetSum() == nil {
2947+
t.Fatal("Aggregate type was updated when it should not have been")
2948+
}
2949+
}
2950+
2951+
func TestInvalidUpdateMaskInColumnFamilyUpdate(t *testing.T) {
2952+
ctx := context.Background()
2953+
2954+
s := &server{
2955+
tables: make(map[string]*table),
2956+
}
2957+
2958+
tblInfo, err := s.CreateTable(ctx, &btapb.CreateTableRequest{
2959+
Parent: "cluster",
2960+
TableId: "t",
2961+
Table: &btapb.Table{
2962+
ColumnFamilies: map[string]*btapb.ColumnFamily{
2963+
"sum": {
2964+
ValueType: &btapb.Type{
2965+
Kind: &btapb.Type_AggregateType{
2966+
AggregateType: &btapb.Type_Aggregate{
2967+
InputType: &btapb.Type{
2968+
Kind: &btapb.Type_Int64Type{},
2969+
},
2970+
Aggregator: &btapb.Type_Aggregate_Sum_{
2971+
Sum: &btapb.Type_Aggregate_Sum{},
2972+
},
2973+
},
2974+
},
2975+
},
2976+
},
2977+
},
2978+
},
2979+
})
2980+
if err != nil {
2981+
t.Fatal(err)
2982+
}
2983+
2984+
if tblInfo.ColumnFamilies["sum"].
2985+
GetGcRule().
2986+
GetMaxNumVersions() == 1 {
2987+
t.Fatal("Unexpected GC policy state at start of test")
2988+
}
2989+
2990+
_, err = s.ModifyColumnFamilies(ctx, &btapb.ModifyColumnFamiliesRequest{
2991+
Name: tblInfo.Name,
2992+
Modifications: []*btapb.ModifyColumnFamiliesRequest_Modification{
2993+
{
2994+
Id: "sum",
2995+
UpdateMask: &fieldmaskpb.FieldMask{
2996+
Paths: []string{"bad", "gc_rule"},
2997+
},
2998+
Mod: &btapb.ModifyColumnFamiliesRequest_Modification_Update{
2999+
Update: &btapb.ColumnFamily{
3000+
GcRule: &btapb.GcRule{
3001+
Rule: &btapb.GcRule_MaxNumVersions{
3002+
MaxNumVersions: 1,
3003+
},
3004+
},
3005+
},
3006+
},
3007+
},
3008+
},
3009+
})
3010+
if err == nil {
3011+
t.Fatal("ModifyColumnFamilies was supposed to return an error, but it did not")
3012+
}
3013+
3014+
tblInfo, err = s.GetTable(ctx, &btapb.GetTableRequest{Name: tblInfo.Name})
3015+
if err != nil {
3016+
t.Fatal(err)
3017+
}
3018+
3019+
if tblInfo.ColumnFamilies["sum"].
3020+
GetGcRule().
3021+
GetMaxNumVersions() == 1 {
3022+
t.Fatal("GC policy was updated when it should not have been")
3023+
}
3024+
}

0 commit comments

Comments
 (0)