Skip to content

Conversation

@gsakkis
Copy link
Contributor

@gsakkis gsakkis commented Sep 26, 2022

DenseTensorSchema.key_range currently returns an IntRange of the key dimension values for dense arrays. For improved efficiency and memory predictability however, it is better if every partition of this key range (generated by partition_by_count and/or partition_by_weight) starts at a tile boundary so that any integral TileDB tile is read (at most) once.

This PR updates DenseTensorSchema.key_range to return a ConstrainedPartitionsIntRange whose start_offsets are a range with step equal to the tile size of the key dimension, ensuring that every partition (with the possible exception of the first) starts at a tile boundary.

While this change is in general beneficial, it is even more important for extending properly #191 to dense arrays (since it is more likely that the default IntRange partitioning doesn't align with tile boundaries for an arbitrary user-selected key dimension slice).

@shortcut-integration
Copy link

This pull request has been linked to Shortcut Story #21626: Support for slicing on key dimension.

@property
def key_range(self) -> IntRange:
def key_range(self) -> ConstrainedPartitionsIntRange:
self._key_range: ConstrainedPartitionsIntRange
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor and old...self._key_range is not defined in __init__.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a cached property, it is defined (and cached) the first time the property is accessed instead of __init__. In Python 3.8+ this is provided by functools.cached_property, since we support 3.7 we do it manually instead.

Copy link
Contributor

@georgeSkoumas georgeSkoumas left a comment

Choose a reason for hiding this comment

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

LGTM!

@gsakkis gsakkis changed the base branch from gsa/InclusiveRange-updates to master October 3, 2022 12:56
@gsakkis gsakkis force-pushed the gsa/sc-21626/support-for-slicing-on-key-dimension-dense branch from 47fba99 to 56a12c7 Compare October 3, 2022 12:56
@gsakkis gsakkis merged commit b9baa54 into master Oct 3, 2022
@gsakkis gsakkis deleted the gsa/sc-21626/support-for-slicing-on-key-dimension-dense branch October 3, 2022 12:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants