feat: support update/delete using MOR for spark3.5 - #79
Conversation
|
ACTION NEEDED The PR title and description are used as the merge commit message. Please update your PR title and description to match the specification. For details on the error please inspect the "PR Title Check" action. |
fangbo
changed the title
fangbo
changed the title
fangbo
changed the title
fangbo
changed the title
71d41ab to
89c28ab
Compare
fangbo
changed the title
d13be61 to
1029bf8
Compare
| package com.lancedb.lance.spark; | ||
|
|
||
| public class LanceConstant { | ||
| public static final String SEGMENT_ID = "_segmentid"; |
There was a problem hiding this comment.
Looks like this should be named like FRAGMENT_ID = "_fragid"?
| private final LanceDataWriter writer; | ||
|
|
||
| // Key is fragmentId, Value is fragment's deleted row indexes | ||
| private final Map<Integer, List<Integer>> deletedRows; |
There was a problem hiding this comment.
We can use a TreeMap<Integer, RoaringBitmap> for better efficiency (basically equivalent to RoaringTreemap in rust.
| } | ||
|
|
||
| @Test | ||
| public void testSparkSqlUpdate() throws Exception { |
There was a problem hiding this comment.
can we add more tests for various cases ,e.g. delete, update, touch a few rows, touch no rows, touch all rows, set scalar value, set struct/list value, set child field in a struct.
|
@jackye1995 Greatly appreciate for you suggestions! I have made some modifications and added more test cases (UpdateListTest, UpdateStructTest, UpdateTableTest) |
jackye1995
left a comment
jackye1995
left a comment
There was a problem hiding this comment.
Overall looks good to me, just some improvement suggestions on tests
There was a problem hiding this comment.
It feels unnecessary to have a base and have different types of update tests, can we just combine everything into a single test class? Also I think we are missing corresponding tests for DELETE FROM
fangbo
changed the title
jackye1995
left a comment
jackye1995
left a comment
There was a problem hiding this comment.
thanks for the great work!! I will merge this one first, @fangbo could you also update the documentation accordingly?
Thanks for your review. I will update the documentation later. |
Related Issue: lance-format#41 and lance-format#57 --------- Co-authored-by: fangbo.0511 <fangbo.0511@bytedance.com>
Related Issue: lance-format#41 and lance-format#57 --------- Co-authored-by: fangbo.0511 <fangbo.0511@bytedance.com>
Related Issue: lance-format#41 and lance-format#57 --------- Co-authored-by: fangbo.0511 <fangbo.0511@bytedance.com>
2 participants
Related Issue: #41 and #57