feat(namespace): support Lance Namespace across table operations - #35
Merged
Conversation
FANNG1
marked this pull request as draft
June 19, 2026 12:17
added 13 commits
July 16, 2026 19:02
…tion Build on the initial namespace read/write support: - Extend namespace params (namespace_impl/namespace_properties/table_id) to merge_columns, merge_columns_df, create_scalar_index, compact_files, threading namespace kwargs into their commit sites. - Add an explicit daft_lance.write_lance() and make the daft monkeypatch opt-in via patch_daft(); wire write_lance(mode="merge") through namespace. - Fix _declare_table fallback that always raised ImportError on the CreateEmptyTableRequest import; keep only declare_table. - Narrow overwrite resolution to only declare on TableNotFound instead of swallowing every describe_table exception. - Drop dead code (open_lance_dataset, pylance<5 compat) and dedupe the worker dataset-reopen logic into open_dataset_from_open_kwargs. - Cover all entry points with dir-namespace tests plus a Gravitino REST namespace e2e roundtrip. - Work around an upstream daft+lance native teardown SIGSEGV (unrelated to test outcomes) via a pytest_unconfigure hard-exit after reporting.
…nk.start() Constructing a LanceDataSink no longer talks to the namespace or opens the dataset; all resolution moves to start(), which Daft runs once on the driver before the sink is serialized to workers. This keeps declare_table from firing during plan construction and from leaving orphan declared tables behind when local parameter validation fails. Create-mode resolution is now describe-first (check_declared=True): a real existing table fails fast with a clear error suggesting overwrite/append, a declared-only stub is reused as a placeholder (write_fragments switches to overwrite when the stub materialized a dataset), and a lost declare race is recovered by re-describing and classifying. resolve_namespace_table returns a ResolvedNamespaceTable carrying uri/storage_options/placeholder state so the sink does not have to re-describe. Claude-Session: https://claude.ai/code/session_01VyD31uRyBSEmKvPQhLtM5E
…locations Previously io_config was converted to storage options only when a plain uri was passed, so a namespace that resolves to s3://... without vending credentials silently ignored the user's io_config. construct_lance_dataset and LanceDataSink now derive storage options from io_config against the resolved location and layer them as: io_config-derived < user-provided storage_options < namespace-vended. Plain-uri behavior is unchanged (user-provided options still replace io_config-derived ones). Claude-Session: https://claude.ai/code/session_01VyD31uRyBSEmKvPQhLtM5E
daft_lance/ sources and the namespace test files now pass mypy strict. The
bulk came from LanceDataSink._namespace_kwargs being annotated dict[str,
object], which poisoned every **-expansion into typed lance APIs. Also
widens read_lance's default_scan_options to dict[str, Any] to match the
documented usage ({"with_row_address": True}) and the other entry points.
Pre-existing errors in the rest of tests/ are out of scope for this branch.
Claude-Session: https://claude.ai/code/session_01VyD31uRyBSEmKvPQhLtM5E
…phan properties - read_lance / merge_columns(_df) / create_scalar_index / compact_files docstrings now describe table_id / namespace_impl / namespace_properties - namespace_properties without namespace_impl now fails fast instead of being silently ignored - README documents io_config fallback for namespace locations and the DAFT_LANCE_NAMESPACE_CACHE_SIZE environment variable Claude-Session: https://claude.ai/code/session_01VyD31uRyBSEmKvPQhLtM5E
- overwrite-mode resolution now describes with check_declared=True (initial
and declare-race recovery): strict namespace impls 404 a plain describe on
declared-only stubs, so the previous recovery path failed in exactly the
case it existed for. A declared-only stub is a valid overwrite target.
- mode=create over a namespace placeholder that materialized a dataset is
rejected with a clear error when use_mem_wal=True; the MemWAL path cannot
apply the overwrite-placeholder semantics.
- plain-uri entry points treat storage_options={} as unset again (falsy
fallthrough to io_config-derived options), matching pre-branch behavior.
Claude-Session: https://claude.ai/code/session_01VyD31uRyBSEmKvPQhLtM5E
…nt count - pickle a started sink and run write()/finalize() on the copy, codifying the driver-resolves/worker-writes contract instead of relying on manual verification - compact_files test now asserts the fragment count actually shrinks, not just data correctness Claude-Session: https://claude.ai/code/session_01VyD31uRyBSEmKvPQhLtM5E
… storage_options handling
- describe_table/declare_table now pass vend_credentials=True: per the
lance-namespace spec, whether credentials are returned is
implementation-defined when the flag is unset, so the documented
"namespace vends storage_options" behavior was not guaranteed
- LanceDataSink now treats storage_options={} the same as the read entry
points (falsy fallthrough to io_config-derived options); previously
write_lance dropped io_config credentials for that input while read_lance
kept them
Claude-Session: https://claude.ai/code/session_01VyD31uRyBSEmKvPQhLtM5E
The old message predated namespace support and directed users to "the daft-lance package" — which is where they already are. Show the actual read_lance(namespace_impl="rest", ...) usage instead. Claude-Session: https://claude.ai/code/session_01VyD31uRyBSEmKvPQhLtM5E
Module docstring covers the three load-bearing decisions (serialize the triple + per-process client cache, describe-first resolution, defensive error classification), and resolve_namespace_table documents the full create/overwrite/read state machine including the declare-race legs. Claude-Session: https://claude.ai/code/session_01VyD31uRyBSEmKvPQhLtM5E
patch_daft() was introduced on this branch and never released, so it can be removed without a deprecation cycle. Until Eventual-Inc/Daft#7282 delegates DataFrame.write_lance / read_lance natively, namespace users call daft_lance.read_lance / daft_lance.write_lance directly — which the README documents as the primary usage anyway. This also removes the only monkeypatch in the package and avoids having to keep the patch compatible with the upstream signature change coming in #7282. Claude-Session: https://claude.ai/code/session_01VyD31uRyBSEmKvPQhLtM5E
FANNG1
marked this pull request as ready for review
July 16, 2026 10:10
FANNG1
marked this pull request as draft
July 16, 2026 13:54
5 tasks
FANNG1
marked this pull request as ready for review
July 21, 2026 03:50
universalmind303
self-requested a review
July 21, 2026 15:14
Collaborator
|
thanks for the PR @FANNG1, Will take a look asap! |
Collaborator
There was a problem hiding this comment.
I ran this through a pretty deep review & a no-mocks e2e harness (real dir namespace via lance-namespace: create/append/read/pushdowns/merge columns/index/compaction all through the triple, cross-checked with plain pylance). happy path holds up nicely. found a handful of real issues though.
I think the percent-encoding & compat wrapper ones need fixing before merge, the rest can be follow ups.
added 6 commits
July 22, 2026 08:45
Namespaces vend URI-encoded locations: a table under `daft lance/` comes back from the dir namespace as `file:///.../daft%20lance/t.lance`. _normalize_file_uri took parsed.path verbatim, so writes landed in a literally-named `daft%20lance` directory while the subsequent describe_table resolved to the decoded one — create appeared to succeed and reads found nothing. Claude-Session: https://claude.ai/code/session_018YLUk2vAhX3jXFL9D7za7u
The wrapper returned only .dataset, discarding the resolved uri, open_kwargs, managed_versioning and default_scan_options that construct_lance_dataset_handle resolves. Callers going through it lost the reusable open context that distributed workers need to reopen the dataset, and nearest-vector scan defaults silently became None. It had no callers inside daft_lance and was never exported, so remove it rather than reconstructing the lost state from dataset private attributes. This also keeps a single explicit context type as the foundation for the serializable access context that credential refresh will need. Claude-Session: https://claude.ai/code/session_018YLUk2vAhX3jXFL9D7za7u
The namespace write path declares the table up front as a metadata-only reservation, then the mem-WAL path asked Lance for a namespace-aware write_dataset(mode="create"), which declared the same table again and raised TableAlreadyExistsError. The combination could never succeed, and _ensure_mem_wal_dataset's except clause does not cover the namespace error types either. Fail at construction time with an actionable message instead of claiming support. Making it work requires the mem-WAL path to skip our declare and let the native create own table creation, which is left to a follow-up. Claude-Session: https://claude.ai/code/session_018YLUk2vAhX3jXFL9D7za7u
Adds unit coverage for _normalize_file_uri (spaces, non-ASCII, literal percent, object-store passthrough) and an end-to-end roundtrip through a namespace root and table id that both percent-encode, asserting data lands under the decoded name. Also asserts namespace + use_mem_wal is rejected, and moves the two construct_lance_dataset call sites to construct_lance_dataset_handle. Claude-Session: https://claude.ai/code/session_018YLUk2vAhX3jXFL9D7za7u
LanceDataset.__reduce__ carries only (uri, storage_options, version, manifest, ...); _namespace_client, _table_id and _namespace_client_managed_versioning are assigned after construction and are dropped by pickle. Compaction, scalar index and merge all shipped the driver's live dataset into their UDFs, so workers silently lost the table's namespace identity and committed as if it were a uri table. Introduce a frozen, serializable DatasetOpenContext carrying the physical uri, the driver's resolved numeric version, the effective initial storage options, the namespace triple, managed_versioning and the worker read options. Workers rebuild the namespace client per process and reopen; the driver keeps its live dataset for planning, validation and commits. Workers open through the low-level LanceDataset constructor with the physical uri rather than lance.dataset(None, namespace_client=...), which resolves the location with a describe_table on every call. Measured with pylance's ops_metrics: the low-level open costs zero namespace calls, the high-level one costs a round-trip per task. Because the context supplies uri, storage options and namespace kwargs, the internal entry points drop those four parameters instead of carrying a second copy of the same state; FastPathFragmentWriter loses its private uri/storage_options fields for the same reason. Each UDF instance opens once, lazily -- the reopen costs a pinned-manifest read, so it must not sit on the per-call path. The serialized manifest is deliberately not carried, keeping the task payload independent of fragment count at the cost of that manifest read. Version semantics are preserved: workers pin the snapshot the driver planned against, and only the index coordinator steps that must observe worker output reopen at latest. Known limitation: workers start from the storage options the driver resolved. pylance installs its namespace refresh provider only when those options are present, and refresh fires on expires_at_millis, so tables whose catalog vends no expiry metadata will not refresh mid-task. Full credential refresh is daft-engine#53. Mem-WAL still rejects namespace writes (daft-engine#54) and builds a uri-only context. Scan keeps its existing open_kwargs path untouched. Claude-Session: https://claude.ai/code/session_01Ss3gSAbK3FwfPmWqk1CSbJ
… declare race Two review findings. Scan workers were not pinned to the version the driver planned against. construct_lance_dataset_handle stored the caller's `version` argument in open_kwargs, which defaults to None, and open_kwargs is exactly what crosses to scan workers -- so `version=None` there means every task independently opens latest. Reproduced both failure modes: a compaction landing between planning and execution leaves workers looking for fragment ids that no longer exist (planned [0,1,2], worker sees [3], get_fragment(0) returns None), and an overwrite silently substitutes different data with no error at all (driver planned 3 rows, worker read 1 different row). open_kwargs now carries the resolved numeric `dataset.version`, and `asof` is dropped since it is an input to that resolution rather than something workers should re-evaluate. pylance lets `version` win when both are passed, so dropping it is belt-and-braces rather than load bearing. This is the same invariant DatasetOpenContext already enforces for the maintenance paths; scan was left out when that change deliberately avoided touching the scan state model. Overwrite could also lose a declare race: when the initial describe raises TableNotFoundError but a rival writer declares before our own declare_table lands, TableAlreadyExistsError propagated and the overwrite failed. Overwrite targets whatever exists now, so it re-describes on that conflict. `create` is unchanged -- for it the conflict is the correct answer, and a test pins that distinction. Claude-Session: https://claude.ai/code/session_01Ss3gSAbK3FwfPmWqk1CSbJ
universalmind303
approved these changes
Jul 22, 2026
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.
Closes #50 (motivating use case: Gravitino Lance REST server managed tables + Daft multimodal processing).
Summary
daft_lance.read_lance, the newdaft_lance.write_lance,LanceDataSink, merge/index/compaction operations, and worker scan reconstructionLanceDatasetHandleinstead of attaching Daft-private attributes toLanceDatasetDatasetOpenContextinstead of shipping a liveLanceDataset, so maintenance tasks keep their namespace identitydeclare_tablerace by re-describing, while leavingcreateto fail on the conflictReview fixes
Three blocking issues raised in review, all fixed here:
Namespace locations are now percent-decoded. Namespaces vend URI-encoded
locations: a table under
daft lance/came back from the dir namespace asfile:///.../daft%20lance/t.lance, and_normalize_file_uritookparsed.pathverbatim. Writes landed in a literally-named
daft%20lancedirectory while thesubsequent
describe_tableresolved to the decoded one, socreateappeared tosucceed and reads found nothing. Covered by unit tests (spaces, non-ASCII,
literal percent, object-store passthrough) plus an end-to-end roundtrip through
a namespace root and table id that both encode.
The
construct_lance_dataset()compatibility wrapper is removed. Itreturned only
.dataset, discarding the resolved uri,open_kwargs,managed_versioninganddefault_scan_options.open_kwargsis theserialization boundary to workers (
lance_scan.pyputs it in the scan task'sfunc_args, andopen_dataset_from_open_kwargsrebuilds the namespace clientfrom it), so callers going through the wrapper left workers with
{}: no vendedcredentials, no namespace triple, and no pinned version, meaning partitions of
one scan could read different snapshots. The stripped-
nearestregression was asecond symptom — the dataset's own
_default_scan_optionshas already hadnearestremoved by then, so it cannot be recovered from the object. Ratherthan reconstruct any of this from
LanceDatasetprivate attributes, the wrapperis deleted; it had no callers inside
daft_lanceand was never exported.Namespace +
use_mem_wal=Trueis now rejected at construction time. Thenamespace path declares the table up front as a metadata-only reservation, then
_ensure_mem_wal_dataset()asked Lance for a namespace-awarewrite_dataset(mode="create"), which declares the same table again —DeclareTableis not idempotent, so this always raisedTableAlreadyExistsError. The surroundingexcept (ValueError, FileNotFoundError, OSError)does not cover the namespace error types either. Making it workrequires hoisting the native create to the driver's
start()so mem-WAL tablecreation is not raced by N workers; that is left to a follow-up rather than
claiming support that cannot work.
Distributed worker reopen
LanceDataset.__reduce__carries only(uri, storage_options, version, manifest, ...)._namespace_client,_table_idand_namespace_client_managed_versioningare assigned after construction, so pickle drops them. Compaction, scalar index
and merge each shipped the driver's live dataset into their UDFs, which meant
workers silently lost the table's namespace identity and committed as though it
were a plain URI table.
Workers now receive a frozen, serializable
DatasetOpenContext— physical uri,the driver's resolved numeric version, effective initial storage options, the
namespace triple,
managed_versioning, and the worker read options — and reopenfrom it, rebuilding the namespace client per process through the existing
lru_cache. The driver keeps its live dataset for planning, validation andcommits.
Reopen goes through the low-level
LanceDataset(uri, namespace_client=...)constructor rather than
lance.dataset(None, namespace_client=..., table_id=...),which resolves the location with a
describe_tableon every call. Measuredwith pylance's
ops_metrics: the low-level open costs zero namespace calls(open and scan), the high-level one costs a round-trip per task. Each UDF
instance opens once, lazily — the reopen costs a pinned-manifest read, so it must
not sit on the per-call path.
The serialized manifest is deliberately not carried, keeping the task payload
independent of fragment count at the cost of that manifest read. This matches
the lance-spark and Lance Ray maintenance paths. Version semantics are preserved:
workers pin the snapshot the driver planned against, and only the index
coordinator steps that must observe worker output reopen at latest.
Scan workers are pinned too.
construct_lance_dataset_handlestored thecaller's
versionargument inopen_kwargs— defaulting toNone— andopen_kwargsis precisely what crosses to scan workers, so each taskindependently opened latest. Both failure modes reproduce: a compaction
landing between planning and execution leaves workers hunting fragment ids that
no longer exist (planned
[0,1,2], worker sees[3],get_fragment(0)returnsNone), and an overwrite silently substitutes different data with no error atall.
open_kwargsnow carries the resolved numericdataset.version, andasofis dropped since it is an input to that resolution rather than somethingworkers should re-evaluate.
Since the context supplies uri, storage options and namespace kwargs, the
internal entry points drop those four parameters rather than carry a second copy
of the same state, and
FastPathFragmentWriterloses its privateuri/storage_optionsfields for the same reason. Scan otherwise keeps itsexisting
open_kwargspath — unifying the two state models is left to aseparate change so this one does not perturb tag/default-scan-option behavior.
Known limitation. Workers start from the storage options the driver resolved.
pylance installs its namespace refresh provider only when those options are
present (
python/src/dataset.rs, gated oninitial_storage_options.is_some()),and the accessor refreshes on
expires_at_millis; the upstream provider'sDescribeTableRequestdoes not setvend_credentials=True. So the wiring isrestored and refresh is possible, but reliable refresh depends on the catalog
vending expiry metadata and credentials — a task outliving its credentials can
still fail. Full credential refresh, including expired-credential tests, is
tracked separately.
Follow-ups (not in this PR)
vend_credentials=Trueon the refresh request, expired-credential andreplacement-token tests, and refresh failure/retry behavior. It should also
carry a switch for whether workers may rebuild the namespace client at all —
lance-spark's
executorCredentialRefreshdefaults to true but is explicitlydisableable, since not every catalog is reachable from the worker network.
protocol-allowed intermediate state; the namespace spec deliberately keeps
catalogs out of slow, unreliable data deletion. Cleanup should be
DeregisterTablescoped to a table this operation declared and confirmed stilldeclared-only, not an unconditional
drop_tableon worker failure — and Daft'sDataSinkhas no abort callback to hang it on.Validation
.venv/bin/pytest -q(347 passed, 5 skipped, 2 xfailed, 2 xpassed).venv/bin/ruff format --check daft_lance tests.venv/bin/ruff check daft_lance tests.venv/bin/mypy daft_lancegit diff --checkCross-repo plan
daft_lance.write_lancein this PR looks like a copy ofDataFrame.write_lance, but thedirection is the opposite: Eventual-Inc/Daft#6957 already migrated all of
daft.io.lancetothis package, and
DataFrame.write_lance's inline body is the last piece left behindupstream. Eventual-Inc/Daft#7282 proposes reducing it to a thin delegation to
daft_lance.write_lance, making this module the single canonical implementation.Until Eventual-Inc/Daft#7282 lands, namespace users call
daft_lance.read_lance/daft_lance.write_lancedirectly. (An earlier revision of this PR shipped apatch_daft()monkeypatch as interim glue for Daft's native entry points; it was removed in favor of the
direct entry points since it had never been released.)