github-actions[bot] commented on code in PR #68785: URL: https://github.com/apache/doris/pull/68785#discussion_r4226228253
########## gensrc/AGENTS.md: ########## @@ -0,0 +1,29 @@ +# 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, the `TJSONProtocol` helpers in `be/src/exec/common/util.hpp`) that break for data serialized under the old name. Review Comment: [P3] Remove `TJSONProtocol` from the name-keyed rename hazards. The cited `util.hpp` helpers use Apache Thrift `TJSONProtocol`, and the JSON fixture passed to `from_json_string<TExpr>` is keyed by numeric field IDs (`"1"`, `"2"`, etc.). Renaming a Thrift field while preserving its ID does not break this path, so this example gives reviewers a false compatibility check. Keep the protobuf JSON and HTTP API paths, which do use names. -- 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]
