github-actions[bot] commented on code in PR #68785:
URL: https://github.com/apache/doris/pull/68785#discussion_r4219135257
##########
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: rename it
`DEPRECATED_<name>` in thrift (`ColumnMetaPB` 14 does this) or list it under
`reserved` in proto. A dropped enum member keeps its value the same way.
Review Comment:
[P3] Correct the deprecated-field example. `ColumnMetaPB` field 14 is in
`gensrc/proto/segment_v2.proto`, where it remains as
`DEPRECATED_default_value`; it is not a Thrift field. That example contradicts
this sentence's instruction to reserve removed protobuf numbers. Use a Thrift
example, or label this as an existing protobuf exception and show the
recommended `reserved` form for future removals.
##########
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: rename it
`DEPRECATED_<name>` in thrift (`ColumnMetaPB` 14 does this) or list it under
`reserved` in proto. 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. A default value is part of the contract: it is what an old sender that
omits the field means to a new receiver, so changing it is a behavior change
for every mixed-version cluster.
+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.
+
+## Before you submit
+
+Compare the struct or enum with master and 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). Put your own file, struct, id and name in and paste
the output into the PR:
+
+ for b in master branch-4.2 branch-4.1 branch-4.0 branch-3.1; do
+ echo "== $b"
+ git show upstream/$b:gensrc/thrift/<File>.thrift | awk '/^struct
<Struct> /,/^}/' | grep -nE '^\s*<id>:|\b<name>\b'
+ done
+
+A branch that does not have the field must print nothing; a branch that
already has it must print the one line with the same id and the same name.
Anything else is a collision. For an enum, grep `= <value>\b|\b<MEMBER>\b`
inside the enum block the same way.
Review Comment:
[P2] Compare type and default as well as ID and name. This pass condition
would accept `TQueryOptions.ivf_nprobe` on HEAD and branch-4.0 because both use
id 182 and the same name, yet their declared defaults are 32 and 1. Rule 5
calls that a mixed-version behavior change. Once the command reads the right
refs, require matching wire type and declared default too, and review semantic
meaning separately; a default mismatch is not an ID collision.
##########
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: rename it
`DEPRECATED_<name>` in thrift (`ColumnMetaPB` 14 does this) or list it under
`reserved` in proto. 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. A default value is part of the contract: it is what an old sender that
omits the field means to a new receiver, so changing it is a behavior change
for every mixed-version cluster.
Review Comment:
[P3] Check receiver-side absence handling as part of the default contract.
`max_scan_mem_ratio`, which this PR moves to id 213, declares a Thrift default
of 0.3, but `QueryContext` uses 1.0 when `__isset.max_scan_mem_ratio` is false.
An older sender omitting a field can therefore mean something other than its
declared default. State that reviewers must trace `__isset` branches and
preserve the actual absent-field behavior.
##########
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: rename it
`DEPRECATED_<name>` in thrift (`ColumnMetaPB` 14 does this) or list it under
`reserved` in proto. 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. A default value is part of the contract: it is what an old sender that
omits the field means to a new receiver, so changing it is a behavior change
for every mixed-version cluster.
+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.
+
+## Before you submit
+
+Compare the struct or enum with master and 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). Put your own file, struct, id and name in and paste
the output into the PR:
+
+ for b in master branch-4.2 branch-4.1 branch-4.0 branch-3.1; do
+ echo "== $b"
+ git show upstream/$b:gensrc/thrift/<File>.thrift | awk '/^struct
<Struct> /,/^}/' | grep -nE '^\s*<id>:|\b<name>\b'
Review Comment:
[P2] Make the branch check run against the candidate schema. This checkout
has only an `origin` remote, so every `git show upstream/$b` in the proposed
loop fails; even changing that to `origin` reads remote master rather than this
PR's changed `HEAD`. A pre-submit check can therefore miss the very id changes
being proposed. Resolve the available release refs, fail on missing refs/files,
and compare those definitions with the worktree or HEAD.
##########
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: rename it
`DEPRECATED_<name>` in thrift (`ColumnMetaPB` 14 does this) or list it under
`reserved` in proto. 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. A default value is part of the contract: it is what an old sender that
omits the field means to a new receiver, so changing it is a behavior change
for every mixed-version cluster.
+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.
+
+## Before you submit
+
+Compare the struct or enum with master and 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). Put your own file, struct, id and name in and paste
the output into the PR:
+
+ for b in master branch-4.2 branch-4.1 branch-4.0 branch-3.1; do
+ echo "== $b"
+ git show upstream/$b:gensrc/thrift/<File>.thrift | awk '/^struct
<Struct> /,/^}/' | grep -nE '^\s*<id>:|\b<name>\b'
+ done
+
+A branch that does not have the field must print nothing; a branch that
already has it must print the one line with the same id and the same name.
Anything else is a collision. For an enum, grep `= <value>\b|\b<MEMBER>\b`
inside the enum block the same way.
+
+## Review checkpoints
+
+- Is every new id the next free one on every branch that has this struct, and
is every new enum member last?
+- Did any existing id or enum value change name, type, default or meaning?
Each is a compatibility break unless it is a `DEPRECATED_` rename.
Review Comment:
[P3] Include the pure-rename exception in this checkpoint. Rules 4 and 6
above permit a name change that keeps the same meaning, type, and ID after
checking live branches and name-keyed paths, but this line calls every
non-`DEPRECATED_` rename a compatibility break. As written, the final checklist
would reject the rename the rules explicitly allow.
##########
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: rename it
`DEPRECATED_<name>` in thrift (`ColumnMetaPB` 14 does this) or list it under
`reserved` in proto. 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. A default value is part of the contract: it is what an old sender that
omits the field means to a new receiver, so changing it is a behavior change
for every mixed-version cluster.
+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.
+
+## Before you submit
+
+Compare the struct or enum with master and 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). Put your own file, struct, id and name in and paste
the output into the PR:
+
+ for b in master branch-4.2 branch-4.1 branch-4.0 branch-3.1; do
+ echo "== $b"
+ git show upstream/$b:gensrc/thrift/<File>.thrift | awk '/^struct
<Struct> /,/^}/' | grep -nE '^\s*<id>:|\b<name>\b'
+ done
+
+A branch that does not have the field must print nothing; a branch that
already has it must print the one line with the same id and the same name.
Anything else is a collision. For an enum, grep `= <value>\b|\b<MEMBER>\b`
inside the enum block the same way.
+
+## Review checkpoints
+
+- Is every new id the next free one on every branch that has this struct, and
is every new enum member last?
Review Comment:
[P2] State the cross-branch ID check in terms of collisions. This checkpoint
asks one new ID to be the next free number on master and every release branch,
which cannot hold for `TQueryOptions`: master now uses 236, while 4.2 stops at
230 and 4.0 has a sparser map. The safe next master ID would not be each older
branch's next free ID. Require the next valid master ID, verify it is unused or
already has the same meaning on every branch, and retain that ID on picks;
update the matching skill checkpoint too.
##########
gensrc/thrift/PaloInternalService.thrift:
##########
@@ -423,7 +423,8 @@ struct TQueryOptions {
182: optional i32 ivf_nprobe = 32;
// Enable hybrid sorting: dynamically selects between PdqSort and TimSort
based on
// runtime profiling to choose the most efficient algorithm for the data
pattern
- 183: optional bool enable_use_hybrid_sort = false;
+ 183: optional bool enable_aggregate_function_null_v2 = false;
Review Comment:
[P3] Move this hybrid-sort comment with its field. The PdqSort/TimSort
description now sits immediately above `enable_aggregate_function_null_v2`,
while `enable_use_hybrid_sort` moved to id 210 without it. Readers of the
schema will attribute the behavior to the wrong option.
--
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]
