refactor: split lance-core into lance-core, lance-io, lance-file, and lance-table #1852

westonpace · 2024-01-19T15:11:53Z

There is very little code change in this PR. It is primarily moving files around.

The only substantial change should be a change to the FileWriter. It is now either FileWriter<NotSelfDescribing> (for testing in lance-file) and FileWriter<ManifestDescribing> for use everywhere else. This is because a lance file writes the schema as a manifest and manifest is part of the table format. In the future we could fix this by creating a schema message in the protobuf in lance-file and changing the FileWriter and FileReader to read this. However, this would be a breaking change and would require a bump in the lance file version (or feature flags). I'd prefer to save that change for later.

westonpace · 2024-01-19T15:12:39Z

Putting this in draft because it depends on #1842 and #1848

westonpace · 2024-01-19T20:01:09Z

rust/lance-file/src/writer.rs

+/// The file format currently includes a "manifest" where it stores the schema for
+/// self-describing files.  Historically this has been a table format manifest that
+/// is empty except for the schema field.
+///
+/// Since this crate is not aware of the table format we need this to be provided
+/// externally.  You should always use lance_table::io::manifest::ManifestDescribing
+/// for this today.
+#[async_trait]
+pub trait ManifestProvider {
+    /// Store the schema in the file
+    ///
+    /// This should just require writing the schema (or a manifest wrapper) as a proto struct
+    ///
+    /// Note: the dictionaries have already been written by this point and the schema should
+    /// be populated with the dictionary lengths/offsets
+    async fn store_schema(
+        object_writer: &mut ObjectWriter,
+        schema: &Schema,
+    ) -> Result<Option<usize>>;
+}


This is new

westonpace · 2024-01-19T20:01:46Z

rust/lance-file/src/writer.rs

+    ///
+    /// The offsets and lengths of the written buffers are stored in the given
+    /// schema so that the dictionaries can be loaded in the future.
+    async fn write_dictionaries(writer: &mut ObjectWriter, schema: &mut Schema) -> Result<()> {


No change here, just refactored this into its own method.

westonpace · 2024-01-19T20:02:11Z

rust/lance-file/src/writer.rs

+        Self::write_dictionaries(&mut self.object_writer, &mut self.schema).await?;
+        let pos = M::store_schema(&mut self.object_writer, &self.schema).await?;


This is where the new ManifestProvider gets used.

westonpace · 2024-01-19T20:03:39Z

rust/lance-table/protos/table.proto

@@ -231,156 +232,3 @@ message DeletionFile {
  uint64 num_deleted_rows = 4;
 } // DeletionFile

-// Metadata of one Lance file.


The old format.proto split into file.proto and table.proto. In the process I changed the package field. This should be a non-breaking change for protobuf as long as we aren't using these messages is a protobuf Any field (we aren't)

westonpace · 2024-01-19T20:04:27Z

rust/lance-table/src/format.rs

 pub const MAJOR_VERSION: i16 = 0;
 pub const MINOR_VERSION: i16 = 1;
 pub const MAGIC: &[u8; 4] = b"LANC";
-pub const INDEX_MAGIC: &[u8; 8] = b"LANC_IDX";


I removed this. It was never used anywhere. I think it's a good idea to a different magic at the end of index files. But we aren't doing that today.

westonpace · 2024-01-19T20:06:52Z

rust/lance-file/src/datatypes.rs

+    }
+}
+
+pub struct Fields(pub Vec<pb::Field>);


Minor introduction here of Fields and (later) FieldsWithMeta. This is because Field and Schema are in lance-core but the to/from protobuf methods are in lance-file. You can't do impl From<X> for Vec<Y> if X doesn't belong to your crate (even if Y does).

I guess it makes some sense to put this here as it is the deserialization part, but part of me still thinks it's easier to just keep this associated with the Schema definition. We could consider having a separate schema.proto.

I think making a schema.proto is fine (can do later). I agree it is a little inconsistent.

westonpace · 2024-01-19T20:07:48Z

rust/lance-core/Cargo.toml

@@ -23,8 +23,6 @@ arrow-select.workspace = true
 async-recursion.workspace = true
 async-trait.workspace = true
 lance-arrow.workspace = true
-aws-config.workspace = true
-aws-credential-types.workspace = true
 aws-sdk-dynamodb = { workspace = true, optional = true }


