mirror of
https://github.com/lancedb/lancedb.git
synced 2026-10-25 13:12:39 +00:00
fix(python): accept with_row_id in take_offsets/take_row_ids on every Table implementation (#4374)
`Table.take_offsets` and `Table.take_row_ids` declare a keyword-only `with_row_id: bool = False`, but none of the three implementations accept it — `LanceTable`, `AsyncTable` and `RemoteTable` all raise `TypeError: ... got an unexpected keyword argument 'with_row_id'` on the documented call. The builders they return already support it (`LanceTakeQueryBuilder.with_row_id()` / `AsyncTakeQuery.with_row_id()`), so this is a signature that never reached the implementations. The base docstrings also don't mention the parameter and still say `take_offsets` returns a `pa.RecordBatch` (it returns a `LanceTakeQueryBuilder`), so those are corrected too. Each implementation now accepts the kwarg and applies `.with_row_id()` to the builder it returns. Tests: sync and async assert `_rowid` appears in the result columns; the remote test uses the suite's request-capturing fixture and asserts `with_row_id: true` reaches the wire (and is absent by default). All three fail on `main` with the `TypeError`. ruff format/check clean. A related mismatch I left alone: the deprecated `LanceTable.create_scalar_index`/`create_fts_index` also drop a `wait_timeout` the base declares — deprecated since 0.25, so not worth touching here. Co-authored-by: Xuanwo <github@xuanwo.io>
This commit is contained in:
co-authored by
Xuanwo
parent
3321860e9f
commit
9da4658d57
@@ -1154,11 +1154,21 @@ class RemoteTable(Table):
|
||||
"""
|
||||
return LOOP.run(self._table.uri())
|
||||
|
||||
def take_offsets(self, offsets: list[int]) -> LanceTakeQueryBuilder:
|
||||
return LanceTakeQueryBuilder(self._table.take_offsets(offsets))
|
||||
def take_offsets(
|
||||
self, offsets: list[int], *, with_row_id: bool = False
|
||||
) -> LanceTakeQueryBuilder:
|
||||
builder = LanceTakeQueryBuilder(self._table.take_offsets(offsets))
|
||||
if with_row_id:
|
||||
builder.with_row_id()
|
||||
return builder
|
||||
|
||||
def take_row_ids(self, row_ids: list[int]) -> LanceTakeQueryBuilder:
|
||||
return LanceTakeQueryBuilder(self._table.take_row_ids(row_ids))
|
||||
def take_row_ids(
|
||||
self, row_ids: list[int], *, with_row_id: bool = False
|
||||
) -> LanceTakeQueryBuilder:
|
||||
builder = LanceTakeQueryBuilder(self._table.take_row_ids(row_ids))
|
||||
if with_row_id:
|
||||
builder.with_row_id()
|
||||
return builder
|
||||
|
||||
def uses_v2_manifest_paths(self) -> bool:
|
||||
return LOOP.run(self._table.uses_v2_manifest_paths())
|
||||
|
||||
@@ -2103,11 +2103,14 @@ class Table(ABC):
|
||||
----------
|
||||
offsets: list[int]
|
||||
The offsets to take.
|
||||
with_row_id: bool, default False
|
||||
If True, include the "_rowid" column in the results.
|
||||
|
||||
Returns
|
||||
-------
|
||||
pa.RecordBatch
|
||||
A record batch containing the rows at the given offsets.
|
||||
LanceTakeQueryBuilder
|
||||
A query builder that can be executed to get the rows at the given
|
||||
offsets.
|
||||
"""
|
||||
|
||||
def __getitems__(self, offsets: list[int]) -> pa.RecordBatch:
|
||||
@@ -2188,11 +2191,13 @@ class Table(ABC):
|
||||
----------
|
||||
row_ids: list[int]
|
||||
The row ids to take.
|
||||
with_row_id: bool, default False
|
||||
If True, include the "_rowid" column in the results.
|
||||
|
||||
Returns
|
||||
-------
|
||||
AsyncTakeQuery
|
||||
A query object that can be executed to get the rows.
|
||||
LanceTakeQueryBuilder
|
||||
A query builder that can be executed to get the rows.
|
||||
"""
|
||||
|
||||
@abstractmethod
|
||||
@@ -3182,11 +3187,21 @@ class LanceTable(Table):
|
||||
"""Get the current version of the table"""
|
||||
return LOOP.run(self._table.version())
|
||||
|
||||
def take_offsets(self, offsets: list[int]) -> LanceTakeQueryBuilder:
|
||||
return LanceTakeQueryBuilder(self._table.take_offsets(offsets))
|
||||
def take_offsets(
|
||||
self, offsets: list[int], *, with_row_id: bool = False
|
||||
) -> LanceTakeQueryBuilder:
|
||||
builder = LanceTakeQueryBuilder(self._table.take_offsets(offsets))
|
||||
if with_row_id:
|
||||
builder.with_row_id()
|
||||
return builder
|
||||
|
||||
def take_row_ids(self, row_ids: list[int]) -> LanceTakeQueryBuilder:
|
||||
return LanceTakeQueryBuilder(self._table.take_row_ids(row_ids))
|
||||
def take_row_ids(
|
||||
self, row_ids: list[int], *, with_row_id: bool = False
|
||||
) -> LanceTakeQueryBuilder:
|
||||
builder = LanceTakeQueryBuilder(self._table.take_row_ids(row_ids))
|
||||
if with_row_id:
|
||||
builder.with_row_id()
|
||||
return builder
|
||||
|
||||
def blob_columns(self) -> list[str]:
|
||||
return LOOP.run(self._table.blob_columns())
|
||||
@@ -7325,7 +7340,9 @@ class AsyncTable:
|
||||
"""
|
||||
await self._inner.restore(version)
|
||||
|
||||
def take_offsets(self, offsets: list[int]) -> AsyncTakeQuery:
|
||||
def take_offsets(
|
||||
self, offsets: list[int], *, with_row_id: bool = False
|
||||
) -> AsyncTakeQuery:
|
||||
"""
|
||||
Take a list of offsets from the table.
|
||||
|
||||
@@ -7343,15 +7360,23 @@ class AsyncTable:
|
||||
----------
|
||||
offsets: list[int]
|
||||
The offsets to take.
|
||||
with_row_id: bool, default False
|
||||
If True, include the "_rowid" column in the results.
|
||||
|
||||
Returns
|
||||
-------
|
||||
pa.RecordBatch
|
||||
A record batch containing the rows at the given offsets.
|
||||
AsyncTakeQuery
|
||||
A query object that can be executed to get the rows at the given
|
||||
offsets.
|
||||
"""
|
||||
return AsyncTakeQuery(self._inner.take_offsets(offsets), self)
|
||||
query = AsyncTakeQuery(self._inner.take_offsets(offsets), self)
|
||||
if with_row_id:
|
||||
query.with_row_id()
|
||||
return query
|
||||
|
||||
def take_row_ids(self, row_ids: list[int]) -> AsyncTakeQuery:
|
||||
def take_row_ids(
|
||||
self, row_ids: list[int], *, with_row_id: bool = False
|
||||
) -> AsyncTakeQuery:
|
||||
"""
|
||||
Take a list of row ids from the table.
|
||||
|
||||
@@ -7372,13 +7397,18 @@ class AsyncTable:
|
||||
----------
|
||||
row_ids: list[int]
|
||||
The row ids to take.
|
||||
with_row_id: bool, default False
|
||||
If True, include the "_rowid" column in the results.
|
||||
|
||||
Returns
|
||||
-------
|
||||
AsyncTakeQuery
|
||||
A query object that can be executed to get the rows.
|
||||
"""
|
||||
return AsyncTakeQuery(self._inner.take_row_ids(row_ids), self)
|
||||
query = AsyncTakeQuery(self._inner.take_row_ids(row_ids), self)
|
||||
if with_row_id:
|
||||
query.with_row_id()
|
||||
return query
|
||||
|
||||
async def blob_columns(self) -> list[str]:
|
||||
return await self._inner.blob_columns()
|
||||
|
||||
@@ -2324,6 +2324,33 @@ def test_take_queries(tmp_path):
|
||||
]
|
||||
|
||||
|
||||
def test_take_queries_with_row_id(tmp_path):
|
||||
db = lancedb.connect(tmp_path)
|
||||
table = db.create_table("test", pa.table({"idx": range(100)}))
|
||||
|
||||
by_row_id = table.take_row_ids([5, 2, 17], with_row_id=True).to_arrow()
|
||||
assert "_rowid" in by_row_id.column_names
|
||||
by_offset = table.take_offsets([5, 2, 17], with_row_id=True).to_arrow()
|
||||
assert "_rowid" in by_offset.column_names
|
||||
|
||||
# The column is opt-in and must stay out of the default projection.
|
||||
assert "_rowid" not in table.take_row_ids([5, 2, 17]).to_arrow().column_names
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_take_queries_with_row_id_async(tmp_path):
|
||||
db = await lancedb.connect_async(tmp_path)
|
||||
table = await db.create_table("test", pa.table({"idx": range(100)}))
|
||||
|
||||
by_row_id = await table.take_row_ids([5, 2, 17], with_row_id=True).to_arrow()
|
||||
assert "_rowid" in by_row_id.column_names
|
||||
by_offset = await table.take_offsets([5, 2, 17], with_row_id=True).to_arrow()
|
||||
assert "_rowid" in by_offset.column_names
|
||||
|
||||
default = await table.take_row_ids([5, 2, 17]).to_arrow()
|
||||
assert "_rowid" not in default.column_names
|
||||
|
||||
|
||||
def test_take_queries_to_batches(tmp_path):
|
||||
# Regression test for the sync take-query path: `to_batches` previously
|
||||
# raised ``AttributeError: 'AsyncTakeQuery' object has no attribute
|
||||
|
||||
@@ -2225,6 +2225,23 @@ def test_head():
|
||||
assert data == pa.table({"id": [1, 2, 3]})
|
||||
|
||||
|
||||
def test_remote_take_row_ids_with_row_id():
|
||||
bodies = []
|
||||
|
||||
def handler(body):
|
||||
bodies.append(body)
|
||||
return pa.table({"id": [1, 2], "_rowid": pa.array([0, 1], pa.uint64())})
|
||||
|
||||
with query_test_table(handler) as table:
|
||||
result = table.take_row_ids([0, 1], with_row_id=True).to_arrow()
|
||||
assert "_rowid" in result.column_names
|
||||
assert bodies[-1]["with_row_id"] is True
|
||||
|
||||
with query_test_table(handler) as table:
|
||||
table.take_row_ids([0, 1]).to_arrow()
|
||||
assert "with_row_id" not in bodies[-1]
|
||||
|
||||
|
||||
def test_query_sync_minimal():
|
||||
def handler(body):
|
||||
assert body == {
|
||||
|
||||
Reference in New Issue
Block a user