New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat: generator-based chunky Table data iteration #5595

Merged

jmao-denver merged 23 commits into deephaven:main from jmao-denver:5186-data-iteration-python

Jul 10, 2024

Contributor

jmao-denver commented Jun 10, 2024 •

edited

Loading

Fixes #5186

jmao-denver self-assigned this

jmao-denver added DocumentationNeeded python-server-side ReleaseNotesNeeded labels

jmao-denver added this to the 3. May 2024 milestone

jmao-denver changed the title ~~Add a table data reader generator~~ New table data reader generator

jmao-denver marked this pull request as ready for review

June 11, 2024 21:42

jmao-denver requested review from chipkent and rcaudy as code owners

June 11, 2024 21:42

jmao-denver requested a review from devinrsmith

June 11, 2024 21:42

Contributor Author

jmao-denver commented Jun 11, 2024 •

edited

Loading

I have looked at the new ColumnVectors and discussed with @rcaudy about utilizing it to achieve the same goal. I am still not very clear about the advantages/disadvantages between the two approach. @rcaudy could you weigh on this with your thoughts?

Another approach @rcaudy mentioned is something similar to ConstructSnaphot, again I am not sure if it has definitive advantages in performance/consistency.

Member

rcaudy commented Jun 12, 2024

I have looked at the new ColumnVectors and discussed with @rcaudy about utilizing it to achieve the same goal. I am still not very clear about the advantages/disadvantages between the two approach. @rcaudy could you weigh on this with your thoughts?

Another approach @rcaudy mentioned is something similar to ConstructSnaphot, again I am not sure if it has definitive advantages in performance/consistency.

The issue with using ColumnVectors is that the iteration pattern is cell-oriented, even if the data retrieval is chunk-oriented. There's no pattern implemented that would let you hand data over to Python in an efficient pattern. I think the Python generator pattern wrapping a getChunk or fillChunk pattern in Java is probably your best bet.

chipkent reviewed

View reviewed changes

py/server/deephaven/table_reader.py Outdated Show resolved Hide resolved

py/server/deephaven/table_reader.py Outdated Show resolved Hide resolved

py/server/deephaven/table_reader.py Outdated Show resolved Hide resolved

py/server/deephaven/table_reader.py Outdated Show resolved Hide resolved

py/server/deephaven/table_reader.py Outdated Show resolved Hide resolved

py/server/deephaven/table_reader.py Outdated Show resolved Hide resolved

jmao-denver requested a review from chipkent

June 14, 2024 02:12

chipkent reviewed

View reviewed changes

py/server/deephaven/table_reader.py Outdated Show resolved Hide resolved

chipkent reviewed

View reviewed changes

py/server/deephaven/table_reader.py Outdated Show resolved Hide resolved

py/server/deephaven/table_reader.py Outdated Show resolved Hide resolved

py/server/deephaven/table_reader.py Outdated Show resolved Hide resolved

py/server/deephaven/table_reader.py Outdated Show resolved Hide resolved

rcaudy reviewed

View reviewed changes

py/server/deephaven/table_reader.py Outdated Show resolved Hide resolved

py/server/deephaven/table_reader.py Outdated Show resolved Hide resolved

jmao-denver force-pushed the 5186-data-iteration-python branch from 8c6abc9 to 23dbcd7 Compare

June 20, 2024 15:29

jmao-denver changed the title ~~New table data reader generator~~ New table data generator

jmao-denver changed the title ~~New table data generator~~ New table data generator-based iteration

jmao-denver requested review from chipkent and rcaudy

June 21, 2024 19:30

jmao-denver changed the title ~~New table data generator-based iteration~~ Generator-based Table data iteration

jmao-denver changed the title ~~Generator-based Table data iteration~~ Generator-based chunky Table data iteration

devinrsmith reviewed

View reviewed changes

py/server/deephaven/_table_reader.py Outdated Show resolved Hide resolved

py/server/deephaven/table.py Outdated Show resolved Hide resolved

py/server/deephaven/table.py Outdated Show resolved Hide resolved

jmao-denver changed the title ~~Generator-based chunky Table data iteration~~ feat: generator-based chunky Table data iteration

devinrsmith reviewed

