| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Sorry, something went wrong.
|
⚠️ Please install the Codecov Report❌ Patch coverage is 1.19760% with 165 lines in your changes missing coverage. Please review.
@@ Coverage Diff @@
## master #6943 +/- ##
==========================================
- Coverage 49.08% 48.99% -0.09%
==========================================
Files 433 435 +2
Lines 54332 54505 +173
Branches 7917 7948 +31
==========================================
+ Hits 26667 26703 +36
- Misses 25788 25926 +138
+ Partials 1877 1876 -1
... and 6 files with indirect coverage changes Continue to review full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Sorry, something went wrong.
There was a problem hiding this comment.
Vector schema inference and validation have correctness gaps, and the required Lance dependencies are not installable through a Feast extra.
Review effort: Balanced
Findings: 1 · 3
· 1
Adds native, non-JVM Lance reads through the DuckDB offline store, including catalog addressing, version pinning, and schema validation.
Changes:
| File | Description |
|---|---|
| lance_source.py | Implements Lance source reads, serialization, and validation. |
| lance/__init__.py | Exports the Lance source API. |
| duckdb.py | Integrates Lance reads with DuckDB retrieval. |
| test_lance_source.py | Tests source behavior and DuckDB integration. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Sorry, something went wrong.
|
@ntkathole @franciscojavierarceo PTAL when you have time |
Sorry, something went wrong.
Adds LanceSource and teaches the DuckDB offline store to read it, completing the read half of feast-dev#6899. LanceFormat landed in feast-dev#6925 as a format descriptor; nothing read Lance until now. Lance already worked through SparkSource, which drives its reader generically from table_format.format_type.value and table_format.properties. What was missing is a path that needs no JVM, which is also the real test of whether the DataSource abstraction is engine-agnostic rather than Spark-agnostic in name only. Placement: a new source read by the existing DuckDB store, rather than a Lance offline store or an extension of FileSource. FileSource is the wrong host. Its format axis is already taken by file_format, so adding table_format would give one source two overlapping format axes. It is also read by two stores with incompatible contracts: duckdb._read_data_source dispatches on type, while dask._read_datasource has no dispatch seam and reads file_options.uri unconditionally as Parquet, and asserts isinstance(..., FileSource) in three places. Decisively, Lance's catalog addressing has no path to put in FileSource.path, so the catalog-based layer would not fit the class even if the path-based one did. A Lance offline store would be the wrong 120 lines. duckdb.py is a binding that injects reader and writer callbacks into the engine in ibis.py, so a Lance store would be a near-copy of it plus a repo_config entry, and would force a choice between Lance and Parquet instead of mixing them in one feature service. Reading a source as ibis.memtable(arrow_table) in the DuckDB store already has two precedents, IcebergSource and MlflowDatasetSource. Following them leaves ibis.py untouched, so the point-in-time join, TTL handling, field mapping and ODFVs work unchanged, and no edit to repo_config.py or data_source.py is needed because CUSTOM_SOURCE plus data_source_class_type is self-describing. Both addressing modes work: a uri, and catalog/namespace/table through namespace_client and table_id. Pin semantics follow what was argued on feast-dev#5782 and feast-dev#6925: a pin selects data, never shape. get_table_column_names_and_types reads the pinned schema so feast apply infers what reads will actually see; a pre-flight check fails with a message naming the pin when a pinned version cannot satisfy the declared schema; and vector widths go through _validate_vector_field_lengths from feast-dev#6909 rather than a second validator. Tests use the dir namespace implementation, which exercises the same namespace_client and table_id code path as a remote catalog with no server required. 43 tests, including a demonstration that a tag pin returns earlier data after the dataset has been overwritten for the same entity and timestamp. Read-only for now: _write_data_source is untouched, so a LanceSource is not yet a persist target and there is no SavedDatasetLanceStorage. Signed-off-by: hao-xu5 <hxu44@apple.com>
Signed-off-by: HaoXuAI <sduxuhao@gmail.com>
feast-dev#6943 made a `LanceSource` readable without Spark. This makes one writable, through the same two addressing modes: a uri, or a `namespace_client` plus `table_id` resolved through a Lance namespace, so a catalog-addressed dataset is committed through its catalog rather than behind its back. `_write_data_source` gains a `LanceSource` branch, following the `IcebergSource` branch already there, which makes the path reachable from `DuckDBOfflineStore.offline_write_batch`. The `isinstance` narrowing the read path open-coded is now a shared `_as_lance_source` helper used by both, rather than a second copy of the optional-import guard. Three things are settled before Lance is called. A pinned source is refused. A Lance commit always produces a new version, so `write_dataset` has no `version` argument at all; a write through a source pinned to version 1 or to a tag would succeed and then be invisible through the very source that performed it. Measured: after appending to a dataset tagged `prod` at version 1, the unpinned source reads 5 rows and the pinned source still reads 3. An absent dataset is created rather than appended to, because Lance has no create-or-append mode and rejects `create` on an existing dataset. An existing dataset's vector widths are compared against the incoming data. Lance rejects an `append` whose schema disagrees, but it accepts an `overwrite` that replaces a 8-wide embedding column with a 32-wide one, silently rewriting the declared shape and leaving a version history whose vectors are not mutually comparable, with every index built on the old width invalidated. No legitimate schema evolution changes an embedding's dimension, so this is refused. The guard is narrow on purpose: only vector widths are policed, and other type changes remain Lance's business. The declared `vector_length` is checked at the store entry point rather than in the writer callback, which is handed a `DataSource` and so cannot see the feature view. It reuses `_validate_vector_field_lengths` from feast-dev#6909 rather than adding a second implementation. Without it, creating a dataset at a width other than the declared one would succeed and fail only on the next read. 19 tests added, all using the `dir` namespace implementation for the catalog-based cases so no server is needed. Signed-off-by: hao-xu5 <hxu44@apple.com>
| Back | FazBrowse Home | New Git URL |
Completes the read half of #6899. LanceFormat landed in #6925 as a format descriptor; nothing read Lance until now.
Lance already worked through SparkSource, which drives its reader generically from table_format.format_type.value and table_format.properties. What was missing is a path that needs no JVM — which is also the real test of whether the DataSource abstraction is engine-agnostic rather than Spark-agnostic in name only.
Placement, and why not the two obvious alternatives
A new LanceSource read by the existing DuckDB store — not a Lance offline store, and not an extension of FileSource.
FileSource is the wrong host. Its format axis is already occupied by file_format (ParquetFormat/DeltaFormat), so adding table_format gives one source two overlapping format axes. It is also read by two stores with incompatible contracts: duckdb._read_data_source dispatches on type, while dask._read_datasource has no dispatch seam at all — it reads file_options.uri unconditionally as Parquet and hard-asserts isinstance(..., FileSource) in three places, so a third format would break silently under Dask. Decisively: Lance's catalog addressing (catalog/namespace/table) has no path to put in FileSource.path, so the catalog-based layer would not fit the class even if the path-based one did.
A Lance offline store would be the wrong 120 lines. duckdb.py is not an engine — it is a binding that injects _read_data_source/_write_data_source callbacks into the real engine in ibis.py. A LanceOfflineStore would be a near-verbatim copy of it plus a repo_config.py entry, and would force users to choose Lance instead of Parquet rather than mixing them in one feature service.
Reading a source as ibis.memtable(arrow_table) in the DuckDB store already has two precedents — IcebergSource and MlflowDatasetSource. Following them means ibis.py is untouched, so the point-in-time join, TTL handling, field mapping and ODFVs all work unchanged, and neither repo_config.py nor data_source.py needs an edit because CUSTOM_SOURCE + data_source_class_type is self-describing in the registry proto.
Net: +47 lines to duckdb.py, one new source module.
Both addressing modes work
Pin semantics: a pin selects data, never shape
Following what was argued on #5782 and #6925. Three things enforce it:
Demonstrated rather than asserted — the dataset is overwritten with a corrected value for the same (entity, timestamp), and the pinned read still returns the original:
Testing
43 tests, all using the dir namespace implementation — it exercises the identical namespace_client + table_id code path as a remote catalog, so the suite needs no server. Point-in-time correctness is covered for both the path-based and catalog-based variants, with a 12:30 query selecting the 11:00 row and never the 13:00 one.
Regression evidence, rebased onto current master. Failure names compared rather than counts, since the suite has flaky members:
ruff check and ruff format --check clean on all four files. mypy --ignore-missing-imports clean. (Bare mypy reports pre-existing missing-stub noise for pyarrow/pandas/ibis/pyiceberg/deltalake; the repo has no mypy config section.)
Known limitations, deliberately
Related: #6899, #6925, #6909, #5782, #5652