Introduce PartitionGetter and Partition utils #214
Conversation
|
@luoyuxia @fresh-borzoni Appreciate your reviews here |
There was a problem hiding this comment.
Pull request overview
This PR introduces partition-related utilities for the Fluss Rust client, mirroring functionality from the Java implementation. It adds partition validation, auto-partition generation, and partition value conversion capabilities.
Changes:
- Added
PartitionGetterfor extracting partition names/specs from rows - Added partition utilities including validation and auto-partition time generation
- Added validation functions to
TablePathfor name and prefix checking - Added new
InvalidPartitionerror variant
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/fluss/src/util/partition.rs | New file containing partition validation, auto-partition generation, and datum-to-string conversion utilities |
| crates/fluss/src/util/mod.rs | Exports the new partition module |
| crates/fluss/src/metadata/table.rs | Adds validation methods for table/partition names and prefixes |
| crates/fluss/src/error.rs | Adds InvalidPartition error variant for partition-related errors |
| crates/fluss/src/client/table/partition_getter.rs | Implements PartitionGetter to extract partition information from rows, with comprehensive test coverage |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Copilot comments have been addressed |
fresh-borzoni
left a comment
fresh-borzoni
left a comment
There was a problem hiding this comment.
@leekeiabstraction Thank you for the PR.
Left comments. PTAL
|
@fresh-borzoni Thank you for great catches. I've updated the PR accordingly. PTAL! |
luoyuxia
left a comment
luoyuxia
left a comment
There was a problem hiding this comment.
@leekeiabstraction Thanks for the pr. Left some comments. PTAL
|
@luoyuxia @fresh-borzoni Thank you for your reviews. Addressed/answered comments. PTAL! |
fresh-borzoni
left a comment
fresh-borzoni
left a comment
There was a problem hiding this comment.
@leekeiabstraction Thanks for the PR. LGTM
Left comment for the discussion.
|
@luoyuxia @fresh-borzoni Thank you for the great comments. Addressed them. PTAL |
|
|
||
| fn get_reordered_partition_values( | ||
| partition_keys: &[String], | ||
| partition_keys: &Arc<[String]>, |
There was a problem hiding this comment.
nit:
mb just &[String]?
and pass partition_keys.as_ref()
There was a problem hiding this comment.
Let's hold this off, it's hard-ish to tell what best arg type to use until everything is wired together. I might even remove this function in the final wiring PR.
There was a problem hiding this comment.
well, it's actually widening the accepted args.
I've approved PR before, so treat it as just discussion :)
3 participants
Purpose
Linked issue: close #209
Brief change log
Tests