View reviewed changes

py/server/deephaven/_table_reader.py Outdated Show resolved Hide resolved

py/server/deephaven/table.py Outdated Show resolved Hide resolved

py/server/deephaven/table.py Outdated Show resolved Hide resolved

py/server/deephaven/table.py Outdated Show resolved Hide resolved

jmao-denver requested a review from devinrsmith

July 1, 2024 16:33

devinrsmith reviewed

View reviewed changes

py/server/deephaven/_table_reader.py Outdated Show resolved Hide resolved

py/server/deephaven/table.py Outdated Show resolved Hide resolved

py/server/deephaven/table.py Outdated Show resolved Hide resolved

chipkent reviewed

View reviewed changes

py/server/deephaven/_table_reader.py Show resolved Hide resolved

py/server/deephaven/_table_reader.py Outdated Show resolved Hide resolved

py/server/deephaven/table_listener.py Outdated Show resolved Hide resolved

py/server/deephaven/table_listener.py Outdated Show resolved Hide resolved

py/server/deephaven/table_listener.py Outdated Show resolved Hide resolved

py/server/deephaven/table.py Outdated Show resolved Hide resolved

py/server/deephaven/table.py Outdated Show resolved Hide resolved

py/server/deephaven/table.py Outdated Show resolved Hide resolved

py/server/deephaven/table.py Outdated Show resolved Hide resolved

py/server/deephaven/table.py Outdated Show resolved Hide resolved

jmao-denver added 11 commits

July 8, 2024 15:14


          minor readability improvement

f475a40


          Add iterators returning tuples

ffe7e7f


          Add test cases for tuple return

c6f5fa7


          Correct typing and an impl error

1a05f4a


          Add chunk_size check, code clean up, test cases

c4f96d4


          Add a read-all variant

d426e76


          Respond to review comments

6771e65


          Tide up the doc strings

1bcaabd


          Force CI rerun

3a0b4f4


          Minor fix of doc strings/test cases

f4f824c


          Refactor to use 'strategy' pattern

b191ce2

jmao-denver force-pushed the 5186-data-iteration-python branch from 2c31523 to b191ce2 Compare

July 8, 2024 21:14

jmao-denver added 3 commits

July 8, 2024 19:30


          Reduce boundary-crossing in row-based iteration

43d7586


          Minor code improvement

480afb3


          Utilize memoryview and map() to speed up

0b99ace

jmao-denver requested review from chipkent and rcaudy

July 9, 2024 16:40


          Fix min version support check failure

chipkent reviewed

View reviewed changes

py/server/deephaven/_table_reader.py Outdated Show resolved Hide resolved

py/server/deephaven/_table_reader.py Outdated Show resolved Hide resolved

py/server/deephaven/_table_reader.py Outdated Show resolved Hide resolved

py/server/deephaven/_table_reader.py Show resolved Hide resolved

py/server/deephaven/_table_reader.py Outdated Show resolved Hide resolved

py/server/deephaven/_table_reader.py Outdated Show resolved Hide resolved

py/server/deephaven/_table_reader.py Outdated Show resolved Hide resolved

jmao-denver and others added 4 commits

July 9, 2024 14:29


          Update py/server/deephaven/_table_reader.py

8a50f17

Co-authored-by: Chip Kent <5250374+chipkent@users.noreply.github.com>


          Update py/server/deephaven/_table_reader.py

0aaa9b8

Co-authored-by: Chip Kent <5250374+chipkent@users.noreply.github.com>


          More refactoring and doc string fixes

dc57eda


          Fix CI failure

b0140eb

chipkent approved these changes

View reviewed changes

jmao-denver dismissed devinrsmith’s stale review

July 10, 2024 16:36

Already implemented what's requested.

jmao-denver merged commit b1feeeb into deephaven:main

16 checks passed

jmao-denver deleted the 5186-data-iteration-python branch

July 10, 2024 16:36

github-actions bot locked and limited conversation to collaborators

Contributor

deephaven-internal commented Jul 10, 2024

Labels indicate documentation is required. Issues for documentation have been opened:

Community: deephaven/deephaven-docs-community#260

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

DocumentationNeeded python-server-side ReleaseNotesNeeded