-
Notifications
You must be signed in to change notification settings - Fork 23
chore: introduce new with capacity to align generic row with java side #212
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
Conversation
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
Updates GenericRow construction to require an explicit field count (pre-initialized to nulls) to better match Java behavior and allow partial rows (e.g., PK-only deletes).
Changes:
- Change
GenericRow::new()toGenericRow::new(field_count: usize)and updateset_fieldto overwrite instead of insert. - Update call sites across core, bindings, examples, and integration tests to pass explicit field counts.
- Modernize a number of error/diagnostic messages to use Rust’s named-format interpolation.
Reviewed changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| crates/fluss/src/row/mod.rs | Introduces GenericRow::new(field_count) with null initialization; changes set_field semantics; updates tests. |
| crates/fluss/src/row/compacted/compacted_row_reader.rs | Adjusts row creation to pre-sized rows for deserialization. |
| crates/fluss/tests/integration/kv_table.rs | Updates integration tests to construct rows with explicit field counts; enables PK-only delete rows. |
| crates/fluss/tests/integration/table_remote_scan.rs | Updates test row construction for append/scan scenario. |
| crates/fluss/src/record/arrow.rs | Updates unit tests to construct rows with explicit field counts. |
| crates/fluss/src/client/table/log_fetch_buffer.rs | Updates test row construction with explicit field counts. |
| crates/examples/src/example_table.rs | Updates example to construct rows with explicit field counts. |
| crates/examples/src/example_kv_table.rs | Updates KV example to use explicit field counts and demonstrate PK-only delete. |
| bindings/cpp/src/types.rs | Updates FFI conversion to allocate rows with correct field count. |
| bindings/python/src/table.rs | Refactors error strings (named interpolation) in Python binding conversions. |
| crates/fluss/src/row/datum.rs | Refactors error strings (named interpolation). |
| crates/fluss/src/row/decimal.rs | Refactors error strings (named interpolation). |
| crates/fluss/src/row/column.rs | Refactors panic message formatting. |
| crates/fluss/src/metadata/table.rs | Refactors error string formatting. |
| crates/fluss/src/metadata/partition.rs | Refactors error string formatting. |
| crates/fluss/src/metadata/json_serde.rs | Refactors JSON serde error string formatting. |
| crates/fluss/src/client/table/upsert.rs | Refactors validation error string formatting. |
| crates/fluss/src/client/table/remote_log.rs | Minor formatting / unused-variable tweak and message formatting updates. |
Comments suppressed due to low confidence (1)
crates/fluss/src/row/compacted/compacted_row_reader.rs:59
GenericRow::new(self.row_type.fields().len())already initializes every field toDatum::Null, so in the nullable/null branch you can skiprow.set_field(col_pos, Datum::Null)and justcontinue;. This avoids an extra bounds-checked write per null field in a hot deserialization loop.
let mut row = GenericRow::new(self.row_type.fields().len());
let mut cursor = reader.initial_position();
for (col_pos, data_field) in self.row_type.fields().iter().enumerate() {
let dtype = &data_field.data_type;
if dtype.is_nullable() && reader.is_null_at(col_pos) {
row.set_field(col_pos, Datum::Null);
continue;
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
c1e6755 to
ee6e40a
Compare
leekeiabstraction
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.
LGTM, TY for the PR!
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.
@luoyuxia Thanks for the PR
LGTM overall. One comment
we have:
impl<'a> Default for GenericRow<'a>
Self::new(0) Old code used insert, now we set:
let mut row = GenericRow::default();
row.set_field(0, value); // PANIC! index out of boundsWe should make it consistent or remove.
|
Thanks all for review |
Purpose
Linked issue: close #186
Brief change log
GenericRow::new() -> GenericRow::new(field_count: usize)
Tests
API and Format
Documentation