[client] Add lookup support for primary key tables #159
Conversation
|
@luoyuxia good morning, i didn't know if u were already working on this, i'm sorry if its the case @leekeiabstraction mr.lee as you are proficient in java and i'm not it would be really appreciated if you can check this out too Have a nice day, open to feedbacks/changes |
There was a problem hiding this comment.
Pull request overview
This PR adds lookup functionality for primary key tables, enabling clients to retrieve values by key from key-value tables. The implementation follows the simple lookup approach without complex batching/queuing, mirroring the Java client implementation.
Changes:
- Added Lookup API (key 1017) to support lookup operations
- Implemented protobuf messages for lookup requests and responses
- Added
lookup()async method toFlussTablefor key-value retrieval
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/fluss/src/rpc/api_key.rs | Added Lookup API key (1017) enum variant and conversions |
| crates/fluss/src/proto/fluss_api.proto | Defined protobuf messages for lookup requests and responses |
| crates/fluss/src/rpc/message/lookup.rs | Implemented LookupRequest struct with RequestBody trait |
| crates/fluss/src/rpc/message/mod.rs | Added lookup module import and export |
| crates/fluss/src/client/table/mod.rs | Added public lookup() method to FlussTable for key retrieval |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
leekeiabstraction
left a comment
leekeiabstraction
left a comment
There was a problem hiding this comment.
Thank you for the PR! I think while we do not need to implement queuing/batching, we should still have the abstraction/interfaces that users use consistent with the rest of the FlussTable function e.g. TableAppend, TableUpsert. These are public contract that would be difficult to change once released without breaking user's code.
luoyuxia
left a comment
luoyuxia
left a comment
There was a problem hiding this comment.
@AndreaBozzo Thanks for the pr. Just one comment to keep api align with java side.
luoyuxia
left a comment
luoyuxia
left a comment
There was a problem hiding this comment.
@AndreaBozzo Thanks for the pr. Left minor comments. PTAL
| /// using the appropriate bucketing function. | ||
| pub fn create_lookuper(self) -> Result<Lookuper<'a>> { | ||
| let num_buckets = self.table_info.get_num_buckets(); | ||
| let bucketing_function = <dyn BucketingFunction>::of(None); |
There was a problem hiding this comment.
we should use actual datalake format when datalake is enabled. See TableConfig#getDataLakeFormat in java code.
| /// * `Ok(Some(Vec<u8>))` - The value bytes if the key exists | ||
| /// * `Ok(None)` - If the key does not exist | ||
| /// * `Err(Error)` - If the lookup fails | ||
| pub async fn lookup(&self, key: Vec<u8>) -> Result<Option<Vec<u8>>> { |
There was a problem hiding this comment.
use row: &dyn InternalRow instead of key: Vec<u8>, we should do the encoding for users.
Also, we will need LookupResult just like java intead of Option<Vec<u8>> to handle lookups mutiple rows.
LookupResult should contains InternalRow instead of Vec<u8>, a byte array is hard to use for user. But for simplity, you can just use CompactedRow
Implements lookup functionality to verify put kv operations (Issue apache#114). - Add Lookup API key (1017) to api_key.rs - Add lookup proto messages (LookupRequest, LookupResponse, etc.) - Create lookup.rs message module with LookupRequest - Add async lookup() method to FlussTable
AndreaBozzo
force-pushed
the
ab/lookup-fluss
branch
from
0ebce14 to
9dec8fd
Compare
|
Rebased and addressed last comments from luo, leaving also edits on ON so he can have maximum control over this since it grew quite a bit from my initial implementation. Thanks for the help! |
Thanks update. I push a minor commit to improve the code. cc @leekeiabstraction |
leekeiabstraction
left a comment
leekeiabstraction
left a comment
There was a problem hiding this comment.
LGTM, nit (haven't left comment) that we should leave TODO comments in places where we know implementation is not at parity w/ Java side.
Approving, as this PR will unblock testing for my later PR as well.
3 participants
Purpose
Linked issue: close #114
Implements lookup functionality to verify put kv operations. This follows the simple lookup approach as suggested in the issue comments (without complex batching/queuing).
Brief change log
LookupAPI key (1017) toapi_key.rs- matching Java Fluss ApiKeys.javaLookupRequest,LookupResponse,PbLookupReqForBucket,PbLookupRespForBucket,PbValue) tofluss_api.protolookup.rsmessage module withLookupRequeststruct implementingRequestBodytraitlookup(bucket_id, key)method toFlussTablethat returnsOption<Vec<u8>>Reference Java implementation:
Tests
API and Format
Adds new public API:
FlussTable::lookup(bucket_id: i32, key: Vec<u8>) -> Result<Option<Vec<u8>>>Documentation
No documentation changes required at this time.