Skip to content

Conversation

@leekeiabstraction
Copy link
Contributor

Purpose

Linked issue: close #185

Fix issue with TableLookup parsing being off by 2 columns

Brief change log

Fix TableLookup so that SchemaId field bytes are not passed to CompactedRow::from_bytes(), these fields can be skipped as current rust client implementation does not check schema and already passes row_type

Tests

Verified using example_kv_table.rs. Integration test to follow

=== Upserting ===
Upserted: GenericRow { values: [Int32(1), String("Verso"), Int64(32)] }
Upserted: GenericRow { values: [Int32(2), String("Noco"), Int64(25)] }
Upserted: GenericRow { values: [Int32(3), String("Esquie"), Int64(35)] }

=== Looking up ===
Found id=1: name=Verso, age=32
Found id=2: name=Noco, age=25
Found id=3: name=Esquie, age=35

=== Updating ===
Updated: GenericRow { values: [Int32(1), String("Verso"), Int64(33)] }
Verified update: name=Verso, age=33

=== Deleting ===
Deleted: GenericRow { values: [Int32(2), String(""), Int64(0)] }
Verified deletion

…tedRow::from_bytes(), these fields can be skipped as current rust client implementation does not check schema and already passes row_type
@leekeiabstraction
Copy link
Contributor Author

@luoyuxia @AndreaBozzo Appreciate if you can review here

Copy link
Contributor

@AndreaBozzo AndreaBozzo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGMT, comment is more of a personal question

Comment on lines -67 to 74
1 => Ok(Some(CompactedRow::from_bytes(self.row_type, &self.rows[0]))),
1 => Ok(Some(CompactedRow::from_bytes(
self.row_type,
&self.rows[0][SCHEMA_ID_LENGTH..],
))),
_ => Err(Error::UnexpectedError {
message: "LookupResult contains multiple rows, use get_rows() instead".to_string(),
source: None,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

more of a brainstorming for later, on paper can still panic if the response is smaller than 2 bytes right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good question. That is right, however that would be an unexpected case. I would expect that schema id field and header to be there, having said that, making this more defensive by returning Error would be a good improvement to make

Copy link
Contributor

@luoyuxia luoyuxia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

@luoyuxia luoyuxia merged commit eaf5a24 into apache:main Jan 21, 2026
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

TableLookup returned values parsing are off

3 participants