Skip to content

[rust] lookup decoding schema id #517

Merged
leekeiabstraction merged 4 commits into
apache:mainfrom
fresh-borzoni:fix/lookup-schema-id
May 3, 2026
Merged

[rust] lookup decoding schema id #517
leekeiabstraction merged 4 commits into
apache:mainfrom
fresh-borzoni:fix/lookup-schema-id

Conversation

@fresh-borzoni
Copy link
Copy Markdown
Member

@fresh-borzoni fresh-borzoni commented May 1, 2026

Summary

closes #508

Lookup rows now decode under the schema id they were written with, then project to the table's current schema. Index mapping matches by column id (stable across ALTER TABLE … RENAME).
Side fix: Column.id is now preserved through Schema JSON serde as it was silently dropped on the read path.

@fresh-borzoni fresh-borzoni changed the title [rust] lookup decoding schema_id e2e [rust] lookup decoding schema id May 1, 2026
Comment thread Cargo.lock
@fresh-borzoni fresh-borzoni force-pushed the fix/lookup-schema-id branch 2 times, most recently from 5e0717f to 0bb99b0 Compare May 1, 2026 14:12
@fresh-borzoni
Copy link
Copy Markdown
Member Author

@leekeiabstraction @charlesdong1991 PTAL 🙏

Copy link
Copy Markdown
Contributor

@charlesdong1991 charlesdong1991 left a comment

Choose a reason for hiding this comment

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

very nice PR!! just some nit comments!

Comment thread crates/fluss/src/client/table/lookup.rs
Comment thread crates/fluss/src/row/projected_row.rs
Comment thread crates/fluss/src/row/lookup_row.rs
@fresh-borzoni
Copy link
Copy Markdown
Member Author

@charlesdong1991 Ty for the review, answered comments, PTAL 🙏

Copy link
Copy Markdown
Contributor

@charlesdong1991 charlesdong1991 left a comment

Choose a reason for hiding this comment

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

the implementation looks good to me! great job!! @fresh-borzoni

Only a nitpick: i think we could update api-references a bit, not blocking and can be separate pr too.

Comment thread crates/fluss/src/client/admin.rs
Comment thread crates/fluss/src/metadata/table.rs
Copy link
Copy Markdown
Contributor

@leekeiabstraction leekeiabstraction left a comment

Choose a reason for hiding this comment

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

TY for the PR, left a couple of comments.

Comment thread crates/fluss/src/client/admin.rs
Comment thread crates/fluss/src/client/table/lookup.rs Outdated
@fresh-borzoni fresh-borzoni force-pushed the fix/lookup-schema-id branch from 0bb99b0 to 22f6057 Compare May 3, 2026 11:47
@fresh-borzoni
Copy link
Copy Markdown
Member Author

@leekeiabstraction Ty for the review, addressed all comments, PTAL 🙏

Copy link
Copy Markdown
Contributor

@leekeiabstraction leekeiabstraction left a comment

Choose a reason for hiding this comment

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

TY for addressing, left further comments.

Comment thread crates/fluss/src/client/table/lookup.rs
Comment thread crates/fluss/src/metadata/datatype.rs Outdated
Comment thread crates/fluss/src/metadata/schema_util.rs Outdated
@fresh-borzoni
Copy link
Copy Markdown
Member Author

@leekeiabstraction Ty, addressed 👍

Copy link
Copy Markdown
Contributor

@leekeiabstraction leekeiabstraction left a comment

Choose a reason for hiding this comment

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

TY for the PR. LGTM

@leekeiabstraction leekeiabstraction merged commit f7e9cb9 into apache:main May 3, 2026
11 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.

[rust] Lookup result decodes every row with the latest schema, ignoring per-row schema id

3 participants