Skip to content

Commit 426fbdd

Browse files
kewang1024James Sun
authored andcommitted
Fix the logic in LocalExchange for acquiring the bucket count
Passing empty node list would make `getBucketNodeMap` stuck. The correct way to acquire the bucket count is to use `getBucketCount`.
1 parent 48fadf1 commit 426fbdd

File tree

2 files changed

+85
-8
lines changed

2 files changed

+85
-8
lines changed

presto-main/src/main/java/com/facebook/presto/operator/exchange/LocalExchange.java

Lines changed: 3 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,6 @@
2323
import com.facebook.presto.operator.PipelineExecutionStrategy;
2424
import com.facebook.presto.operator.PrecomputedHashGenerator;
2525
import com.facebook.presto.spi.BucketFunction;
26-
import com.facebook.presto.spi.connector.ConnectorBucketNodeMap;
2726
import com.facebook.presto.spi.connector.ConnectorNodePartitioningProvider;
2827
import com.facebook.presto.sql.planner.PartitioningHandle;
2928
import com.facebook.presto.sql.planner.PartitioningProviderManager;
@@ -150,7 +149,7 @@ else if (partitioning.equals(FIXED_HASH_DISTRIBUTION) || partitioning.getConnect
150149
}
151150
}
152151

153-
private static PartitionFunction createPartitionFunction(
152+
static PartitionFunction createPartitionFunction(
154153
PartitioningProviderManager partitioningProviderManager,
155154
Session session,
156155
PartitioningHandle partitioning,
@@ -170,14 +169,10 @@ private static PartitionFunction createPartitionFunction(
170169
}
171170

172171
ConnectorNodePartitioningProvider partitioningProvider = partitioningProviderManager.getPartitioningProvider(partitioning.getConnectorId().get());
173-
ConnectorBucketNodeMap connectorBucketNodeMap = partitioningProvider.getBucketNodeMap(
172+
int bucketCount = partitioningProvider.getBucketCount(
174173
partitioning.getTransactionHandle().orElse(null),
175174
session.toConnectorSession(partitioning.getConnectorId().get()),
176-
partitioning.getConnectorHandle(),
177-
ImmutableList.of());
178-
checkArgument(connectorBucketNodeMap != null, "No partition map %s", partitioning);
179-
180-
int bucketCount = connectorBucketNodeMap.getBucketCount();
175+
partitioning.getConnectorHandle());
181176
int[] bucketToPartition = new int[bucketCount];
182177
for (int bucket = 0; bucket < bucketCount; bucket++) {
183178
bucketToPartition[bucket] = bucket % partitionCount;

presto-main/src/test/java/com/facebook/presto/operator/exchange/TestLocalExchange.java

Lines changed: 82 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -20,10 +20,21 @@
2020
import com.facebook.presto.execution.Lifespan;
2121
import com.facebook.presto.operator.InterpretedHashGenerator;
2222
import com.facebook.presto.operator.PageAssertions;
23+
import com.facebook.presto.operator.PartitionFunction;
2324
import com.facebook.presto.operator.PipelineExecutionStrategy;
2425
import com.facebook.presto.operator.exchange.LocalExchange.LocalExchangeFactory;
2526
import com.facebook.presto.operator.exchange.LocalExchange.LocalExchangeSinkFactory;
2627
import com.facebook.presto.operator.exchange.LocalExchange.LocalExchangeSinkFactoryId;
28+
import com.facebook.presto.spi.BucketFunction;
29+
import com.facebook.presto.spi.ConnectorId;
30+
import com.facebook.presto.spi.ConnectorSession;
31+
import com.facebook.presto.spi.ConnectorSplit;
32+
import com.facebook.presto.spi.Node;
33+
import com.facebook.presto.spi.connector.ConnectorBucketNodeMap;
34+
import com.facebook.presto.spi.connector.ConnectorNodePartitioningProvider;
35+
import com.facebook.presto.spi.connector.ConnectorPartitioningHandle;
36+
import com.facebook.presto.spi.connector.ConnectorTransactionHandle;
37+
import com.facebook.presto.sql.planner.PartitioningHandle;
2738
import com.facebook.presto.sql.planner.PartitioningProviderManager;
2839
import com.google.common.collect.ImmutableList;
2940
import com.google.common.util.concurrent.ListenableFuture;
@@ -36,17 +47,23 @@
3647
import java.util.List;
3748
import java.util.Optional;
3849
import java.util.function.Consumer;
50+
import java.util.function.ToIntFunction;
51+
import java.util.stream.Stream;
3952

4053
import static com.facebook.airlift.testing.Assertions.assertContains;
4154
import static com.facebook.presto.common.type.BigintType.BIGINT;
4255
import static com.facebook.presto.operator.PipelineExecutionStrategy.GROUPED_EXECUTION;
4356
import static com.facebook.presto.operator.PipelineExecutionStrategy.UNGROUPED_EXECUTION;
57+
import static com.facebook.presto.operator.exchange.LocalExchange.createPartitionFunction;
58+
import static com.facebook.presto.spi.connector.ConnectorBucketNodeMap.createBucketNodeMap;
59+
import static com.facebook.presto.spi.schedule.NodeSelectionStrategy.SOFT_AFFINITY;
4460
import static com.facebook.presto.sql.planner.SystemPartitioningHandle.FIXED_ARBITRARY_DISTRIBUTION;
4561
import static com.facebook.presto.sql.planner.SystemPartitioningHandle.FIXED_BROADCAST_DISTRIBUTION;
4662
import static com.facebook.presto.sql.planner.SystemPartitioningHandle.FIXED_HASH_DISTRIBUTION;
4763
import static com.facebook.presto.sql.planner.SystemPartitioningHandle.FIXED_PASSTHROUGH_DISTRIBUTION;
4864
import static com.facebook.presto.sql.planner.SystemPartitioningHandle.SINGLE_DISTRIBUTION;
4965
import static com.facebook.presto.testing.TestingSession.testSessionBuilder;
66+
import static com.google.common.collect.ImmutableList.toImmutableList;
5067
import static io.airlift.units.DataSize.Unit.BYTE;
5168
import static org.testng.Assert.assertEquals;
5269
import static org.testng.Assert.assertFalse;
@@ -434,6 +451,71 @@ public void testPartition(PipelineExecutionStrategy executionStrategy)
434451
});
435452
}
436453

454+
@Test
455+
public void testCreatePartitionFunction()
456+
{
457+
int partitionCount = 10;
458+
PartitioningProviderManager partitioningProviderManager = new PartitioningProviderManager();
459+
partitioningProviderManager.addPartitioningProvider(
460+
new ConnectorId("prism"),
461+
new ConnectorNodePartitioningProvider() {
462+
@Override
463+
public ConnectorBucketNodeMap getBucketNodeMap(ConnectorTransactionHandle transactionHandle, ConnectorSession session, ConnectorPartitioningHandle partitioningHandle, List<Node> sortedNodes)
464+
{
465+
return createBucketNodeMap(Stream.generate(() -> sortedNodes).flatMap(List::stream).limit(10).collect(toImmutableList()), SOFT_AFFINITY);
466+
}
467+
468+
@Override
469+
public ToIntFunction<ConnectorSplit> getSplitBucketFunction(ConnectorTransactionHandle transactionHandle, ConnectorSession session, ConnectorPartitioningHandle partitioningHandle)
470+
{
471+
return null;
472+
}
473+
474+
@Override
475+
public BucketFunction getBucketFunction(ConnectorTransactionHandle transactionHandle, ConnectorSession session, ConnectorPartitioningHandle partitioningHandle, List<Type> partitionChannelTypes, int bucketCount)
476+
{
477+
return (Page page, int position) -> partitionCount;
478+
}
479+
480+
@Override
481+
public int getBucketCount(ConnectorTransactionHandle transactionHandle, ConnectorSession session, ConnectorPartitioningHandle partitioningHandle)
482+
{
483+
return 10;
484+
}
485+
});
486+
PartitioningHandle partitioningHandle = new PartitioningHandle(
487+
Optional.of(new ConnectorId("prism")),
488+
Optional.of(new ConnectorTransactionHandle() {
489+
@Override
490+
public int hashCode()
491+
{
492+
return super.hashCode();
493+
}
494+
495+
@Override
496+
public boolean equals(Object obj)
497+
{
498+
return super.equals(obj);
499+
}
500+
}),
501+
new ConnectorPartitioningHandle() {
502+
@Override
503+
public boolean isSingleNode()
504+
{
505+
return false;
506+
}
507+
508+
@Override
509+
public boolean isCoordinatorOnly()
510+
{
511+
return false;
512+
}
513+
});
514+
PartitionFunction partitionFunction = createPartitionFunction(partitioningProviderManager, session, partitioningHandle, 600, ImmutableList.of(), false);
515+
516+
assertEquals(partitionFunction.getPartitionCount(), partitionCount);
517+
}
518+
437519
@Test(dataProvider = "executionStrategy")
438520
public void writeUnblockWhenAllReadersFinish(PipelineExecutionStrategy executionStrategy)
439521
{

0 commit comments

Comments
 (0)