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

Wrap the new PartitionedTable interface #2446

Merged
merged 8 commits into from
Jun 1, 2022

Conversation

jmao-denver
Copy link
Contributor

Phase 1 wrapping:

  • convenience method - partition_by (no PartitionedTableFactory)
  • PartitionedTable class (no proxy, transform support)

Fixes #2432
Related to #2434

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 jmao-denver requested a review from rcaudy May 27, 2022 15:14
@jmao-denver jmao-denver requested a review from rcaudy May 27, 2022 20:06
rcaudy
rcaudy previously approved these changes May 27, 2022
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
Comment on lines 1300 to 1302
"""A partitioned table is a table with one column containing like-defined constituent tables,
optionally with key columns defined to allow binary operation based transformation or joins with other like-keyed
partitioned tables."""
Copy link
Member

Choose a reason for hiding this comment

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

I don't think users are going to find this clear.


@property
def table(self) -> Table:
"""The underlying Table."""
Copy link
Member

Choose a reason for hiding this comment

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

Users will probably not find this clear. Is this the source table the PartitionedTable was created from?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, in the partition_by case, it is the table created with agg_by on the source table.

Copy link
Member

Choose a reason for hiding this comment

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

This is the partitioned table. The PartitionedTable is just a wrapper around it.

py/server/deephaven/table.py Show resolved Hide resolved
Comment on lines +1424 to +1425
def constituent_tables(self) -> List[Table]:
"""Returns all the current constituent tables."""
Copy link
Member

Choose a reason for hiding this comment

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

Again, I don't think the nomenclature is clear. e.g. a name like subtables may be more apparent.

Copy link
Member

Choose a reason for hiding this comment

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

I very intentionally didn't call this sub-tables.

py/server/tests/test_partitioned_table.py Show resolved Hide resolved
py/server/tests/test_partitioned_table.py Show resolved Hide resolved
py/server/tests/test_partitioned_table.py Show resolved Hide resolved
rcaudy
rcaudy previously approved these changes Jun 1, 2022
py/server/deephaven/table.py Outdated Show resolved Hide resolved

@property
def unique_keys(self) -> bool:
"""Whether the keys in the underlying table are unique."""
Copy link
Member

Choose a reason for hiding this comment

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

I still don't understand this method. Is this indicating if a key can be repeated in the underlying table? It isn't clear to me how a key could be repeated when methods like constituent_by_keys indicate a 1:1 mapping.

Co-authored-by: Chip Kent <5250374+chipkent@users.noreply.github.com>
@jmao-denver jmao-denver requested a review from rcaudy June 1, 2022 18:07
@jmao-denver jmao-denver merged commit b778e11 into deephaven:main Jun 1, 2022
@github-actions github-actions bot locked and limited conversation to collaborators Jun 1, 2022
@jmao-denver jmao-denver deleted the 2432-wrap-partitioned-table branch February 8, 2023 18:50
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.

Wrap PartitionedTable, update Table.partiton_by
3 participants