fix(table): ensure partition path is deterministic#767
fix(table): ensure partition path is deterministic#767ferhatelmas wants to merge 1 commit intoapache:mainfrom
Conversation
|
Ah, oops. Thanks for catching this and making the fix 🙇 . |
| i := 0 | ||
| for field := range spec.Fields() { | ||
| val, ok := partitionContext.partitionData[field.FieldID] | ||
| if !ok { | ||
| return "", fmt.Errorf("unexpected missing partition value for field id %d in spec id %d", field.FieldID, partitionContext.specID) | ||
| } | ||
| data[i] = val | ||
| i++ | ||
| } |
There was a problem hiding this comment.
should we change the Fields() method of partition specs to follow the more common pattern of returning an iterator of itr.Seq2[int, PartitionField] so that this can be for i, field := range spec.Fields() ?
It would just be changing Fields() to use slices.All instead of slices.Values. Thoughts? It would let us avoid this separately defined i var
There was a problem hiding this comment.
It would cause some churn and would prevent AppendSeq usage.
If we change, I think we should change SortOrder.Fields() as well and do it separately and then use here since it's a bigger scope. Happy to do if agreed
There was a problem hiding this comment.
Do we utilize AppendSeq much at all? It still might be better making the change to simplify scenarios like this (and users get a more familiar interaction with range that returns index and field) than to optimize for the AppendSeq scenario.
If we change, I think we should change SortOrder.Fields() as well and do it separately and then use here since it's a bigger scope. Happy to do if agreed
I say go for it unless either of you can think of a reason not to do so. We can hold off on merging this until you make the change and then we can update this. Thanks for volunteering!
There was a problem hiding this comment.
I will update this when it's merged
related to apache#767 Signed-off-by: ferhat elmas <elmas.ferhat@gmail.com>
) related to #767 Signed-off-by: ferhat elmas <elmas.ferhat@gmail.com>
partition record order is expected to match partition spec but map.Values can change it and cause cross-partition writer reuse, delete expected files, etc. Signed-off-by: ferhat elmas <elmas.ferhat@gmail.com>
2070c92 to
52cd2ed
Compare
partition record order is expected to match partition spec
but maps.Values can change it and cause cross-partition writer reuse,
delete expected files, etc.
related to #721