-
Notifications
You must be signed in to change notification settings - Fork 23
Introduce PartitionGetter and Partition utils #214
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
@luoyuxia @fresh-borzoni Appreciate your reviews here |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@leekeiabstraction Thanks for the PR. LGTM
Left comment for the discussion.
| use crate::row::{Date, Datum, Time, TimestampLtz, TimestampNtz}; | ||
| use jiff::ToSpan; | ||
|
|
||
| fn hex_string(bytes: &[u8]) -> String { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: technically we can format into buffer directly:
use std::fmt::Write;
fn hex_string(bytes: &[u8]) -> String {
let mut hex = String::with_capacity(bytes.len() * 2);
for &b in bytes {
write!(&mut hex, "{:02x}", b).unwrap();
}
hex
}it would avoid allocation this way
| pub struct PartitionGetter<'a> { | ||
| partitions: Vec<(&'a String, &'a DataType, FieldGetter)>, | ||
| pub struct PartitionGetter { | ||
| partition_keys: Vec<String>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if Arc[String] is better, so cloning only Arc, as this code is hot, here it still makes sense.
I mean this later feels heavy:
ResolvedPartitionSpec::new(self.partition_keys.clone(), ...)Though it would require changes in other places.
Perhaps it's better to handle in separate PR if we consider this worth it. WDYT?
cc @luoyuxia as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree. I think we can consider in another pr.
Purpose
Linked issue: close #209
Brief change log
Tests