Skip to content
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.

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

Add the optional dependencies parameters in the transform/partitioned_transform methods #4891

Merged

Conversation

jmao-denver
Copy link
Contributor

@jmao-denver jmao-denver commented Nov 29, 2023

Fixes #4890

@jmao-denver jmao-denver force-pushed the 4890-add-dependencies-transform branch from 035d92d to f4ad2b7 Compare November 29, 2023 01:44
py/server/deephaven/table.py Outdated Show resolved Hide resolved
py/server/tests/test_partitioned_table.py Outdated Show resolved Hide resolved
rcaudy
rcaudy previously approved these changes Nov 29, 2023
py/server/deephaven/table.py Outdated Show resolved Hide resolved
Comment on lines 2688 to 2689
j_dependencies = [d.j_table for d in dependencies if isinstance(d, Table)]
j_dependencies.extend([d.table.j_table for d in dependencies if isinstance(d, PartitionedTable)])
Copy link
Member

Choose a reason for hiding this comment

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

There should be a size assert because this will happily run if non Table / PartitionedTable garbage is included.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It might be an overkill, I think it is safe to assume that users who need to use this parameter knows what they are doing.

Copy link
Member

Choose a reason for hiding this comment

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

Will Python's type system (hah) allow that?

py/server/deephaven/table.py Outdated Show resolved Hide resolved
py/server/deephaven/table.py Outdated Show resolved Hide resolved
py/server/tests/test_partitioned_table.py Outdated Show resolved Hide resolved
@jmao-denver jmao-denver merged commit 1745a66 into deephaven:main Nov 29, 2023
12 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Nov 29, 2023
@deephaven-internal
Copy link
Contributor

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

How-to: https://github.com/deephaven/deephaven.io/issues/3496
Reference: https://github.com/deephaven/deephaven.io/issues/3495

@jmao-denver jmao-denver deleted the 4890-add-dependencies-transform branch December 18, 2023 17:26
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add the new 'dependencies' parameter to the Python transform/partitioned_transform wrapper methods
4 participants