feat(BREAKING!): support incell list/map aggregation across duplicate rows with consistency checks by Kybxd · Pull Request #395 · tableauio/tableau

Kybxd · 2026-04-15T11:44:10Z

Summary

This PR introduces a new field property aggregate on incell list / map fields, enabling them to collect elements across vertically (or horizontally) duplicated rows sharing the same primary key into a single aggregated collection. Previously such duplicated rows were required to carry the identical incell value; now users can spread the contents of one logical list/map over multiple rows.

Alongside the new capability, this PR adds strict consistency / uniqueness diagnostics and performs a large refactor of internal/confgen/table_parser.go to unify the aggregation and duplicate-detection code paths.

What's New

1. New field property: `FieldProp.aggregate`

Added a new option in proto/tableau/protobuf/tableau.proto:

message FieldProp {
  // ...
  // If true, this incell list/map field collects elements across vertically
  // or horizontally duplicated rows (same primary key) into a single
  // aggregated collection, instead of requiring all rows to carry the
  // identical value.
  bool aggregate = 21;
}

ExtractMapFieldProp / ExtractListFieldProp now take the field layout and only propagate aggregate when the layout is LAYOUT_INCELL.

2. New error codes

Code	Meaning
E2023	`inconsistent value in aggregated cells` — a non-aggregated incell list/map that appears on duplicated primary-key rows carries a value different from its first presence.
E2028	`duplicate elements in incell keyed-list` — elements inside a single incell keyed-list are not unique.

Both en.yaml / zh.yaml are updated, and ecode_generated.go is regenerated accordingly.

3. Simplified E2005

E2005 is narrowed from "map or keyed-list key not unique" to "map key not unique". Keyed-list duplicate detection now flows through the dedicated E2028 path, making error classification cleaner.

4. Refactor of `table_parser.go`

Large restructuring (~1.5k lines touched) to:

Unify the vertical / horizontal duplicate-row handling behind a single aggregation pipeline.
Consistently apply the new aggregate semantics for incell list/map fields.
Emit E2023 whenever a non-aggregated incell collection is observed to change across duplicated rows, using the first occurrence as the source of truth.
Emit E2028 when the parsed incell keyed-list contains duplicate elements.
Simplify map field handling in document_parser.go / parser.go.

Two small helpers (listValues, mapValues) were added in internal/confgen/util.go to extract values from protoreflect.List / protoreflect.Map for diagnostics.

5. Test coverage

Updated functest proto definitions (vertical_list.proto, horizontal_map.proto, etc.) to annotate incell list/map fields with prop:{aggregate:true} where cross-row aggregation is intended.
Expanded internal/confgen/table_parser_test.go for the new aggregation & consistency paths.
Filled missing Name / Desc columns in several test CSVs, and added the Attr map column to HorizontalStructMap to exercise an incell map aggregation case.

Migration Guide

Add aggregate:true to incell list/map fields to enable vertical aggregation. Scalars, enums, structs and horizontal lists or maps are not allowed to aggregate.

Before

ID
[]uint32
ID
1,2,3
4,5
6

After

ID
[]uint32\|{aggregate:true}
ID
1,2,3
4,5
6

codecov · 2026-04-15T11:46:29Z

Codecov Report

❌ Patch coverage is 72.82609% with 200 lines in your changes missing coverage. Please review.
✅ Project coverage is 73.23%. Comparing base (e7a79d9) to head (39ecbf2).
⚠️ Report is 1 commits behind head on master.

Files with missing lines	Patch %	Lines
internal/confgen/table_parser.go	73.93%	125 Missing and 52 partials ⚠️
internal/confgen/document_parser.go	42.30%	12 Missing and 3 partials ⚠️
internal/confgen/util.go	46.15%	7 Missing ⚠️
internal/localizer/i18n/i18n.go	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #395      +/-   ##
==========================================
- Coverage   73.35%   73.23%   -0.12%     
==========================================
  Files          88       88              
  Lines       11284    11386     +102     
==========================================
+ Hits         8277     8339      +62     
- Misses       2440     2477      +37     
- Partials      567      570       +3

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

wenchy · 2026-04-20T04:12:43Z

