Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 11 additions & 5 deletions tests/integration/test_dbapi_integration.py
Original file line number Diff line number Diff line change
Expand Up @@ -1876,12 +1876,18 @@ def test_segments_cursor(trino_connection):
start => 1,
stop => 5,
step => 1)) n""")
rows = cur.fetchall()
assert len(rows) > 0
for spooled_data, spooled_segment in rows:
segments = cur.fetchall()
assert len(segments) > 0
row_mapper = trino.mapper.RowMapperFactory().create(columns=cur._query.columns, legacy_primitive_types=False)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

change import to from trino.mapper import RowMapperFactory

total = 0
for spooled_data in segments:
assert len(spooled_data.segments) == 1, "Expected SpooledData to contain a single segment"
segment = spooled_data.segments[0]
assert spooled_data.encoding == trino_connection._client_session.encoding
assert isinstance(spooled_segment.uri, str), f"Expected string for uri, got {spooled_segment.uri}"
assert isinstance(spooled_segment.ack_uri, str), f"Expected string for ack_uri, got {spooled_segment.ack_uri}"
assert isinstance(segment.uri, str), f"Expected string for uri, got {segment.uri}"
assert isinstance(segment.ack_uri, str), f"Expected string for ack_uri, got {segment.ack_uri}"
total += len(list(trino.client.SegmentIterator(spooled_data, row_mapper)))
assert total == 300875, f"Expected total rows 300875, got {total}"


def get_cursor(legacy_prepared_statements, run_trino):
Expand Down
3 changes: 2 additions & 1 deletion trino/client.py
Original file line number Diff line number Diff line change
Expand Up @@ -1187,7 +1187,8 @@ def __iter__(self) -> Iterator[Tuple["SpooledData", "Segment"]]:
return self

def __next__(self) -> Tuple["SpooledData", "Segment"]:
return self, next(self._segments_iterator)
segment = next(self._segments_iterator)
return SpooledData(self._encoding, [segment]), segment
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not also change SpooledData to contain a single segment, rather than a list?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because SpooledData can contain multiple segments. I think we should not reuse this object as the iterable object but return another class that has all the required fields. Let's get rid of Tuple["SpooledData", "Segment"].

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, each response can contain multiple segment URLs, so we need the transfer object to reflect that, but the abstraction we expose to the client doesn't need to mirror that.


def __repr__(self):
return (f"SpooledData(encoding={self._encoding}, segments={list(self._segments)})")
Expand Down
2 changes: 1 addition & 1 deletion trino/dbapi.py
Original file line number Diff line number Diff line change
Expand Up @@ -742,7 +742,7 @@ def execute(self, operation, params=None):
self._query = trino.client.TrinoQuery(self._request, query=operation,
legacy_primitive_types=self._legacy_primitive_types,
fetch_mode="segments")
self._iterator = iter(self._query.execute())
self._iterator = map(lambda tuple: tuple[0], iter(self._query.execute()))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note this is a breaking change, but i agree it is a nicer API.

However, how can the client retrieve the encoding?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See the usage in the integration test

return self


Expand Down