Skip to content

refactor: rename Update operation fields#4832

Closed
yanghua wants to merge 17 commits intolance-format:mainfrom
yanghua:refactor-update-op-fields
Closed

refactor: rename Update operation fields#4832
yanghua wants to merge 17 commits intolance-format:mainfrom
yanghua:refactor-update-op-fields

Conversation

@yanghua
Copy link
Copy Markdown
Collaborator

@yanghua yanghua commented Sep 28, 2025

When I work on #4589, I found the field fields_modified in Update operation is hard to understand, there are four places we need to set it when updating:

  • pure update;
  • merge insert(two full schema) and sub col;

In some places, it is not based on the naming to set the modified fields. For example, here and here.

Actually, it is used to prune the index's frag bitmap based on some fields.

IMO, we could rename them(another one is fields_for_preserving_frag_bitmap) for a better name based on their purpose.

@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Sep 28, 2025

Codecov Report

❌ Patch coverage is 94.28571% with 4 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
rust/lance/src/dataset/transaction.rs 84.00% 4 Missing ⚠️

📢 Thoughts on this report? Let us know!

@yanghua yanghua force-pushed the refactor-update-op-fields branch from 7313dae to 988c5e2 Compare September 29, 2025 13:18
@yanghua yanghua marked this pull request as ready for review September 30, 2025 08:06
@jackye1995
Copy link
Copy Markdown
Contributor

I think a problem is that the train has already been released, it is hard to now change the name. What about we just add more documentation in the transaction protobuf definition and rust struct so people can better understand what those fields mean? I am working on fully refreshing the table spec, so once we update the proto the expectation is that it will be displayed in the spec doc in the future automatically.

@yanghua
Copy link
Copy Markdown
Collaborator Author

yanghua commented Oct 14, 2025

I think a problem is that the train has already been released, it is hard to now change the name.

What does the train mean? Do you mean the compatibility?

It seems protobuf is really an issue that would cause compatibility. Not sure if the community would introduce a fallback configuration mapping mechanism in the future. What about doing this:

  • for fields in pb and rust struct, use the old names;
  • for general variable naming them to be the new;

WDYT?

@jackye1995
Copy link
Copy Markdown
Contributor

What does the train mean? Do you mean the compatibility?

yeah sorry I mean a release train

for fields in pb and rust struct, use the old names;
for general variable naming them to be the new;

I could see the point for fields_modified, but how is bitmap_preserve_field_ids better than fields_for_preserving_frag_bitmap? Aren't we just changing the position of the words?

@yanghua
Copy link
Copy Markdown
Collaborator Author

yanghua commented Oct 15, 2025

but how is bitmap_preserve_field_ids better than fields_for_preserving_frag_bitmap? Aren't we just changing the position of the words?

My original thought is that if fields_modified could be changed to be bitmap_prune_field_ids, as a similar purpose field, fields_for_preserving_frag_bitmap could be aligned to bitmap_preserve_field_ids. It also makes the naming more concise. But the main motivation is to align with bitmap_prune_field_ids.

@yanghua yanghua force-pushed the refactor-update-op-fields branch from 6b171aa to 8d9641e Compare October 29, 2025 00:05
@yanghua
Copy link
Copy Markdown
Collaborator Author

yanghua commented Oct 29, 2025

@jackye1995 I did a trade-off: keep the old name in pb to keep compatibility, just renamed the other fields to make them follow the real semantics and better readability. WDYT?

@yanghua yanghua closed this Jan 21, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants