gortiz commented on code in PR #14893:
URL: https://github.com/apache/pinot/pull/14893#discussion_r1926692030
##########
pinot-query-planner/src/main/java/org/apache/pinot/calcite/rel/logical/PinotLogicalExchange.java:
##########
@@ -35,38 +36,43 @@
*/
public class PinotLogicalExchange extends Exchange {
private final PinotRelExchangeType _exchangeType;
+ // NOTE: We might change distribution type for local exchange. Store the
keys separately.
+ private final List<Integer> _keys;
private PinotLogicalExchange(RelOptCluster cluster, RelTraitSet traitSet,
RelNode input, RelDistribution distribution,
- PinotRelExchangeType exchangeType) {
+ PinotRelExchangeType exchangeType, List<Integer> keys) {
Review Comment:
In which cases the keys will be different than `distribution.getKeys`? What
is in fact the meaning of having keys = {X, Y, Z} and a distribution like
random that doesn't support keys? Wouldn't be better to change the
`distribution` value depending on the keys? If we want to use a distribution +
keys that is not permitted by Calcite we can create our own implementation of
RelDistribution
##########
pinot-query-planner/src/main/java/org/apache/pinot/query/planner/physical/MailboxAssignmentVisitor.java:
##########
@@ -42,7 +40,7 @@ public class MailboxAssignmentVisitor extends
DefaultPostOrderTraversalVisitor<V
public Void process(PlanNode node, DispatchablePlanContext context) {
if (node instanceof MailboxSendNode) {
MailboxSendNode sendNode = (MailboxSendNode) node;
- int senderStageId = sendNode.getStageId();
+ Integer senderStageId = sendNode.getStageId();
Review Comment:
Why `Integer`? BaseNode.getStageId() is always a `int`, right?
##########
pinot-query-planner/src/main/java/org/apache/pinot/calcite/rel/rules/PinotJoinExchangeNodeInsertRule.java:
##########
@@ -102,7 +102,9 @@ private static PinotLogicalExchange
createExchangeForLookupJoin(PinotHintOptions
List<Integer> keys, RelNode child) {
switch (distributionType) {
case LOCAL:
- return PinotLogicalExchange.create(child, RelDistributions.SINGLETON);
+ // NOTE: We use SINGLETON to represent local distribution. Add keys to
the exchange because we might want to
+ // switch it to HASH distribution to increase parallelism. See
MailboxAssignmentVisitor for details.
Review Comment:
BTW, I don't get why we set DistributionType as SINGLETON in cases where we
want to use HASH.
##########
pinot-query-planner/src/main/java/org/apache/pinot/calcite/rel/rules/PinotJoinExchangeNodeInsertRule.java:
##########
@@ -102,7 +102,9 @@ private static PinotLogicalExchange
createExchangeForLookupJoin(PinotHintOptions
List<Integer> keys, RelNode child) {
switch (distributionType) {
case LOCAL:
- return PinotLogicalExchange.create(child, RelDistributions.SINGLETON);
+ // NOTE: We use SINGLETON to represent local distribution. Add keys to
the exchange because we might want to
+ // switch it to HASH distribution to increase parallelism. See
MailboxAssignmentVisitor for details.
Review Comment:
Reading RelDistribution.Type, shouldn't this be broadcast? The definition of
broadcast is:
> There are multiple instances of the stream, and all records appear in each
instance
While the definition of singleton is:
> There is only one instance of the stream. It sees all records.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
