feat: add table config #2820

dsgibbons · 2024-09-03T13:25:34Z

#2200 reference the concept of "table metadata". This PR uses the name "config" to avoid potential confusion with other uses of the word "metadata" throughout the Lance format.

This PR introduces:

A new table.proto field called config.
New Dataset methods: update_config and delete_config_keys.
config field in Manifest with public methods for updating and deleting.
A new transaction operation UpdateConfig along with conflict logic that returns an error if an operation mutates a key that is being upserted by another operation.
A new writer feature flag called FLAG_TABLE_CONFIG.
Unit tests for new Dataset methods, concurrent config updaters, and conflict resolution logic.

codecov-commenter · 2024-09-03T13:46:08Z

Codecov Report

Attention: Patch coverage is 92.38329% with 31 lines in your changes missing coverage. Please review.

Project coverage is 78.86%. Comparing base (cdac5de) to head (e62bd82).

Files with missing lines	Patch %	Lines
rust/lance/src/dataset/transaction.rs	91.35%	15 Missing and 1 partial ⚠️
rust/lance/src/io/commit.rs	87.93%	0 Missing and 14 partials ⚠️
rust/lance/src/dataset.rs	98.03%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2820      +/-   ##
==========================================
+ Coverage   78.77%   78.86%   +0.08%     
==========================================
  Files         237      237              
  Lines       74099    74482     +383     
  Branches    74099    74482     +383     
==========================================
+ Hits        58374    58737     +363     
- Misses      12703    12721      +18     
- Partials     3022     3024       +2

