Conversation
Extends the existing TableFormat abstraction (feast-dev#5650) with Lance rather than introducing a separate data source, so Lance is addressed the same way Iceberg, Delta and Hudi already are. Closes part of feast-dev#6899. LanceFormat carries catalog/namespace addressing plus an optional pin to a dataset version or tag. Because SparkSource already drives its reader generically from table_format.format_type.value and table_format.properties, this works with SparkSource with no changes to it: format_type.value is "lance", and the pin is mirrored into properties as lance.version / lance.tag. version is validated as >= 1 so the Python and proto semantics agree. Lance dataset versions start at 1 and the proto treats 0 as unset, so without that guard version=0 would not round-trip, since to_proto/from_proto read 0 as absent. version and tag are mutually exclusive, because a tag already resolves to a version. Only DataFormat_pb2 is regenerated, using grpcio-tools 1.62.3 so the emitted gencode stays at the 4.25.1 level the other checked-in protos use. Regenerating with the pinned grpcio-tools 1.84.0 instead emits gencode that calls ValidateProtobufRuntimeVersion for protobuf 7.35.1, which would break the declared protobuf>=4.24.0 floor for that one module. DataSource_pb2 is deliberately left untouched. It is already stale against DataSource.proto on master, missing ConnectionRef entries, and regenerating it produces ~125 lines of churn unrelated to this change. Signed-off-by: hao-xu5 <hxu44@apple.com>
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #6925 +/- ##
==========================================
+ Coverage 48.64% 48.68% +0.04%
==========================================
Files 427 427
Lines 53864 53906 +42
Branches 7849 7858 +9
==========================================
+ Hits 26204 26246 +42
+ Misses 25792 25788 -4
- Partials 1868 1872 +4
Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds Lance to the existing
TableFormatabstraction from #5650, rather than introducing a separate data source. Addresses the core of #6899.Why
TableFormatand not a new sourceTableFormatalready models Iceberg, Delta and Hudi as formats a source can carry, with catalog/namespace addressing and a properties bag. Lance fits that shape exactly, so it needs no new source class, no new offline store, and no changes to any existing consumer.It works with
SparkSourceunchangedSparkSourcealready drives its reader generically:So mirroring the pin into
propertiesis what makes this fall out for free. Verified:Pin semantics
version/tagis the Lance-shaped instance of #5782. Two invariants:version >= 1is enforced. Lance dataset versions start at 1 and the proto treats0as unset, so without the guardversion=0would not round-trip —to_proto/from_protoread0as absent. Rejecting it keeps the Python and proto semantics in agreement rather than silently dropping a pin.versionandtagare mutually exclusive, since a tag already resolves to a version.On the broader question @jfw-ppi raised in #5782 — whether a pin can change the response shape — the position I'd argue for is that a pin selects data, never shape: the declared
FeatureViewschema stays the contract, and a pinned version whose schema disagrees should fail explicitly. That belongs in whatever consumes the pin, so it is not in this PR, but the format carries enough information to enforce it.Proto regeneration, deliberately constrained
Two things worth flagging, both about not doing the obvious thing.
1. Generated with
grpcio-tools==1.62.3, not the pinned1.84.0.Regenerating with the pinned toolchain emits gencode that opens with:
pyproject.tomldeclaresprotobuf>=4.24.0. That call would hard-fail for anyone on protobuf 4/5/6 — andgoogle.protobuf.runtime_versiondoes not exist in 4.x at all, so it is anImportErroron that one module while every other proto still imports. Using 1.62.3 emits4.25.1-level gencode, matching every other checked-in proto, and keeps the declared floor honest.Worth noting independently: the checked-in protos are at gencode
4.25.1while the requirements pingrpcio-tools==1.84.0/protobuf==7.36.2, so a full regeneration on master today would touch ~70 files and raise the effective protobuf floor. That looks like something to decide on purpose rather than as a side effect of a feature PR.2.
DataSource_pb2is left untouched on purpose.Regenerating also rewrites
DataSource_pb2.py/.pyi(~125 lines), but that churn is pre-existing — I verified it reproduces on pristinemasterwith no changes at all. The checked-in copy is missing_CONNECTIONREF_PARAMSENTRYentries, i.e. it is stale againstDataSource.proto. Happy to fix that separately; it does not belong here.Net result is 5 files, and only
DataFormat_pb2regenerated.Scope
This adds the format descriptor. It does not add a Lance reader or offline store — that is the follow-on discussed in #6899, and it is why the tests here cover
LanceFormatsemantics rather than reading real datasets (no new test dependency onpylance).Also explicitly not proposing Lance as an online store: it is a format plus indexes, not a low-latency KV service, and its write path is columnar while
online_write_batchis row-oriented proto.Testing
11 new tests in
test_table_format.py(24 total in the file): creation, minimal construction, version pin, tag pin,version < 1rejection,version+tagrejection, dict/json/proto round-trips, unpinned not coming back as version 0, and factory dispatch.test_table_format.py— 24 passedtest_utils.py,test_data_sources.py,test_types.py— 52 passed, 1 skippedmypy feast/table_format.py— clean, no issuesruff check/ruff format --check— cleanRelated: #6899, #5782, #5650, #6499