docs: Add comments to clarify Distribution #147
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
meta-cla
bot
added
the
CLA Signed
Contributor
mbasmanova
changed the title
mbasmanova
changed the title
Contributor
|
CC: @feilong-liu @MBkkt |
mbasmanova
approved these changes
Aug 9, 2025
Contributor
mbasmanova
left a comment
mbasmanova
left a comment
There was a problem hiding this comment.
@oerling Thank you for adding these comments. Super helpful. Please, fix typos before landing.
| /// data at rest and workers for data in flight based on values of | ||
| /// special columns. We use the word partitioning to mean that where | ||
| /// a row of data at rest or in flight is found depends on a | ||
| /// function of one or more columns of the row. Hash partitioning nd |
Contributor
There was a problem hiding this comment.
typo: partitioning nd -> partitioning and
| /// rows of a partition are the rows of a dataset for which the | ||
| /// partitioning function satisfy some criteria, e.g. has % number | ||
| /// of partitions == id of partition. The corresponding concept in | ||
| /// Hive, e.g. Presto or Spark is bucketing. the word partitioning |
Contributor
There was a problem hiding this comment.
typo: the word -> The word
|
|
||
| /// Distribution of data. 'numPartitions' is 1 if the data is not partitioned. | ||
| /// There is copartitioning if the DistributionType is the same on both sides | ||
| /// There is copartitioning if the DistributionType iscompatible on both sides |
Contributor
There was a problem hiding this comment.
typo: iscompatible -> is compatible
Should there be a function that takes 2 DistributionType's and returns true if they are compatible?
| ShuffleMode mode{ShuffleMode::kNone}; | ||
| int32_t numPartitions{1}; | ||
| LocusCP locus{nullptr}; | ||
| bool isGather{false}; |
Contributor
There was a problem hiding this comment.
Would you document locus and isGather properties? Why do we need 'locus'?
| /// True if 'other' has the same ordering columns and order type. | ||
| bool isSameOrder(const Distribution& other) const; | ||
|
|
||
| /// makes a new Distribution where 'exprs' are replaced with the |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.