fix(agg): fix the wrong passing of dist key. #3437
Conversation
BowenXiao1999
force-pushed
the
bw/fix-dist-key
branch
from
7d96377
to
d289c77
Compare
Codecov Report
@@ Coverage Diff @@
## main #3437 +/- ##
==========================================
- Coverage 74.25% 74.25% -0.01%
==========================================
Files 768 768
Lines 106499 106502 +3
==========================================
+ Hits 79086 79088 +2
- Misses 27413 27414 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more |
| @@ -410,6 +410,7 @@ pub fn generate_state_table<S: StateStore>( | |||
| let table_desc = generate_column_descs(agg_call, group_keys, pk_indices, agg_schema, input_ref); | |||
| // Always leave 1 space for agg call value. | |||
| let relational_pk_len = table_desc.len() - 1; | |||
| let dist_keys: Vec<usize> = (0..group_keys.len()).collect(); | |||
There was a problem hiding this comment.
IIUC here dist_keys stands for the dist key indices in pk. But in fact we should pass dist key indices in value to state table? cc. @BugenZhao
There was a problem hiding this comment.
Hmmm, I remembered that when compute vnode we are using indices on pk?
There was a problem hiding this comment.
But in fact we should pass dist key indices in value to state table?
Yes. However, it seems they are the same in this case?
There was a problem hiding this comment.
Hmmm, I remembered that when compute vnode we are using indices on pk?
We switch to indices on all columns to reduce the complexity of the interfaces. The "indices based on pk" will be inferred in the cell-based table.
I hereby agree to the terms of the Singularity Data, Inc. Contributor License Agreement.
What's changed and what's your intention?
Otherwise the vnode will not be computed for hash_agg. The group key will always be the first [0, group_key.len()) of insert row.
Checklist
./risedev check(or alias,./risedev c)Refer to a related PR or issue link (optional)