Flag	Coverage Δ
unittests	`78.86% <92.38%> (+0.08%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

wjones127

Thanks for working on this. This seems to be in the direction I was thinking. I've left some guiding comments.

rust/lance-table/src/format/manifest.rs

wjones127 · 2024-09-05T15:20:12Z

protos/table.proto

@@ -85,6 +85,7 @@ message Manifest {
  // * 1: deletion files are present
  // * 2: move_stable_row_ids: row IDs are tracked and stable after move operations
  //       (such as compaction), but not updates.
+  // * 8: table metadata are present


Shouldn't this be 4?

Also, I'm thinking this should be a writer flag only, at least in practice. My use case in my head is that we will use this as configuration for various write operations (how often to auto-cleanup, compact, etc.). But reading the table properly should require interpreting these.

Isn't 4 taken by FLAG_USE_V2_FORMAT_DEPRECATED?: https://github.com/lancedb/lance/blob/main/rust/lance-table/src/feature_flags.rs#L18

Oh okay. Well then we should add that to the proto docs.

Updated to include docs for 4.

Also, I'm thinking this should be a writer flag only, at least in practice. My use case in my head is that we will use this as configuration for various write operations (how often to auto-cleanup, compact, etc.). But reading the table properly should require interpreting these.

Does anything have to change to accomodate this comment?

Could you move it to just be under the writer_feature_flags?

Ok, I think I've done this in e62bd82, but let me know if anything else needs editing to make this only apply to writers. I'm not sure if that comment in the proto file needs to change? The code in feature_flags.rs + existing comments in table.proto makes it seem like the flag should still present be under reader_feature_flags even if it only applies to writers?

The code in feature_flags.rs + existing comments in table.proto makes it seem like the flag should still present be under reader_feature_flags even if it only applies to writers?

What do you mean by this? There are separate can_read_dataset and can_write_dataset methods that check the respective flags.

protos/transaction.proto

rust/lance/src/dataset.rs

wjones127 · 2024-09-05T15:25:11Z

rust/lance/src/dataset.rs

@@ -3039,6 +3089,53 @@ mod tests {
        assert!(fragments[0].metadata.deletion_file.is_some());
    }

+    #[rstest]
+    #[tokio::test]
+    async fn test_table_metadata(


The main test I would like to see is that if you have 5 concurrent writers each adding a different metadata key, that each of them can succeed and their write is reflected in the final result. Same for deletion. That would let us know that conflict resolution is working correctly.

I've updated lance::dataset::transaction::tests::test_conflicts to include the new operations. Is this sufficient?

I couldn't immediately find any other existing tests that use concurrent writers to base additional testing on. Do you know of any tests that use concurrent writers iflance::dataset::transaction::tests::test_conflicts is not sufficient? If no such tests exist yet, I would be tempted to add concurrent writer testing as a separate PR first before finishing off this one.

You are testing the path where we detect conflicts, but not where we resolve them.

For example if you have two concurrent writers:

Writer A: Set metadata x=1

Writer B: Set metadata y=2

The final metadata should include both x=1 and y=2. That's what I want tested.

Here is a test where we validate concurrent appends work right:

lance/rust/lance/src/io/commit.rs

Lines 838 to 839 in 9c361fe

#[tokio::test]

async fn test_concurrent_writes() {

Life has been a bit hectic lately so I've only just had a chance to look at this. I've added some tests here.

dsgibbons · 2024-09-15T01:09:38Z

Updated to address all feedback so far. See above for responses to some individual items. In addition, I have the following comments:

Is "table metadata" the best name for this? It is a bit confusing because metadata is referenced in other locations (e.g. schema, version). Would a name like "config" be better?
I have a SetMetadata operation. Should this perhaps be renamed to MergeMetadata (or MergeConfig if 1. is accepted) to make it clear we are merging with the existing table metadata?

wjones127

Is "table metadata" the best name for this? It is a bit confusing because metadata is referenced in other locations (e.g. schema, version). Would a name like "config" be better?

That's a fair criticism. I'm good with calling it "config".

I have a SetMetadata operation. Should this perhaps be renamed to MergeMetadata (or MergeConfig if 1. is accepted) to make it clear we are merging with the existing table metadata?

I actually am thinking there's no good reason we shouldn't merge SetMetadata and DeleteMetadata into a single UpdateMetadata transaction. Would make things a bit easier.

wjones127 · 2024-09-18T18:25:30Z

protos/table.proto

@@ -85,6 +85,7 @@ message Manifest {
  // * 1: deletion files are present
  // * 2: move_stable_row_ids: row IDs are tracked and stable after move operations
  //       (such as compaction), but not updates.
+  // * 8: table metadata are present


Could you move it to just be under the writer_feature_flags?

protos/table.proto

wjones127 · 2024-09-18T18:30:01Z

protos/transaction.proto

+  // An operation that sets table metadata.
+  message SetMetadata {
+    map<string, string> metadata = 1;
+  }
+
+  // An operation that deletes table metadata.
+  message DeleteMetadata {
+    repeated string metadata_keys = 1;
+  }


I'm thinking maybe we should combine these?

Suggested change

// An operation that sets table metadata.

message SetMetadata {

map<string, string> metadata = 1;

}

// An operation that deletes table metadata.

message DeleteMetadata {

repeated string metadata_keys = 1;

}

// An operation that modifies table metadata.

message UpdateMetadata {

// Values that should be inserted or updated.

map<string, string> upsert_values = 1;

// Keys that should be removed.

repeated string delete_keys = 2;

}

I like it. Should this API merge carry through to the other public table metadata methods?

I don't think it has to, but if you want you could.

Done in 5daaf16 - I decided to still keep the methods separate in dataset.rs for now, but happy to merge if you would prefer.

Co-authored-by: Will Jones <willjones127@gmail.com>

…Config

dsgibbons · 2024-10-12T13:45:56Z

That's a fair criticism. I'm good with calling it "config".

Applied in 5daaf16.

wjones127

Excellent work. Thanks for doing this!

github-actions bot added the enhancement New feature or request label Sep 3, 2024

wjones127 self-requested a review September 3, 2024 15:07

wjones127 reviewed Sep 5, 2024

View reviewed changes

dsgibbons force-pushed the feat/table-metadata branch from 34326f1 to dc474ba Compare September 15, 2024 00:31

dsgibbons force-pushed the feat/table-metadata branch from 732f30e to cbf43db Compare September 15, 2024 01:28

github-actions bot added the python label Sep 15, 2024

dsgibbons force-pushed the feat/table-metadata branch from 4055898 to caf02f1 Compare September 15, 2024 04:55

wjones127 reviewed Sep 18, 2024

View reviewed changes

dsgibbons and others added 6 commits October 13, 2024 00:12

feat: add table_metadata field to Manifest

4e8a016

chore: address initial pr feedback

5ea28eb

test: add concurrent writer tests

86fcf9a

chore: cargo fmt

d38918c

docs: note use of lance. prefix for table metadata

ffd7c09

Co-authored-by: Will Jones <willjones127@gmail.com>

chore: rename table metadata to config and merge SetConfig and Delete…

5daaf16

…Config

dsgibbons force-pushed the feat/table-metadata branch from 77636dc to 5daaf16 Compare October 12, 2024 13:42

chore: only apply feature flag to writers

e62bd82

dsgibbons changed the title ~~feat: add table_metadata field to Manifest~~ feat: add table config Oct 12, 2024

dsgibbons marked this pull request as ready for review October 12, 2024 14:07

wjones127 self-requested a review October 12, 2024 21:54

wjones127 approved these changes Oct 14, 2024

View reviewed changes

wjones127 merged commit 5c8f565 into lancedb:main Oct 14, 2024
25 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add table config #2820

feat: add table config #2820

dsgibbons commented Sep 3, 2024 •

edited

Loading

codecov-commenter commented Sep 3, 2024 •

edited

Loading

wjones127 left a comment

wjones127 Sep 5, 2024

dsgibbons Sep 5, 2024

wjones127 Sep 5, 2024

dsgibbons Sep 15, 2024 •

edited

Loading

wjones127 Sep 18, 2024

dsgibbons Oct 12, 2024

wjones127 Oct 14, 2024

wjones127 Sep 5, 2024

dsgibbons Sep 15, 2024

wjones127 Sep 18, 2024

dsgibbons Oct 12, 2024 •

edited

Loading

dsgibbons commented Sep 15, 2024 •

edited

Loading

wjones127 left a comment

wjones127 Sep 18, 2024

wjones127 Sep 18, 2024

dsgibbons Sep 18, 2024

wjones127 Sep 18, 2024

dsgibbons Oct 12, 2024 •

edited

Loading

dsgibbons commented Oct 12, 2024

wjones127 left a comment

feat: add table config #2820

feat: add table config #2820

Conversation

dsgibbons commented Sep 3, 2024 • edited Loading

codecov-commenter commented Sep 3, 2024 • edited Loading

Codecov Report

wjones127 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dsgibbons Sep 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dsgibbons Oct 12, 2024 • edited Loading

Choose a reason for hiding this comment

dsgibbons commented Sep 15, 2024 • edited Loading

wjones127 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dsgibbons Oct 12, 2024 • edited Loading

Choose a reason for hiding this comment

dsgibbons commented Oct 12, 2024

wjones127 left a comment

Choose a reason for hiding this comment

dsgibbons commented Sep 3, 2024 •

edited

Loading

codecov-commenter commented Sep 3, 2024 •

edited

Loading

dsgibbons Sep 15, 2024 •

edited

Loading

dsgibbons Oct 12, 2024 •

edited

Loading

dsgibbons commented Sep 15, 2024 •

edited

Loading

dsgibbons Oct 12, 2024 •

edited

Loading