+			if field.opts.GetProp().GetAggregate() {
+				presentList := msg.Mutable(field.fd).List()
+				for i := range listValue.List().Len() {
+					newValue := listValue.List().Get(i)
+					if field.opts.GetKey() != "" && field.fd.Kind() != protoreflect.MessageKind {
+						// check duplicate elems for scalar/enum keyed-list
+						for j := range presentList.Len() {
+							elemVal := presentList.Get(j)
+							if elemVal.Equal(newValue) {
+								return false, xerrors.WrapKV(xerrors.E2028(newValue), r.CellDebugKV(colName)...)
+							}
+						}
+					}
+					presentList.Append(newValue)
+				}


list aggregating, just append elems, no need to check uniqueness (if it is not keyed-list)

wellknown messages should also be considered

This is already implemented by current logic.

A better way to do this is to remove field.fd.Kind() != protoreflect.MessageKind check. In this situation, we can also check uniqueness for incell struct keyed-list.

wenchy · 2026-04-20T04:20:09Z

  //
  // See https://github.com/bufbuild/protovalidate.
  string validate_message = 20;
+  // Whether this incell list/map field's value is aggregated vertically.


Same as horizontally, should also be supported.

I don't think it's a good idea to support aggregating a horizontal list vertically. This may look weird:

WeaponID Attr1Name Attr1Value Attr2Name Attr2Value Level UpgradeCost

map<uint32, Weapon> [Attr]string int32 string int32 map<int32, Level> int32

Weapon's ID Attr1's name Attr1's value Attr2's name Attr2's value Weapon's Level Cost to upgrade weapon to next level

1 Attack 100 Defence 200 1 10

1 Crit 30 CritDamage 50 2 20

The design should be concistent and symmetry, Not only by visual.

wenchy · 2026-04-20T07:51:53Z

@@ -1,5 +1,5 @@
 ID,#IGNORE
-"[]int32|{refer:""EquipConf.ID""}",
+"[]int32|{refer:""EquipConf.ID"" aggregate:true}",


This is a serious Breaking Change!

updated to title & description.

wenchy · 2026-04-20T07:52:29Z

@@ -1,5 +1,5 @@
 Target
-[]{union.Target}|{form:FORM_TEXT}
+[]{union.Target}|{form:FORM_TEXT aggregate:true}


why need aggregate:true ?

The list's layout is incell but is used vertically, so need aggregate:true.

wenchy · 2026-04-20T07:54:41Z

+1,,11,110
 2,Orange,20,200


The default behaviour should be: fill the same value as before (just like the same key filled). Please also fix the other cases.

wenchy · 2026-04-20T11:40:07Z

  // See https://github.com/bufbuild/protovalidate.
  string validate_message = 20;
+  // Whether this incell list/map field's value is aggregated vertically.
+  bool aggregate = 21;


merge is a more accurate and intuitive word.

wenchy requested changes Apr 20, 2026

View reviewed changes

Kybxd added 5 commits April 20, 2026 14:35

feat: Add vertical aggregation consistency check and E2023 error

d5c7557

feat: Add aggregate support across parser and proto definitions

d2d321f

feat: Support incell keyed-list duplicate check and error E2028

cbe13b5

refactor: Simplify E2005 error message to "map key not unique"

1dd2687

fix: cr

e786604

Kybxd force-pushed the vertical-aggregation branch from b3ed9b7 to e786604 Compare April 20, 2026 07:02

wenchy requested changes Apr 20, 2026

View reviewed changes

chore: Fill missing Name/Desc fields in test CSV data

d052ffa

Kybxd changed the title ~~feat: Implement vertical aggregation with consistency validation~~ feat(BREAKING!): add fieldprop aggregate to enable aggregating incell lists/maps vertically Apr 21, 2026

wenchy requested changes Apr 22, 2026

View reviewed changes

refactor: Clarify aggregation and update map field handling

39ecbf2

Kybxd changed the title ~~feat(BREAKING!): add fieldprop aggregate to enable aggregating incell lists/maps vertically~~ feat(BREAKING!): support incell list/map aggregation across duplicate rows with consistency checks Apr 23, 2026

WeaponID	Attr1Name	Attr1Value	Attr2Name	Attr2Value	Level	UpgradeCost
map<uint32, Weapon>	[Attr]string	int32	string	int32	map<int32, Level>	int32
Weapon's ID	Attr1's name	Attr1's value	Attr2's name	Attr2's value	Weapon's Level	Cost to upgrade weapon to next level
1	Attack	100	Defence	200	1	10
1	Crit	30	CritDamage	50	2	20

		1,,11,110
		2,Orange,20,200

Conversation

Kybxd commented Apr 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

What's New

1. New field property: FieldProp.aggregate

2. New error codes

3. Simplified E2005

4. Refactor of table_parser.go

5. Test coverage

Migration Guide

Before

After

Uh oh!

codecov Bot commented Apr 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Kybxd commented Apr 15, 2026 •

edited

Loading

1. New field property: `FieldProp.aggregate`

4. Refactor of `table_parser.go`

codecov Bot commented Apr 15, 2026 •

edited

Loading