fix(python): correct the asof and reader input annotations - #8955
Open
jonasdedden wants to merge 3 commits into
Open
jonasdedden wants to merge 3 commits into
jonasdedden wants to merge 3 commits into
Conversation
![lance-gatekeeper[bot]](https://avatars.githubusercontent.com/in/4433141?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
lance-gatekeeper
Bot
added
the
K-changes
lance-gatekeeper
Bot
removed
the
K-changes
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
lance-gatekeeper
Bot
added
K-approved
jonasdedden
force-pushed
the
fix/python-structural-optional-types
branch
from
September 2, 2026 20:57
8490a06 to
3ccef5f
Compare
jonasdedden
changed the title
lance-gatekeeper
Bot
removed
K-approved
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
lance-gatekeeper
Bot
added
K-approved
The stubs bundled with pandas declare `Timestamp.__new__` as returning `Self | NaTType`, so `lance.dataset(asof=pd.Timestamp(...))` was rejected by strict type checkers. Name `NaTType` in `ts_types`; `pd.Timestamp` needs no member of its own because it subclasses `datetime`. `NaT` subclasses `datetime` too, so it previously passed through `sanitize_ts` untouched and then compared false against every version timestamp, surfacing as a misleading "earlier than the first version" error. Reject it at the boundary instead. The pandas string-parsing branch was guarded by `_check_for_pandas(ts)`, which inspects the argument's MRO and is therefore always false for a `str`. That branch had never run: `asof="2026-01-01"` raised "Try installing Pandas" on machines that had pandas installed. Guard on `_PANDAS_AVAILABLE` instead. The new typing regression file joins the pyright target and pins the accepted and rejected `asof` types in both directions, so the annotation also fails the type check if it becomes too permissive.
`ReaderLike` listed `pd.Timestamp` rather than a dataframe type, and
reached the Arrow classes through `pa.dataset.Dataset`, which type
checkers cannot resolve because `pyarrow` does not expose `dataset` as an
attribute. The unresolved member made the whole union accept anything, so
neither defect surfaced and the union drifted out of step with
`_coerce_reader`: pandas and Polars dataframes, HuggingFace datasets,
column dicts, row dicts and Pydantic model instances are all coerced at
runtime but were absent from the annotation.
List every input `_coerce_reader` handles, import the Arrow classes
directly, and dispatch on the optional dependencies the way the rest of
the codebase does: `_check_for_polars` plus a real `isinstance` replaces
the duplicated `__module__.startswith("polars")` string matching, which
also lets a type checker narrow the branch. `LanceDataset` moves to a
function-local import because `lance.dataset` names both this module and
a function on the package.
`types.py` cannot join the pyright target yet: without `pyarrow-stubs`,
`isinstance(x, pa.Table)` does not narrow and every branch reports an
error, so that is left for a follow-up along with the stub dependency.
The union members are pinned in the typing regression file instead.
jonasdedden
force-pushed
the
fix/python-structural-optional-types
branch
from
September 2, 2026 21:46
3ccef5f to
9775b51
Compare
lance-gatekeeper
Bot
removed
K-approved
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
lance-gatekeeper
Bot
added
K-approved
ReaderLike accepts anything under the repo's pyright setup (no pyarrow stubs), so its acceptance checks could not fail. Fold sanitize_ts runtime tests into parametrized cases and import pydantic only for type checking.
lance-gatekeeper
Bot
removed
K-approved
Contributor
Author
|
@Xuanwo any review? :) |
Contributor
There was a problem hiding this comment.
The timestamp contract is aligned between typing and runtime: ordinary pandas timestamps are accepted, while NaT is rejected. The expanded reader union matches the existing reader-dispatch paths, and the latest cleanup correctly removes typing assertions that could not fail in the current environment.
A bounded typing risk remains: Pyright reports missing PyArrow stubs, making ReaderLike effectively permissive and leaving invalid reader inputs without regression-sensitive static coverage. Mapping and Sequence are also broader than the concrete dict and list runtime branches. Resolvable Arrow stubs and tighter container shapes would make that contract enforceable.
lance-gatekeeper
Bot
added
the
K-approved
lance-gatekeeper
Bot
added
the
K-risk
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
1 participant
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.
Fixes two wrong type annotations for optional-dependency inputs, each of which hid a runtime bug.
asofts_typeswasUnion[datetime, pd.Timestamp, str], defined twice. pandas typespd.Timestamp(...)asTimestamp | NaTType, solance.dataset(uri, asof=pd.Timestamp(...))failed strict type checking. It is nowUnion[datetime, NaTType, str], defined once inlance.util.pd.NaTsubclassesdatetime, sosanitize_tspassed it through and it compared false against every version. It is now rejected withValueError._check_for_pandas(ts), which is always false for astr. Soasof="2026-01-01"asked you to install pandas even when it was installed. It is now guarded by_PANDAS_AVAILABLE.ReaderLikepd.Timestampwhere a DataFrame belongs and referencedpa.dataset.*, which type checkers can't resolve. It now lists every input_coerce_readeraccepts, with the Arrow classes imported directly._check_for_polars+isinstanceinstead of matching__module__.write_datasetdocstring now lists the accepted inputs, andtest_input_datagains dict and row-dict cases.test_optional_types_typing.pypins the accepted and rejectedasoftypes under Pyright.ReaderLikecan't be checked in CI until pyarrow stubs are added: without them the union accepts anything.