github-actions[bot] commented on code in PR #68785: URL: https://github.com/apache/doris/pull/68785#discussion_r4227358796
########## gensrc/AGENTS.md: ########## @@ -0,0 +1,22 @@ +# AGENTS.md — gensrc (thrift / protobuf) + +Everything under `gensrc/thrift` and `gensrc/proto` is a wire format between FE, BE and the cloud services; `olap_file.proto` and `segment_v2.proto` are also on-disk formats that every later release reads back. A field id or an enum value that has shipped on any branch is taken for good: a field that lands on another field of the same wire type is read as that field, and an enum member is just an integer on the wire, so neither mismatch produces an error. + +## Rules + +1. Append only. A new field takes the next unused id of its struct; a new enum member goes after the last one. Never insert into the middle of an enum, and never fill a gap: an id that any live release branch uses for a different field is taken on master too. +2. Never reuse an id. To drop a field, keep its id taken: in thrift leave the field in place marked `// Deprecated` (`TQueryOptions` 1) or comment the line out with its id (`TQueryOptions` 11); in proto list the number under `reserved`. Older proto messages renamed the field to `DEPRECATED_<name>` instead (`ColumnMetaPB` 14), which also keeps the number. A dropped enum member keeps its value the same way. +3. A field picked to a release branch keeps the id it has on master. If that id is already taken on the branch, renumber on master first so the same option never has two ids on two lines. `TQueryOptions` 183-231 on branch-4.1/4.2 against master is what happens otherwise; #68785 is the cleanup. Review Comment: [P2] Limit this renumber-on-master advice to IDs that have never shipped. If a released binary or persisted schema already uses the master's old ID, moving the field only on master leaves that released reader or writer on the old mapping, so rolling upgrades and stored-data reads can still fail. That case needs an explicit compatibility or migration plan. This PR can move these particular IDs because the old master assignments were unreleased. ########## gensrc/AGENTS.md: ########## @@ -0,0 +1,22 @@ +# AGENTS.md — gensrc (thrift / protobuf) + +Everything under `gensrc/thrift` and `gensrc/proto` is a wire format between FE, BE and the cloud services; `olap_file.proto` and `segment_v2.proto` are also on-disk formats that every later release reads back. A field id or an enum value that has shipped on any branch is taken for good: a field that lands on another field of the same wire type is read as that field, and an enum member is just an integer on the wire, so neither mismatch produces an error. + +## Rules + +1. Append only. A new field takes the next unused id of its struct; a new enum member goes after the last one. Never insert into the middle of an enum, and never fill a gap: an id that any live release branch uses for a different field is taken on master too. Review Comment: [P2] Apply these field-ID rules to Thrift union variants and service arguments too. `TNestedField` has numbered union alternatives, and `BackendService.submit_tasks(1:...)` shows tagged method arguments; if branches assign the same tag to different struct-valued meanings, a receiver can decode the wrong value without a wire-type error. The branch comparison below names only structs, enums, and messages, so it skips these tags. ########## gensrc/AGENTS.md: ########## @@ -0,0 +1,22 @@ +# AGENTS.md — gensrc (thrift / protobuf) + +Everything under `gensrc/thrift` and `gensrc/proto` is a wire format between FE, BE and the cloud services; `olap_file.proto` and `segment_v2.proto` are also on-disk formats that every later release reads back. A field id or an enum value that has shipped on any branch is taken for good: a field that lands on another field of the same wire type is read as that field, and an enum member is just an integer on the wire, so neither mismatch produces an error. + +## Rules + +1. Append only. A new field takes the next unused id of its struct; a new enum member goes after the last one. Never insert into the middle of an enum, and never fill a gap: an id that any live release branch uses for a different field is taken on master too. +2. Never reuse an id. To drop a field, keep its id taken: in thrift leave the field in place marked `// Deprecated` (`TQueryOptions` 1) or comment the line out with its id (`TQueryOptions` 11); in proto list the number under `reserved`. Older proto messages renamed the field to `DEPRECATED_<name>` instead (`ColumnMetaPB` 14), which also keeps the number. A dropped enum member keeps its value the same way. +3. A field picked to a release branch keeps the id it has on master. If that id is already taken on the branch, renumber on master first so the same option never has two ids on two lines. `TQueryOptions` 183-231 on branch-4.1/4.2 against master is what happens otherwise; #68785 is the cleanup. +4. Changing what a field means is a new field. Keep the type and id for a pure rename only; a new meaning gets a new id even when the old field is dropped in the same PR. `TabletSchemaPB` 24 went from `cluster_key_idxes` (column index) to `cluster_key_uids` (column unique id) in place, so a 4.0 BE reads a 3.1 tablet's indexes as uids. +5. What a receiver does with an unset field is part of the contract: the declared default, or the value an `__isset` / `has_` branch picks instead (`QueryContext` uses 1.0 for an unset `max_scan_mem_ratio`, whose declared default is 0.3). It only matters for a sender that does not set the field, such as an older FE or a path that builds the struct without it, but changing it changes what that sender means. +6. Renaming is allowed: the wire and the on-disk binaries carry ids, not names. Rename only when the meaning is unchanged, rename on every live branch that has the field or not at all, and check the name-keyed paths (json2pb dumps that `meta_tool load_meta` reads back, the cloud meta-service HTTP API) that break for data serialized under the old name. + +## Before you submit + +Compare the change with every release branch that still receives picks (today: branch-4.2, branch-4.1, branch-4.0, branch-3.1; add a new branch when it is cut). Fetch the remote that points to apache/doris, then read the same struct, enum or message on each branch without checking it out (`git show <remote>/branch-4.2:gensrc/thrift/<File>.thrift`) and look for both the id and the name. Commented-out and `reserved` lines still own their ids. A branch that does not have the field must have nothing at that id. A branch that already has it must have the same name, type and default; a different name or type is a collision, and a different default must be explained in the PR (for example, FE always sets the field). Paste what each branch has at that id into the PR. Review Comment: [P2] Compare requiredness and cardinality as well as name, type, and default. Branch-4.2's `CoordinatorContext.buildForLoad` can omit optional `cte_max_recursion_depth` (ID 184); making that same-ID/same-type field required would cause a new BE to reject the older FE's load fragment even though every listed check passes. Check optional/required and proto cardinality or oneof membership, or document why all senders remain compatible. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
