feat(protogen): support per-sheet union split via WorksheetOptions - #444
feat(protogen): support per-sheet union split via WorksheetOptions#444hazy-wu wants to merge 3 commits into
Conversation
Some worksheets define very large unions that make protoc-gen-cpp emit
a single very large .pb.cc translation unit, blocking parallel C++
compilation. This adds an opt-in mechanism to split a huge union into
shard proto files, controlled per worksheet (rather than by a global
CLI flag).
Wiring:
- tableau.proto WorksheetOptions +field 25 (union_split_threshold)
and +field 26 (union_split_shard_size).
- metabook.proto Metasheet +field 26/27 with matching metasheet
column names, so users can toggle the behavior directly from a
workbook's @tableau metasheet.
- internal/importer/book/sheet.go: copy Meta.UnionSplitThreshold /
UnionSplitShardSize into ws.Options during metasheet parsing.
- internal/protogen/exporter.go:
* bookExporter gains auxFiles / auxImportPaths, and a new
writeAuxFile emits each shard as a fresh top-level proto with
matching header (editions or proto3), imports and file options.
* exportUnion collects candidate sub-messages, decides shouldSplit
based on the per-sheet threshold, and either keeps the original
nested behaviour or delegates to a new exportUnionSplit.
* exportUnionSplit renames extracted sub-messages to
'<Union>T_<SubType>' and packs them into shards of at most
shard_size messages each (default 40).
- internal/protogen/exporter_test.go: golden test 'export-union-split'
covers the new emitter path; existing golden cases are untouched.
The 'T_' separator between the parent union name and the extracted
sub-message name is intentional: it avoids colliding with
protoc-gen-go's oneof wrapper naming (<Parent>_<CamelField>), which
is only disambiguated intra-file. Renaming only applies to locally
defined sub-messages (field.FullType == ''); predefined external
types keep their original fully-qualified names.
e8b3325 to
7d152ae
Compare
| // Threshold for splitting a huge union sheet into auxiliary shard proto | ||
| // files. Only takes effect when this sheet's mode is | ||
| // MODE_UNION_TYPE / MODE_UNION_TYPE_MULTI. When the number of sub-messages | ||
| // in the union exceeds this threshold, sub-messages are extracted out of | ||
| // the union as top-level messages named `<Union>T_<SubType>` placed in | ||
| // shard files, and the main proto imports those shards. This substantially | ||
| // shrinks any single generated `.pb.cc` translation unit and lets the C++ | ||
| // back-end compile the shards in parallel. | ||
| // | ||
| // Extracted sub-messages are renamed to `<Union>T_<SubType>` (the "T_" | ||
| // separator avoids colliding with protoc-gen-go's oneof wrapper naming | ||
| // `<Parent>_<CamelField>`, which is only disambiguated intra-file). Code | ||
| // that only depends on the union's `Type` enum and generated field | ||
| // accessors is unaffected by this rename. | ||
| // | ||
| // Set to 0 to disable (the default): the union is emitted with all | ||
| // sub-messages nested inline as usual. | ||
| // | ||
| // Default: 0 (disabled). | ||
| int32 union_split_threshold = 25; | ||
| // Number of extracted sub-messages placed in each shard file when | ||
| // `union_split_threshold` triggers splitting. Only used when | ||
| // `union_split_threshold > 0`. | ||
| // | ||
| // Default: 40. | ||
| int32 union_split_shard_size = 26; |
There was a problem hiding this comment.
To clarify "no globally": should UnionShardSize live only in Metasheet (with a new internal-only field on internal.Worksheet to reach the exporter), or is keeping it in WorksheetOptions acceptable?
There was a problem hiding this comment.
I suggest in internal.Worksheet only.
| // Threshold for splitting a huge union sheet into auxiliary shard proto | ||
| // files. Only takes effect when this sheet's Mode is | ||
| // MODE_UNION_TYPE / MODE_UNION_TYPE_MULTI. When the number of sub-messages | ||
| // in the union exceeds this threshold, sub-messages are extracted out of | ||
| // the union as top-level messages named `<Union>T_<SubType>` placed in | ||
| // shard files. This shrinks the biggest generated `.pb.cc` translation | ||
| // unit and enables parallel C++ back-end compilation for huge unions. | ||
| // | ||
| // Set to 0 to disable (the default). | ||
| int32 union_split_threshold = 26 [(tableau.field) = { | ||
| name: "UnionSplitThreshold" | ||
| prop: {optional: true} | ||
| }]; | ||
| // Number of extracted sub-messages placed in each shard file when | ||
| // `union_split_threshold` triggers splitting. Only used when | ||
| // `union_split_threshold > 0`. | ||
| // | ||
| // Default: 40. | ||
| int32 union_split_shard_size = 27 [(tableau.field) = { | ||
| name: "UnionSplitShardSize" | ||
| prop: {optional: true} | ||
| }]; |
There was a problem hiding this comment.
Just one option is enough:
// Number of oneof message defintions placed in each shard file for Union Type definition.
//
// Default: 50.
int32 union_shard_size = 26 [(tableau.field) = {
name: "UnionShardSize"
prop: {optional: true}
}];
There was a problem hiding this comment.
DO NOT ADD verbose T_, use <Union><SubType> instead for clean code and protobuf conventions.
<Union>T_<SubType> -> <Union><SubType>
There was a problem hiding this comment.
Missing functest for protogen at https://github.com/tableauio/tableau/tree/master/test/functest
Add csv files for Union type definiton, and use the new option "UnionShardSize"
Kybxd
left a comment
Kybxd
left a comment
There was a problem hiding this comment.
Thanks for this PR! The overall direction looks good — the per-sheet split strategy is the right level of granularity. Below are the detailed review comments based on discussing with the team.
1. (Blocking) Merge two config fields into a single union_shard_size
Files: proto/tableau/protobuf/tableau.proto, proto/tableau/protobuf/internal/metabook.proto, internal/protogen/exporter.go
As wenchy pointed out, there is no need for two separate fields (union_split_threshold + union_split_shard_size). A single field is sufficient:
// Number of oneof sub-messages placed in each shard file for Union Type definition.
// Set to 0 to disable splitting (the default).
// Default: 0 (disabled).
int32 union_shard_size = 25;Semantics:
union_shard_size = 0(default) → no split, original inline behaviorunion_shard_size > 0→ split enabled, each shard holds up to N sub-messages
This also simplifies the judgment logic in exportUnion():
shardSize := int(x.ws.GetOptions().GetUnionShardSize())
shouldSplit := shardSize > 02. (Blocking) Remove the T_ separator — use flat <Union><SubType> directly
File: internal/protogen/exporter.go
We evaluated two alternatives:
Option A (recommended) — flat concatenation: <Union><SubType>, e.g. TaskTargetPvpBattle
Option B — nested wrapper: <Union><ShardN> enclosing <SubType>, e.g.
message TaskTarget1 {
message PvpBattle { ... }
}We recommend Option A. Option B adds unnecessary nesting, making the oneof field declarations more verbose (TaskTarget1.PvpBattle pvp_battle = 1) without solving a real problem — shard files already isolate namespaces via the file path.
Regarding the T_ collision rationale from the current PR description: protoc-gen-go's oneof wrapper naming (<Parent>_<CamelField>) is an internal Go type name, not a proto-level name. It does not collide with proto-level message definitions across files, since protoc resolves types file-by-file via imports. Therefore <Union><SubType> is safe to use without T_.
Actions:
- Delete constant
unionSplitTypeSep - In
exportUnion(), change oneof field type tox.ws.Name + typ - In
exportUnionSplit(), changeprefixedTyptox.ws.Name + sm.typ
3. (Blocking) Missing functest
Directory: test/functest/
A functest case is needed. Add a CSV file defining a Union type sheet with UnionShardSize set and verify the end-to-end proto generation output.
4. (Blocking) Remove defaultUnionSplitShardSize — split only when user explicitly sets a positive value
File: internal/protogen/exporter.go
The current code has defaultUnionSplitShardSize = 40, which means setting union_split_threshold > 0 but leaving union_split_shard_size = 0 still triggers splitting. Per discussion, the user's explicit opt-in should be required for every aspect: union_shard_size = 0 means no split at all. Delete the defaultUnionSplitShardSize constant and the fallback logic.
5. (Blocking) Extract common proto file header generation to eliminate duplication
File: internal/protogen/exporter.go
export() and the new writeAuxFile() both duplicate ~30 lines of header generation code (the "Code generated by tableau" comment, syntax/edition, package, sorted imports, sorted file options). Extract this into a shared method:
func (x *bookExporter) writeProtoHeader(p *printer.Printer, imports map[string]bool) {
// comment, syntax/edition, package, sorted imports, sorted file options
}Both export() and writeAuxFile() should call this method. writeAuxFile() then becomes much simpler: call writeProtoHeader → write the shard body.
6. (Suggestion) Add boundary test cases
File: internal/protogen/exporter_test.go
The current export-union-split test only covers threshold=1, shardSize=1 with 2 sub-messages. Consider adding:
shardSize=0→ no split (default)shardSize=1with only 1 sub-message → single shard- Mixed local types and external predefined types → verify external types are excluded from subMsgs
7. (Suggestion) Use filepath.Ext for shard file naming
File: internal/protogen/exporter.go, exportUnionSplit()
The current strings.TrimSuffix(mainPath, x.be.FilenameSuffix+".proto") is fragile when FilenameSuffix is empty or non-standard. Use filepath.Ext + filepath.Base instead:
mainPath := x.be.GetProtoFilePath()
ext := filepath.Ext(mainPath)
base := strings.TrimSuffix(filepath.Base(mainPath), ext)
shardTag := strcase.FromContext(x.be.gen.ctx).ToSnake(x.ws.Name)
shardRelPath := fmt.Sprintf("%s_%s_%d%s", base, shardTag, shardIdx+1, ext)Keep the shardTag — it is necessary to disambiguate when a single workbook proto contains multiple union sheets.
8. (Confirmed) No additional mode validation needed
No need to add a warning when union_shard_size is set on non-union sheets. Other parameters (e.g. Index only effective for MODE_DEFAULT) already follow this pattern of being silently ignored for inapplicable modes.
Summary of action items:
| # | Type | Summary |
|---|---|---|
| 1 | Blocking | Merge into single union_shard_size field, >0 enables, =0 disables |
| 2 | Blocking | Use <Union><SubType> flat naming, remove T_ separator |
| 3 | Blocking | Add functest |
| 4 | Blocking | Remove defaultUnionSplitShardSize, no implicit default |
| 5 | Blocking | Extract shared writeProtoHeader to eliminate header duplication |
| 6 | Suggestion | Add boundary test cases |
| 7 | Suggestion | Use filepath.Ext for shard file naming |
| 8 | Confirmed | No mode validation needed |
Merge union split threshold and shard size into a single Worksheet.union_shard_size field (moved out of WorksheetOptions); drop the "T_" type separator in favor of <Union><SubType> naming; extract writeProtoHeader to dedupe header generation; name shard files via filepath.Ext; emit (tableau.workbook) option on shard protos and let confgen skip files with no worksheet messages; assign field numbers to extracted sub-messages; add functest and edge-case unit tests.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #444 +/- ##
==========================================
+ Coverage 75.63% 75.69% +0.06%
==========================================
Files 88 88
Lines 9531 9679 +148
==========================================
+ Hits 7209 7327 +118
- Misses 1747 1765 +18
- Partials 575 587 +12 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Fix union split oneof rewriting to handle custom-named structs and reused local types (not just empty-Type structs); keep scalar and predefined types unchanged. Add unit cases and functest golden (SplitTarget with scalar/predefined/reused/custom-named types) covering two shard files.
3 participants
Summary
Add a per-sheet knob to split an oversized
MODE_UNION_TYPEsheet intoauxiliary shard proto files, instead of always inlining every sub-message into
the single union message (which can produce a multi-megabyte generated file for
huge unions).
The switch is controlled through the existing
@TABLEAUmetasheet columnsUnionSplitThresholdandUnionSplitShardSize, mapped toWorksheetOptions—same wiring path as
Mode,FieldPresence,OrderedMap, etc.Motivation
A huge union sheet (hundreds of sub-messages) emits one very large generated
file that dominates compile time for downstream consumers (e.g. a single
~5.7 MB.pb.ccon the C++ side). Splitting only that sheet cuts the largesttranslation unit by ~80% and lets the build parallelize the shards.
However splitting every union is undesirable: small/mid unions read much
better as nested messages. A per-sheet knob is the right granularity, and it is
backward compatible — sheets that don't set the columns keep emitting exactly
the same nested output.
Design
WorksheetOptionsgains two fields:union_split_threshold = 24— split when the sub-message count exceeds it.union_split_shard_size = 25— how many sub-messages go into each shard file.0, meaning disabled (byte-for-byte identical to today).Metasheetgains matching metasheet columnsUnionSplitThreshold(field 25) /UnionSplitShardSize(field 26).internal/importer/book/sheet.gocopies the two fields fromMetaintoWorksheetOptions(one line per field, same shape as every other metasheet option).internal/protogen/exporter.goreadsws.GetOptions().GetUnionSplitThreshold()/GetUnionSplitShardSize()at theexportUnionsite; when triggered, sub-messages are extracted as top-levelmessages
<Union>T_<SubType>into per-shard proto files and the main protoimports them.
<Union>T_<SubType>. TheT_separatoris required: protoc-gen-go names each oneof wrapper
<Parent>_<CamelField>and only disambiguates intra-file, so using
_would cause aredeclarederror across shard files. Business code that only depends on the
Typeenumand generated accessors is unaffected.
Behavior
Tests
Test_sheetExporter_exportUnion:export-union(threshold = 0) — verifies the default nested behavior.export-union-split— verifies the new shard-emitter path (golden diff).test/functest/testdata/.../union,.../metasheet) are unaffected because they don't set the threshold.Notes for reviewers
go test -raceclean.go.modstays ongo 1.21.T_separator is the only non-obvious choice — rationale is in theunionSplitTypeSepdoc comment inexporter.go.