feat(java): introduce update in transaction#4408
feat(java): introduce update in transaction#4408jackye1995 merged 3 commits intolance-format:mainfrom
Conversation
fangbo
force-pushed
the
update-transaction
branch
from
3d03943 to
1a0a277
Compare
majin1102
left a comment
majin1102
left a comment
There was a problem hiding this comment.
Thanks for this contribution. I left some comments.
Please take a look
| public static class Builder { | ||
| private List<Long> removedFragmentIds = Collections.emptyList(); | ||
| private List<FragmentMetadata> updatedFragments = Collections.emptyList(); | ||
| private List<FragmentMetadata> newFragments = Collections.emptyList(); |
There was a problem hiding this comment.
I wonder why leave them not adapted:
/// The fields that have been modified
fields_modified: Vec<u32>,
/// The MemWAL (pre-image) that should be marked as flushed after this transaction
mem_wal_to_flush: Option<MemWal>,
There was a problem hiding this comment.
I wonder why leave them:
/// The fields that have been modified fields_modified: Vec<u32>, /// The MemWAL (pre-image) that should be marked as flushed after this transaction mem_wal_to_flush: Option<MemWal>,
At https://github.com/lancedb/lance/blob/main/rust/lance/src/dataset/write/update.rs#L437 , these two properties is not setted.
There was a problem hiding this comment.
Yeah, I see
I encountered some similar situations. I think this could be challenged for the unalignment with rust format. Maybe we should adapt them and don't use them in that case neither.
What do you think @jackye1995
There was a problem hiding this comment.
mem_wal_to_flush we can ignore those for now, WAL is WIP feature.
fields_modified I think we have that for future proof, for example add a new column while upserting, but that is also not implement for now.
As long as we use a builder style, we could be able to add those new fields in the future.
| config_upsert_values, | ||
| } | ||
| } | ||
| "Update" => { |
There was a problem hiding this comment.
We need to implement read transaction path as well(convert_to_java_operation)
fangbo
force-pushed
the
update-transaction
branch
from
7835ee0 to
0e561dc
Compare
jackye1995
left a comment
jackye1995
left a comment
There was a problem hiding this comment.
Looks good to me, pending rebase
fangbo
force-pushed
the
update-transaction
branch
from
237b063 to
c2334ce
Compare
Thanks very much . The PR has already been rebased. |
3 participants
Issue: #4332