I suspect there are more dependencies that can be removed. I want to take a pass at doing this later.

eddyxu

YOLO

wjones127 · 2024-01-19T20:55:33Z

rust/lance-table/protos/table.proto

I actually liked that the protos had a top-level folder. This separated the specification of the format from our particular implementation.

Also, portions of our format docs make direct references to these files, so if we split them like this we need to fix those references. Example:

lance/docs/format.rst

Lines 61 to 65 in f3d2ae3

.. literalinclude:: ../protos/format.proto

:language: protobuf

:linenos:

:start-at: message Metadata {

:end-at: } // Metadata

Ok, I've moved the docs back to the central location (I hadn't realized/remembered it was a symlink) but I kept the split of format.proto into file.proto and table.proto. I've fixed the relevant rst files (and done a search for any other references to format.proto and didn't find any)

wjones127 · 2024-01-19T21:07:28Z

rust/lance-io/README.md

If you want to, there is a nice directive you can use to include this readme as the module-level docs:

#![doc = include_str!("path/to/docs.md")]

I'll save this for a follow-up as we aren't doing it anywhere else and I probably wouldn't be able to resist the temptation to clean the module-level docs up some if I'm going to expose them.

However, this is a neat trick!

wjones127

Seems fine, except I think we should keep the proto files in that consolidated folder.

…ance-table

…r file.proto

westonpace requested a review from eddyxu January 19, 2024 15:12

westonpace mentioned this pull request Jan 19, 2024

refactor: split any reference to fragment or manifest out of the file reader #1848

Merged

westonpace force-pushed the refactor/split-io-crates branch 2 times, most recently from 8acfd80 to d3c202f Compare January 19, 2024 20:00

westonpace commented Jan 19, 2024

View reviewed changes

westonpace added 3 commits January 19, 2024 12:10

Split lance-core into lance-core, lance-io, lance-file, and lance-table

f9bce75

Fix after rebase

655a63e

Fix after rebase

b0e2708

westonpace force-pushed the refactor/split-io-crates branch from d3c202f to b0e2708 Compare January 19, 2024 20:12

Apply clippy suggestions

b9d54e1

westonpace marked this pull request as ready for review January 19, 2024 20:15

Apply clippy suggestions

2efd15c

eddyxu approved these changes Jan 19, 2024

View reviewed changes

wjones127 reviewed Jan 19, 2024

View reviewed changes

westonpace added 4 commits January 19, 2024 13:26

Restore protos to the top-level proto folder

e138284

Moved protos back to top-level directory. Moved dynamodb feature to l…

5417d71

…ance-table

Fix old references to format.proto which are now either table.proto o…

3a01f49

…r file.proto

Fix dynamodb dependency

1cedfc3

wjones127 approved these changes Jan 19, 2024

View reviewed changes

Fix dynamodb import errors revealed by addition of feature

2545179

westonpace merged commit a8def71 into lancedb:main Jan 19, 2024
17 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: split lance-core into lance-core, lance-io, lance-file, and lance-table #1852

refactor: split lance-core into lance-core, lance-io, lance-file, and lance-table #1852

westonpace commented Jan 19, 2024

westonpace commented Jan 19, 2024

westonpace Jan 19, 2024

westonpace Jan 19, 2024

westonpace Jan 19, 2024

westonpace Jan 19, 2024

westonpace Jan 19, 2024

westonpace Jan 19, 2024

wjones127 Jan 19, 2024

westonpace Jan 19, 2024

westonpace Jan 19, 2024

eddyxu left a comment

wjones127 Jan 19, 2024

westonpace Jan 19, 2024

wjones127 Jan 19, 2024

westonpace Jan 19, 2024

wjones127 left a comment

		Self::write_dictionaries(&mut self.object_writer, &mut self.schema).await?;
		let pos = M::store_schema(&mut self.object_writer, &self.schema).await?;

	.. literalinclude:: ../protos/format.proto
	:language: protobuf
	:linenos:
	:start-at: message Metadata {
	:end-at: } // Metadata

refactor: split lance-core into lance-core, lance-io, lance-file, and lance-table #1852

refactor: split lance-core into lance-core, lance-io, lance-file, and lance-table #1852

Conversation

westonpace commented Jan 19, 2024

westonpace commented Jan 19, 2024

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eddyxu 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

wjones127 left a comment

Choose a reason for hiding this comment