Codecov Report

Merging #156 (b81dddf) into main (cabaee0) will increase coverage by 0.00%.
The diff coverage is 69.23%.

@@            Coverage Diff            @@
##               main     #156   +/-   ##
=========================================
  Coverage     74.65%   74.65%           
  Complexity     2654     2654           
=========================================
  Files           834      834           
  Lines         47883    47888    +5     
  Branches       1562     1562           
=========================================
+ Hits          35747    35752    +5     
  Misses        11337    11337           
  Partials        799      799           

Flags with carried forward coverage won't be shown. Click here to find out more.

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cabaee0...b81dddf. Read the comment docs.

How about directly assembling a real operator_id here, so that we can only pass a single parameters to initialization of Keyspace?

        // We assume that the operator_id of different instances from the same RelNode will be the
        // same.
        let executor_id = ((actor_id as u64) << 32)    + node.get_operator_id();
+       let operator_id = ((fragment_id as u64) << 32) + node.get_operator_id();
-       let operator_id = node.get_operator_id().try_into().unwrap();

Using operator_id as parameter in shared_executor_root is a little bit confusing, and i don't think of a better naming for it.

How about directly assembling a real operator_id here, so that we can only pass a single parameters to initialization of Keyspace?

        // We assume that the operator_id of different instances from the same RelNode will be the
        // same.
        let executor_id = ((actor_id as u64) << 32)    + node.get_operator_id();
+       let operator_id = ((fragment_id as u64) << 32) + node.get_operator_id();
-       let operator_id = node.get_operator_id().try_into().unwrap();

Using operator_id as parameter in shared_executor_root is a little bit confusing, and i don't think of a better naming for it.

Actually node.get_operator_id() generated by the frontend is not what we want for operator_id, it's more like an "offset" which is only meaningful when we combine it with actor_id or fragment_id. Keyspace may not care about what exactly its parameters are for, it just wants to accept a unique ID corresponding to its RelNode.

How about directly assembling a real operator_id here, so that we can only pass a single parameters to initialization of Keyspace?

        // We assume that the operator_id of different instances from the same RelNode will be the
        // same.
        let executor_id = ((actor_id as u64) << 32)    + node.get_operator_id();
+       let operator_id = ((fragment_id as u64) << 32) + node.get_operator_id();
-       let operator_id = node.get_operator_id().try_into().unwrap();

Using operator_id as parameter in shared_executor_root is a little bit confusing, and i don't think of a better naming for it.

Actually node.get_operator_id() generated by the frontend is not what we want for operator_id, it's more like an "offset" which is only meaningful when we combine it with actor_id or fragment_id. Keyspace may not care about what exactly its parameters are for, it just wants to accept a unique ID corresponding to its RelNode.

Well, if we treat operator_id as an offset, it makes sense here. I will change it.