github-actions[bot] commented on code in PR #65483:
URL: https://github.com/apache/doris/pull/65483#discussion_r3627560787


##########
be/src/core/value/bitmap_value.h:
##########
@@ -2281,6 +2281,67 @@ class BitmapValue {
         return 0;
     }
 
+    uint64_t xor_cardinality(const BitmapValue& rhs) const {
+        switch (rhs._type) {
+        case EMPTY:
+            return cardinality();
+        case SINGLE:
+            switch (_type) {
+            case EMPTY:
+                return 1;
+            case SINGLE:
+                return 1 + (_sv != rhs._sv);

Review Comment:
   [P1] Return zero for equal singleton XORs
   
   When both operands use the `SINGLE` representation and contain the same 
value, their symmetric difference is empty, but this expression returns `1 + 
false == 1`. The new `bitmap_xor_count` shortcut calls `xor_cardinality()` 
directly, so with `enable_set_in_bitmap_value=false`, 
`bitmap_xor_count(to_bitmap(k), to_bitmap(k))` (and the rewritten 
count-over-XOR form) reaches equal `SINGLE` operands and reports 1 instead of 
0. The current representation-matrix test only compares singleton 1 with 
singleton 2, so please add the equal case as well.
   
   ```suggestion
                   return 2 * (_sv != rhs._sv);
   ```



##########
fe/fe-core/src/main/java/org/apache/doris/qe/CoordinatorContext.java:
##########
@@ -310,6 +310,7 @@ public static CoordinatorContext buildForLoad(
         queryOptions.setBeExecVersion(Config.be_exec_version);
         queryOptions.setNewVersionUnixTimestamp(true);
         queryOptions.setNewVersionPercentile(true);
+        queryOptions.setNewVersionBitmapOpCount(true);

Review Comment:
   [P1] Propagate the flag through the legacy broker-load coordinator
   
   This updates the Nereids load-options builder, but the cloud broker-load 
fallback still reaches `CloudCoordinator(Long, ...)` / `Coordinator(Long, ...)` 
when the global `enable_nereids_distribute_planner` option is disabled. That 
parallel constructor creates a bare `TQueryOptions` without this flag. For a 
mapping such as `dst = bitmap_and_count(bitmap_from_string(src1), 
bitmap_from_string(src2))`, the nullable inputs make legacy BE derive 
`Nullable(BIGINT)` while the new FE serialized non-null `BIGINT`, so function 
preparation rejects the load. This is separate from the earlier short-circuit 
thread, whose planner options already contain the flag. Please set or 
centralize the version option in the legacy load coordinator too and cover this 
cloud fallback.



-- 
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]

Reply via email to