From f2e6a83f4829388609da69997a97065748f01819 Mon Sep 17 00:00:00 2001 From: Saksham Sachdev Date: Fri, 10 Jul 2020 17:04:27 -0700 Subject: [PATCH 01/10] Revoke memory after initial output page has been produced in tests Cherry-pick of https://github.com/prestosql/presto/commit/0a9b8047f36fd22fd33193d1a9e3fea4e88efb4b Co-authored-by: Karol Sobczak --- .../java/com/facebook/presto/operator/OperatorAssertion.java | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/presto-main/src/test/java/com/facebook/presto/operator/OperatorAssertion.java b/presto-main/src/test/java/com/facebook/presto/operator/OperatorAssertion.java index b5c08bd7eff09..843df85092a45 100644 --- a/presto-main/src/test/java/com/facebook/presto/operator/OperatorAssertion.java +++ b/presto-main/src/test/java/com/facebook/presto/operator/OperatorAssertion.java @@ -102,13 +102,16 @@ public static List finishOperator(Operator operator) if (handledBlocked(operator)) { continue; } - handleMemoryRevoking(operator); + operator.finish(); Page outputPage = operator.getOutput(); if (outputPage != null && outputPage.getPositionCount() != 0) { outputPages.add(outputPage); loopsSinceLastPage = 0; } + + // revoke memory when output pages have started being produced + handleMemoryRevoking(operator); } assertEquals(operator.isFinished(), true, "Operator did not finish"); From 80890daea7f0528c99bc7e90bc31c0708eaa449d Mon Sep 17 00:00:00 2001 From: Saksham Sachdev Date: Mon, 13 Jul 2020 14:48:50 -0700 Subject: [PATCH 02/10] Allow memory revoke only during operator finish phase Cherry-pick of https://github.com/prestosql/presto/commit/5fef5aa58626286d2952a2008ee8598232cd954c Co-authored-by: Karol Sobczak --- .../presto/operator/OperatorAssertion.java | 39 +++++++++++++++++-- 1 file changed, 36 insertions(+), 3 deletions(-) diff --git a/presto-main/src/test/java/com/facebook/presto/operator/OperatorAssertion.java b/presto-main/src/test/java/com/facebook/presto/operator/OperatorAssertion.java index 843df85092a45..3b9cf58fdba5a 100644 --- a/presto-main/src/test/java/com/facebook/presto/operator/OperatorAssertion.java +++ b/presto-main/src/test/java/com/facebook/presto/operator/OperatorAssertion.java @@ -67,7 +67,20 @@ public static List toPages(Operator operator, Iterator input) .build(); } + public static List toPages(Operator operator, Iterator input, boolean revokeMemoryWhenAddingPages) + { + return ImmutableList.builder() + .addAll(toPagesPartial(operator, input, revokeMemoryWhenAddingPages)) + .addAll(finishOperator(operator)) + .build(); + } + public static List toPagesPartial(Operator operator, Iterator input) + { + return toPagesPartial(operator, input, true); + } + + public static List toPagesPartial(Operator operator, Iterator input, boolean revokeMemory) { // verify initial state assertEquals(operator.isFinished(), false); @@ -77,7 +90,10 @@ public static List toPagesPartial(Operator operator, Iterator input) if (handledBlocked(operator)) { continue; } - handleMemoryRevoking(operator); + + if (revokeMemory) { + handleMemoryRevoking(operator); + } if (input.hasNext() && operator.needsInput()) { operator.addInput(input.next()); @@ -140,9 +156,14 @@ private static void handleMemoryRevoking(Operator operator) } public static List toPages(OperatorFactory operatorFactory, DriverContext driverContext, List input) + { + return toPages(operatorFactory, driverContext, input, true); + } + + public static List toPages(OperatorFactory operatorFactory, DriverContext driverContext, List input, boolean revokeMemoryWhenAddingPages) { try (Operator operator = operatorFactory.createOperator(driverContext)) { - return toPages(operator, input.iterator()); + return toPages(operator, input.iterator(), revokeMemoryWhenAddingPages); } catch (Exception e) { throwIfUnchecked(e); @@ -221,7 +242,19 @@ public static void assertOperatorEqualsIgnoreOrder( boolean hashEnabled, Optional hashChannel) { - List pages = toPages(operatorFactory, driverContext, input); + assertOperatorEqualsIgnoreOrder(operatorFactory, driverContext, input, expected, hashEnabled, hashChannel, true); + } + + public static void assertOperatorEqualsIgnoreOrder( + OperatorFactory operatorFactory, + DriverContext driverContext, + List input, + MaterializedResult expected, + boolean hashEnabled, + Optional hashChannel, + boolean revokeMemoryWhenAddingPages) + { + List pages = toPages(operatorFactory, driverContext, input, revokeMemoryWhenAddingPages); if (hashEnabled && hashChannel.isPresent()) { // Drop the hashChannel for all pages pages = dropChannel(pages, ImmutableList.of(hashChannel.get())); From df4ae85b0dd0118713b7bf768efc26a5f2c58e28 Mon Sep 17 00:00:00 2001 From: Saksham Sachdev Date: Mon, 13 Jul 2020 14:58:06 -0700 Subject: [PATCH 03/10] Produce more than single page in testHashAggregation Cherry-pick of https://github.com/prestosql/presto/commit/e69b66863126e8a82c62f5538d12a032a66dd662 Co-authored-by: Karol Sobczak --- .../presto/operator/OperatorAssertion.java | 20 ++++++++++-- .../operator/TestHashAggregationOperator.java | 31 +++++++++---------- 2 files changed, 32 insertions(+), 19 deletions(-) diff --git a/presto-main/src/test/java/com/facebook/presto/operator/OperatorAssertion.java b/presto-main/src/test/java/com/facebook/presto/operator/OperatorAssertion.java index 3b9cf58fdba5a..edca881047394 100644 --- a/presto-main/src/test/java/com/facebook/presto/operator/OperatorAssertion.java +++ b/presto-main/src/test/java/com/facebook/presto/operator/OperatorAssertion.java @@ -254,12 +254,26 @@ public static void assertOperatorEqualsIgnoreOrder( Optional hashChannel, boolean revokeMemoryWhenAddingPages) { - List pages = toPages(operatorFactory, driverContext, input, revokeMemoryWhenAddingPages); + assertPagesEqualIgnoreOrder( + driverContext, + toPages(operatorFactory, driverContext, input, revokeMemoryWhenAddingPages), + expected, + hashEnabled, + hashChannel); + } + + public static void assertPagesEqualIgnoreOrder( + DriverContext driverContext, + List actualPages, + MaterializedResult expected, + boolean hashEnabled, + Optional hashChannel) + { if (hashEnabled && hashChannel.isPresent()) { // Drop the hashChannel for all pages - pages = dropChannel(pages, ImmutableList.of(hashChannel.get())); + actualPages = dropChannel(actualPages, ImmutableList.of(hashChannel.get())); } - MaterializedResult actual = toMaterializedResult(driverContext.getSession(), expected.getTypes(), pages); + MaterializedResult actual = toMaterializedResult(driverContext.getSession(), expected.getTypes(), actualPages); assertEqualsIgnoreOrder(actual.getMaterializedRows(), expected.getMaterializedRows()); } diff --git a/presto-main/src/test/java/com/facebook/presto/operator/TestHashAggregationOperator.java b/presto-main/src/test/java/com/facebook/presto/operator/TestHashAggregationOperator.java index b55d29e135fe5..ee6ad50a22f56 100644 --- a/presto-main/src/test/java/com/facebook/presto/operator/TestHashAggregationOperator.java +++ b/presto-main/src/test/java/com/facebook/presto/operator/TestHashAggregationOperator.java @@ -69,6 +69,7 @@ import static com.facebook.presto.operator.GroupByHashYieldAssertion.createPagesWithDistinctHashKeys; import static com.facebook.presto.operator.GroupByHashYieldAssertion.finishOperatorWithYieldingGroupByHash; import static com.facebook.presto.operator.OperatorAssertion.assertOperatorEqualsIgnoreOrder; +import static com.facebook.presto.operator.OperatorAssertion.assertPagesEqualIgnoreOrder; import static com.facebook.presto.operator.OperatorAssertion.dropChannel; import static com.facebook.presto.operator.OperatorAssertion.toMaterializedResult; import static com.facebook.presto.operator.OperatorAssertion.toPages; @@ -153,15 +154,17 @@ public void tearDown() @Test(dataProvider = "hashEnabledAndMemoryLimitForMergeValues") public void testHashAggregation(boolean hashEnabled, boolean spillEnabled, long memoryLimitForMerge, long memoryLimitForMergeWithMemory) { + // make operator produce multiple pages during finish phase + int numberOfRows = 40_000; InternalAggregationFunction countVarcharColumn = getAggregation("count", VARCHAR); InternalAggregationFunction countBooleanColumn = getAggregation("count", BOOLEAN); InternalAggregationFunction maxVarcharColumn = getAggregation("max", VARCHAR); List hashChannels = Ints.asList(1); RowPagesBuilder rowPagesBuilder = rowPagesBuilder(hashEnabled, hashChannels, VARCHAR, VARCHAR, VARCHAR, BIGINT, BOOLEAN); List input = rowPagesBuilder - .addSequencePage(10, 100, 0, 100, 0, 500) - .addSequencePage(10, 100, 0, 200, 0, 500) - .addSequencePage(10, 100, 0, 300, 0, 500) + .addSequencePage(numberOfRows, 100, 0, 100_000, 0, 500) + .addSequencePage(numberOfRows, 100, 0, 200_000, 0, 500) + .addSequencePage(numberOfRows, 100, 0, 300_000, 0, 500) .build(); HashAggregationOperatorFactory operatorFactory = new HashAggregationOperatorFactory( @@ -191,20 +194,16 @@ public void testHashAggregation(boolean hashEnabled, boolean spillEnabled, long DriverContext driverContext = createDriverContext(memoryLimitForMerge); - MaterializedResult expected = resultBuilder(driverContext.getSession(), VARCHAR, BIGINT, BIGINT, DOUBLE, VARCHAR, BIGINT, BIGINT) - .row("0", 3L, 0L, 0.0, "300", 3L, 3L) - .row("1", 3L, 3L, 1.0, "301", 3L, 3L) - .row("2", 3L, 6L, 2.0, "302", 3L, 3L) - .row("3", 3L, 9L, 3.0, "303", 3L, 3L) - .row("4", 3L, 12L, 4.0, "304", 3L, 3L) - .row("5", 3L, 15L, 5.0, "305", 3L, 3L) - .row("6", 3L, 18L, 6.0, "306", 3L, 3L) - .row("7", 3L, 21L, 7.0, "307", 3L, 3L) - .row("8", 3L, 24L, 8.0, "308", 3L, 3L) - .row("9", 3L, 27L, 9.0, "309", 3L, 3L) - .build(); + MaterializedResult.Builder expectedBuilder = resultBuilder(driverContext.getSession(), VARCHAR, BIGINT, BIGINT, DOUBLE, VARCHAR, BIGINT, BIGINT); + for (int i = 0; i < numberOfRows; ++i) { + expectedBuilder.row(Integer.toString(i), 3L, 3L * i, (double) i, Integer.toString(300_000 + i), 3L, 3L); + } + MaterializedResult expected = expectedBuilder.build(); + + List pages = toPages(operatorFactory, driverContext, input); + assertGreaterThan(pages.size(), 1, "Expected more than one output page"); + assertPagesEqualIgnoreOrder(driverContext, pages, expected, hashEnabled, Optional.of(hashChannels.size())); - assertOperatorEqualsIgnoreOrder(operatorFactory, driverContext, input, expected, hashEnabled, Optional.of(hashChannels.size())); assertTrue(spillEnabled == (spillerFactory.getSpillsCount() > 0), format("Spill state mismatch. Expected spill: %s, spill count: %s", spillEnabled, spillerFactory.getSpillsCount())); } From 9aa39fb4893a71c7ac2f2070e987d8f389918ee2 Mon Sep 17 00:00:00 2001 From: Saksham Sachdev Date: Fri, 19 Oct 2018 16:35:56 +0200 Subject: [PATCH 04/10] Extract DummySpillerFactory from TestHashAggregationOperator Cherry-pick of https://github.com/prestosql/presto/commit/f01067ba9dda7f4a1bbcabcaf901b3c008969207 Co-authored-by: Atri Sharma --- .../presto/operator/DummySpillerFactory.java | 70 +++++++++++++++++++ .../operator/TestHashAggregationOperator.java | 42 ----------- 2 files changed, 70 insertions(+), 42 deletions(-) create mode 100644 presto-main/src/test/java/com/facebook/presto/operator/DummySpillerFactory.java diff --git a/presto-main/src/test/java/com/facebook/presto/operator/DummySpillerFactory.java b/presto-main/src/test/java/com/facebook/presto/operator/DummySpillerFactory.java new file mode 100644 index 0000000000000..6efd315cee599 --- /dev/null +++ b/presto-main/src/test/java/com/facebook/presto/operator/DummySpillerFactory.java @@ -0,0 +1,70 @@ +/* + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package com.facebook.presto.operator; + +import com.facebook.presto.common.Page; +import com.facebook.presto.common.type.Type; +import com.facebook.presto.memory.context.AggregatedMemoryContext; +import com.facebook.presto.spiller.Spiller; +import com.facebook.presto.spiller.SpillerFactory; +import com.google.common.collect.ImmutableList; +import com.google.common.util.concurrent.ListenableFuture; + +import java.util.ArrayList; +import java.util.Iterator; +import java.util.List; + +import static com.google.common.collect.ImmutableList.toImmutableList; +import static com.google.common.util.concurrent.Futures.immediateFuture; + +public class DummySpillerFactory + implements SpillerFactory +{ + private long spillsCount; + + @Override + public Spiller create(List types, SpillContext spillContext, AggregatedMemoryContext memoryContext) + { + return new Spiller() + { + private final List> spills = new ArrayList<>(); + + @Override + public ListenableFuture spill(Iterator pageIterator) + { + spillsCount++; + spills.add(ImmutableList.copyOf(pageIterator)); + return immediateFuture(null); + } + + @Override + public List> getSpills() + { + return spills.stream() + .map(Iterable::iterator) + .collect(toImmutableList()); + } + + @Override + public void close() + { + } + }; + } + + public long getSpillsCount() + { + return spillsCount; + } +} diff --git a/presto-main/src/test/java/com/facebook/presto/operator/TestHashAggregationOperator.java b/presto-main/src/test/java/com/facebook/presto/operator/TestHashAggregationOperator.java index ee6ad50a22f56..0201b74b2dbd4 100644 --- a/presto-main/src/test/java/com/facebook/presto/operator/TestHashAggregationOperator.java +++ b/presto-main/src/test/java/com/facebook/presto/operator/TestHashAggregationOperator.java @@ -80,7 +80,6 @@ import static com.google.common.collect.ImmutableList.toImmutableList; import static com.google.common.collect.Iterables.getOnlyElement; import static com.google.common.util.concurrent.Futures.immediateFailedFuture; -import static com.google.common.util.concurrent.Futures.immediateFuture; import static io.airlift.slice.SizeOf.SIZE_OF_DOUBLE; import static io.airlift.slice.SizeOf.SIZE_OF_LONG; import static io.airlift.units.DataSize.Unit.KILOBYTE; @@ -781,47 +780,6 @@ private static InternalAggregationFunction getAggregation(String name, Type... a return functionManager.getAggregateFunctionImplementation(functionManager.lookupFunction(name, fromTypes(arguments))); } - private static class DummySpillerFactory - implements SpillerFactory - { - private long spillsCount; - - @Override - public Spiller create(List types, SpillContext spillContext, AggregatedMemoryContext memoryContext) - { - return new Spiller() - { - private final List> spills = new ArrayList<>(); - - @Override - public ListenableFuture spill(Iterator pageIterator) - { - spillsCount++; - spills.add(ImmutableList.copyOf(pageIterator)); - return immediateFuture(null); - } - - @Override - public List> getSpills() - { - return spills.stream() - .map(Iterable::iterator) - .collect(toImmutableList()); - } - - @Override - public void close() - { - } - }; - } - - public long getSpillsCount() - { - return spillsCount; - } - } - private static class FailingSpillerFactory implements SpillerFactory { From 3af951b6a96b219164f5b67aac1366d7615cb83e Mon Sep 17 00:00:00 2001 From: Saksham Sachdev Date: Thu, 9 Jul 2020 16:09:08 -0700 Subject: [PATCH 05/10] Use WorkProcessor in OrderByOperator Cherry-pick of https://github.com/prestosql/presto/commit/e7d8dd5e7035520d1dbb7990803b51383cc11aae Co-authored-by: Piotr Findeisen --- .../presto/operator/OrderByOperator.java | 53 ++++++++----------- .../facebook/presto/operator/PagesIndex.java | 1 - 2 files changed, 22 insertions(+), 32 deletions(-) diff --git a/presto-main/src/main/java/com/facebook/presto/operator/OrderByOperator.java b/presto-main/src/main/java/com/facebook/presto/operator/OrderByOperator.java index 550893717fd9f..f232fa6f40455 100644 --- a/presto-main/src/main/java/com/facebook/presto/operator/OrderByOperator.java +++ b/presto-main/src/main/java/com/facebook/presto/operator/OrderByOperator.java @@ -14,7 +14,7 @@ package com.facebook.presto.operator; import com.facebook.presto.common.Page; -import com.facebook.presto.common.PageBuilder; +import com.facebook.presto.common.block.Block; import com.facebook.presto.common.block.SortOrder; import com.facebook.presto.common.type.Type; import com.facebook.presto.memory.context.LocalMemoryContext; @@ -22,9 +22,11 @@ import com.google.common.collect.ImmutableList; import com.google.common.primitives.Ints; +import java.util.Iterator; import java.util.List; import static com.google.common.base.Preconditions.checkState; +import static com.google.common.base.Verify.verify; import static java.util.Objects.requireNonNull; public class OrderByOperator @@ -108,8 +110,7 @@ private enum State private final PagesIndex pageIndex; - private final PageBuilder pageBuilder; - private int currentPosition; + private Iterator sortedPages; private State state = State.NEEDS_INPUT; @@ -131,8 +132,6 @@ public OrderByOperator( this.localUserMemoryContext = operatorContext.localUserMemoryContext(); this.pageIndex = pagesIndexFactory.newPagesIndex(sourceTypes, expectedPositions); - - this.pageBuilder = new PageBuilder(toTypes(sourceTypes, outputChannels)); } @Override @@ -147,8 +146,9 @@ public void finish() if (state == State.NEEDS_INPUT) { state = State.HAS_OUTPUT; - // sort the index pageIndex.sort(sortChannels, sortOrder); + WorkProcessor resultPages = WorkProcessor.fromIterator(pageIndex.getSortedPages()); + sortedPages = resultPages.iterator(); } } @@ -171,11 +171,7 @@ public void addInput(Page page) requireNonNull(page, "page is null"); pageIndex.addPage(page); - - if (!localUserMemoryContext.trySetBytes(pageIndex.getEstimatedSize().toBytes())) { - pageIndex.compact(); - localUserMemoryContext.setBytes(pageIndex.getEstimatedSize().toBytes()); - } + updateMemoryUsage(); } @Override @@ -185,37 +181,32 @@ public Page getOutput() return null; } - if (currentPosition >= pageIndex.getPositionCount()) { + verify(sortedPages != null, "sortedPages is null"); + if (!sortedPages.hasNext()) { state = State.FINISHED; return null; } - // iterate through the positions sequentially until we have one full page - pageBuilder.reset(); - currentPosition = pageIndex.buildPage(currentPosition, outputChannels, pageBuilder); - - // output the page if we have any data - if (pageBuilder.isEmpty()) { - state = State.FINISHED; - return null; + Page nextPage = sortedPages.next(); + Block[] blocks = new Block[outputChannels.length]; + for (int i = 0; i < outputChannels.length; i++) { + blocks[i] = nextPage.getBlock(outputChannels[i]); } + return new Page(nextPage.getPositionCount(), blocks); + } - Page page = pageBuilder.build(); - return page; + private void updateMemoryUsage() + { + if (!localUserMemoryContext.trySetBytes(pageIndex.getEstimatedSize().toBytes())) { + pageIndex.compact(); + localUserMemoryContext.setBytes(pageIndex.getEstimatedSize().toBytes()); + } } @Override public void close() { pageIndex.clear(); - } - - private static List toTypes(List sourceTypes, List outputChannels) - { - ImmutableList.Builder types = ImmutableList.builder(); - for (int channel : outputChannels) { - types.add(sourceTypes.get(channel)); - } - return types.build(); + sortedPages = null; } } diff --git a/presto-main/src/main/java/com/facebook/presto/operator/PagesIndex.java b/presto-main/src/main/java/com/facebook/presto/operator/PagesIndex.java index 30454f4893a73..ac4d9ba7b6f71 100644 --- a/presto-main/src/main/java/com/facebook/presto/operator/PagesIndex.java +++ b/presto-main/src/main/java/com/facebook/presto/operator/PagesIndex.java @@ -564,7 +564,6 @@ protected Page computeNext() }; } - // TODO: This is similar to what OrderByOperator does, look into reusing this logic in OrderByOperator as well. public Iterator getSortedPages() { return new AbstractIterator() From e24359d19908df10d879164f4f64a3ee1e4a50ab Mon Sep 17 00:00:00 2001 From: Saksham Sachdev Date: Fri, 10 Jul 2020 15:27:50 -0700 Subject: [PATCH 06/10] Add Spill To Disk for ORDER BY ORDER BY currently will error out if the data being processed exceeds query memory limit. This commit introduces paging from disk and ensures that ORDER BY is limited only by the amount of disk present. Cherry-pick of https://github.com/prestosql/presto/commit/24868b3481a974f3e068197d9c432788de9ddacb Co-authored-by: Piotr Findeisen --- .../presto/benchmark/OrderByBenchmark.java | 5 +- .../presto/operator/OrderByOperator.java | 171 ++++++++++++++++-- .../sql/planner/LocalExecutionPlanner.java | 6 +- .../presto/operator/TestOrderByOperator.java | 36 +++- 4 files changed, 195 insertions(+), 23 deletions(-) diff --git a/presto-benchmark/src/main/java/com/facebook/presto/benchmark/OrderByBenchmark.java b/presto-benchmark/src/main/java/com/facebook/presto/benchmark/OrderByBenchmark.java index cb6e6047836a6..e9b7162cda070 100644 --- a/presto-benchmark/src/main/java/com/facebook/presto/benchmark/OrderByBenchmark.java +++ b/presto-benchmark/src/main/java/com/facebook/presto/benchmark/OrderByBenchmark.java @@ -23,6 +23,7 @@ import com.google.common.collect.ImmutableList; import java.util.List; +import java.util.Optional; import static com.facebook.presto.benchmark.BenchmarkQueryRunner.createLocalQueryRunner; import static com.facebook.presto.common.block.SortOrder.ASC_NULLS_LAST; @@ -53,7 +54,9 @@ protected List createOperatorFactories() ROWS, ImmutableList.of(0), ImmutableList.of(ASC_NULLS_LAST), - new PagesIndex.TestingFactory(false)); + new PagesIndex.TestingFactory(false), + false, + Optional.empty()); return ImmutableList.of(tableScanOperator, limitOperator, orderByOperator); } diff --git a/presto-main/src/main/java/com/facebook/presto/operator/OrderByOperator.java b/presto-main/src/main/java/com/facebook/presto/operator/OrderByOperator.java index f232fa6f40455..83037b8462f84 100644 --- a/presto-main/src/main/java/com/facebook/presto/operator/OrderByOperator.java +++ b/presto-main/src/main/java/com/facebook/presto/operator/OrderByOperator.java @@ -19,14 +19,24 @@ import com.facebook.presto.common.type.Type; import com.facebook.presto.memory.context.LocalMemoryContext; import com.facebook.presto.spi.plan.PlanNodeId; +import com.facebook.presto.spiller.Spiller; +import com.facebook.presto.spiller.SpillerFactory; import com.google.common.collect.ImmutableList; import com.google.common.primitives.Ints; +import com.google.common.util.concurrent.ListenableFuture; import java.util.Iterator; import java.util.List; +import java.util.Optional; +import static com.facebook.airlift.concurrent.MoreFutures.checkSuccess; +import static com.facebook.presto.util.MergeSortedPages.mergeSortedPages; +import static com.google.common.base.Preconditions.checkArgument; import static com.google.common.base.Preconditions.checkState; import static com.google.common.base.Verify.verify; +import static com.google.common.collect.ImmutableList.toImmutableList; +import static com.google.common.collect.Iterators.transform; +import static com.google.common.util.concurrent.Futures.immediateFuture; import static java.util.Objects.requireNonNull; public class OrderByOperator @@ -44,6 +54,8 @@ public static class OrderByOperatorFactory private final List sortOrder; private boolean closed; private final PagesIndex.Factory pagesIndexFactory; + private final boolean spillEnabled; + private final Optional spillerFactory; public OrderByOperatorFactory( int operatorId, @@ -53,7 +65,9 @@ public OrderByOperatorFactory( int expectedPositions, List sortChannels, List sortOrder, - PagesIndex.Factory pagesIndexFactory) + PagesIndex.Factory pagesIndexFactory, + boolean spillEnabled, + Optional spillerFactory) { this.operatorId = operatorId; this.planNodeId = requireNonNull(planNodeId, "planNodeId is null"); @@ -64,6 +78,9 @@ public OrderByOperatorFactory( this.sortOrder = ImmutableList.copyOf(requireNonNull(sortOrder, "sortOrder is null")); this.pagesIndexFactory = requireNonNull(pagesIndexFactory, "pagesIndexFactory is null"); + this.spillEnabled = spillEnabled; + this.spillerFactory = requireNonNull(spillerFactory, "spillerFactory is null"); + checkArgument(!spillEnabled || spillerFactory.isPresent(), "Spiller Factory is not present when spill is enabled"); } @Override @@ -79,7 +96,9 @@ public Operator createOperator(DriverContext driverContext) expectedPositions, sortChannels, sortOrder, - pagesIndexFactory); + pagesIndexFactory, + spillEnabled, + spillerFactory); } @Override @@ -91,7 +110,17 @@ public void noMoreOperators() @Override public OperatorFactory duplicate() { - return new OrderByOperatorFactory(operatorId, planNodeId, sourceTypes, outputChannels, expectedPositions, sortChannels, sortOrder, pagesIndexFactory); + return new OrderByOperatorFactory( + operatorId, + planNodeId, + sourceTypes, + outputChannels, + expectedPositions, + sortChannels, + sortOrder, + pagesIndexFactory, + spillEnabled, + spillerFactory); } } @@ -106,11 +135,21 @@ private enum State private final List sortChannels; private final List sortOrder; private final int[] outputChannels; + private final LocalMemoryContext revocableMemoryContext; private final LocalMemoryContext localUserMemoryContext; private final PagesIndex pageIndex; - private Iterator sortedPages; + private final List sourceTypes; + + private final boolean spillEnabled; + private final Optional spillerFactory; + + private Optional spiller = Optional.empty(); + private ListenableFuture spillInProgress = immediateFuture(null); + private Runnable finishMemoryRevoke = () -> {}; + + private Iterator> sortedPages; private State state = State.NEEDS_INPUT; @@ -121,7 +160,9 @@ public OrderByOperator( int expectedPositions, List sortChannels, List sortOrder, - PagesIndex.Factory pagesIndexFactory) + PagesIndex.Factory pagesIndexFactory, + boolean spillEnabled, + Optional spillerFactory) { requireNonNull(pagesIndexFactory, "pagesIndexFactory is null"); @@ -129,9 +170,14 @@ public OrderByOperator( this.outputChannels = Ints.toArray(requireNonNull(outputChannels, "outputChannels is null")); this.sortChannels = ImmutableList.copyOf(requireNonNull(sortChannels, "sortChannels is null")); this.sortOrder = ImmutableList.copyOf(requireNonNull(sortOrder, "sortOrder is null")); + this.sourceTypes = ImmutableList.copyOf(requireNonNull(sourceTypes, "sourceTypes is null")); this.localUserMemoryContext = operatorContext.localUserMemoryContext(); + this.revocableMemoryContext = operatorContext.localRevocableMemoryContext(); this.pageIndex = pagesIndexFactory.newPagesIndex(sourceTypes, expectedPositions); + this.spillEnabled = spillEnabled; + this.spillerFactory = requireNonNull(spillerFactory, "spillerFactory is null"); + checkArgument(!spillEnabled || spillerFactory.isPresent(), "Spiller Factory is not present when spill is enabled"); } @Override @@ -143,12 +189,28 @@ public OperatorContext getOperatorContext() @Override public void finish() { + if (!spillInProgress.isDone()) { + return; + } + checkSuccess(spillInProgress, "spilling failed"); + if (state == State.NEEDS_INPUT) { state = State.HAS_OUTPUT; + // transition the revocable memory to regular memory, since we no longer can give the memory back + // TODO when we have more memory than fits in regular memory, we should spill instead + updateMemoryUsage(); + pageIndex.sort(sortChannels, sortOrder); - WorkProcessor resultPages = WorkProcessor.fromIterator(pageIndex.getSortedPages()); - sortedPages = resultPages.iterator(); + Iterator sortedPagesIndex = pageIndex.getSortedPages(); + + List> spilledPages = getSpilledPages(); + if (spilledPages.isEmpty()) { + sortedPages = transform(sortedPagesIndex, Optional::of); + } + else { + sortedPages = mergeSpilledAndMemoryPages(spilledPages, sortedPagesIndex).yieldingIterator(); + } } } @@ -169,6 +231,7 @@ public void addInput(Page page) { checkState(state == State.NEEDS_INPUT, "Operator is already finishing"); requireNonNull(page, "page is null"); + checkSuccess(spillInProgress, "spilling failed"); pageIndex.addPage(page); updateMemoryUsage(); @@ -177,6 +240,7 @@ public void addInput(Page page) @Override public Page getOutput() { + checkSuccess(spillInProgress, "spilling failed"); if (state != State.HAS_OUTPUT) { return null; } @@ -187,7 +251,11 @@ public Page getOutput() return null; } - Page nextPage = sortedPages.next(); + Optional next = sortedPages.next(); + if (!next.isPresent()) { + return null; + } + Page nextPage = next.get(); Block[] blocks = new Block[outputChannels.length]; for (int i = 0; i < outputChannels.length; i++) { blocks[i] = nextPage.getBlock(outputChannels[i]); @@ -195,11 +263,92 @@ public Page getOutput() return new Page(nextPage.getPositionCount(), blocks); } + @Override + public ListenableFuture startMemoryRevoke() + { + checkSuccess(spillInProgress, "spilling failed"); + + if (revocableMemoryContext.getBytes() == 0) { + // This must be stale revoke request + + verify(pageIndex.getPositionCount() == 0 || state == State.HAS_OUTPUT); + finishMemoryRevoke = () -> {}; + return immediateFuture(null); + } + + verify(state == State.NEEDS_INPUT, "Cannot spill in %s state", state); + + // TODO try pageIndex.compact(); before spilling, as in com.facebook.presto.operator.HashBuilderOperator.startMemoryRevoke + + if (!spiller.isPresent()) { + spiller = Optional.of(spillerFactory.get().create( + sourceTypes, + operatorContext.getSpillContext(), + operatorContext.newAggregateSystemMemoryContext())); + } + + pageIndex.sort(sortChannels, sortOrder); + spillInProgress = spiller.get().spill(pageIndex.getSortedPages()); + finishMemoryRevoke = () -> { + pageIndex.clear(); + updateMemoryUsage(); + }; + + return spillInProgress; + } + + @Override + public void finishMemoryRevoke() + { + finishMemoryRevoke.run(); + finishMemoryRevoke = () -> {}; + } + + private List> getSpilledPages() + { + if (!spiller.isPresent()) { + return ImmutableList.of(); + } + + return spiller.get().getSpills().stream() + .map(WorkProcessor::fromIterator) + .collect(toImmutableList()); + } + + private WorkProcessor mergeSpilledAndMemoryPages(List> spilledPages, Iterator sortedPagesIndex) + { + List> sortedStreams = ImmutableList.>builder() + .addAll(spilledPages) + .add(WorkProcessor.fromIterator(sortedPagesIndex)) + .build(); + + return mergeSortedPages( + sortedStreams, + // TODO use compiled comparator, like PagesIndex's OrderingCompiler + new SimplePageWithPositionComparator(sourceTypes, sortChannels, sortOrder), + sourceTypes, + operatorContext.aggregateUserMemoryContext(), + operatorContext.getDriverContext().getYieldSignal()); + } + private void updateMemoryUsage() { - if (!localUserMemoryContext.trySetBytes(pageIndex.getEstimatedSize().toBytes())) { - pageIndex.compact(); - localUserMemoryContext.setBytes(pageIndex.getEstimatedSize().toBytes()); + if (spillEnabled && state == State.NEEDS_INPUT) { + if (pageIndex.getPositionCount() == 0) { + localUserMemoryContext.setBytes(pageIndex.getEstimatedSize().toBytes()); + revocableMemoryContext.setBytes(0L); + } + else { + localUserMemoryContext.setBytes(0); + revocableMemoryContext.setBytes(pageIndex.getEstimatedSize().toBytes()); + } + } + else { + revocableMemoryContext.setBytes(0); + if (!localUserMemoryContext.trySetBytes(pageIndex.getEstimatedSize().toBytes())) { + pageIndex.compact(); + localUserMemoryContext.setBytes(pageIndex.getEstimatedSize().toBytes()); + } } } diff --git a/presto-main/src/main/java/com/facebook/presto/sql/planner/LocalExecutionPlanner.java b/presto-main/src/main/java/com/facebook/presto/sql/planner/LocalExecutionPlanner.java index a9e1af8030720..790c39196ae3b 100644 --- a/presto-main/src/main/java/com/facebook/presto/sql/planner/LocalExecutionPlanner.java +++ b/presto-main/src/main/java/com/facebook/presto/sql/planner/LocalExecutionPlanner.java @@ -1058,6 +1058,8 @@ public PhysicalOperation visitSort(SortNode node, LocalExecutionPlanContext cont outputChannels.add(i); } + boolean spillEnabled = isSpillEnabled(context.getSession()); + OperatorFactory operator = new OrderByOperatorFactory( context.getNextOperatorId(), node.getId(), @@ -1066,7 +1068,9 @@ public PhysicalOperation visitSort(SortNode node, LocalExecutionPlanContext cont 10_000, orderByChannels, sortOrder.build(), - pagesIndexFactory); + pagesIndexFactory, + spillEnabled, + Optional.of(spillerFactory)); return new PhysicalOperation(operator, source.getLayout(), context, source); } diff --git a/presto-main/src/test/java/com/facebook/presto/operator/TestOrderByOperator.java b/presto-main/src/test/java/com/facebook/presto/operator/TestOrderByOperator.java index 0b053026c6247..133f63cfd791d 100644 --- a/presto-main/src/test/java/com/facebook/presto/operator/TestOrderByOperator.java +++ b/presto-main/src/test/java/com/facebook/presto/operator/TestOrderByOperator.java @@ -23,9 +23,11 @@ import io.airlift.units.DataSize.Unit; import org.testng.annotations.AfterMethod; import org.testng.annotations.BeforeMethod; +import org.testng.annotations.DataProvider; import org.testng.annotations.Test; import java.util.List; +import java.util.Optional; import java.util.concurrent.ExecutorService; import java.util.concurrent.ScheduledExecutorService; @@ -51,6 +53,12 @@ public class TestOrderByOperator private ScheduledExecutorService scheduledExecutor; private DriverContext driverContext; + @DataProvider + public static Object[][] spillEnabled() + { + return new Object[][] {{false}, {true}}; + } + @BeforeMethod public void setUp() { @@ -68,8 +76,8 @@ public void tearDown() scheduledExecutor.shutdownNow(); } - @Test - public void testSingleFieldKey() + @Test(dataProvider = "spillEnabled") + public void testSingleFieldKey(boolean spillEnabled) { List input = rowPagesBuilder(BIGINT, DOUBLE) .row(1L, 0.1) @@ -87,7 +95,9 @@ public void testSingleFieldKey() 10, ImmutableList.of(0), ImmutableList.of(ASC_NULLS_LAST), - new PagesIndex.TestingFactory(false)); + new PagesIndex.TestingFactory(false), + spillEnabled, + Optional.of(new DummySpillerFactory())); MaterializedResult expected = resultBuilder(driverContext.getSession(), DOUBLE) .row(-0.1) @@ -99,8 +109,8 @@ public void testSingleFieldKey() assertOperatorEquals(operatorFactory, driverContext, input, expected); } - @Test - public void testMultiFieldKey() + @Test(dataProvider = "spillEnabled") + public void testMultiFieldKey(boolean spillEnabled) { List input = rowPagesBuilder(VARCHAR, BIGINT) .row("a", 1L) @@ -118,7 +128,9 @@ public void testMultiFieldKey() 10, ImmutableList.of(0, 1), ImmutableList.of(ASC_NULLS_LAST, DESC_NULLS_LAST), - new PagesIndex.TestingFactory(false)); + new PagesIndex.TestingFactory(false), + spillEnabled, + Optional.of(new DummySpillerFactory())); MaterializedResult expected = MaterializedResult.resultBuilder(driverContext.getSession(), VARCHAR, BIGINT) .row("a", 4L) @@ -130,8 +142,8 @@ public void testMultiFieldKey() assertOperatorEquals(operatorFactory, driverContext, input, expected); } - @Test - public void testReverseOrder() + @Test(dataProvider = "spillEnabled") + public void testReverseOrder(boolean spillEnabled) { List input = rowPagesBuilder(BIGINT, DOUBLE) .row(1L, 0.1) @@ -149,7 +161,9 @@ public void testReverseOrder() 10, ImmutableList.of(0), ImmutableList.of(DESC_NULLS_LAST), - new PagesIndex.TestingFactory(false)); + new PagesIndex.TestingFactory(false), + spillEnabled, + Optional.of(new DummySpillerFactory())); MaterializedResult expected = resultBuilder(driverContext.getSession(), BIGINT) .row(4L) @@ -184,7 +198,9 @@ public void testMemoryLimit() 10, ImmutableList.of(0), ImmutableList.of(ASC_NULLS_LAST), - new PagesIndex.TestingFactory(false)); + new PagesIndex.TestingFactory(false), + false, + Optional.of(new DummySpillerFactory())); toPages(operatorFactory, driverContext, input); } From 52adeba9d527dce64ceeabeac3e4ac1dc5c642d7 Mon Sep 17 00:00:00 2001 From: Saksham Sachdev Date: Fri, 10 Jul 2020 15:46:31 -0700 Subject: [PATCH 07/10] Use OrderingCompiler in OrderBy spilling Cherry-pick of https://github.com/prestosql/presto/commit/472fb5ad36cb0e0fea0a2cf5f90e598b28ce01fe Co-authored-by: Karol Sobczak --- .../presto/benchmark/OrderByBenchmark.java | 4 +++- .../presto/operator/OrderByOperator.java | 23 +++++++++++++------ .../presto/sql/gen/OrderingCompiler.java | 2 +- .../sql/planner/LocalExecutionPlanner.java | 3 ++- .../presto/operator/TestOrderByOperator.java | 13 +++++++---- 5 files changed, 31 insertions(+), 14 deletions(-) diff --git a/presto-benchmark/src/main/java/com/facebook/presto/benchmark/OrderByBenchmark.java b/presto-benchmark/src/main/java/com/facebook/presto/benchmark/OrderByBenchmark.java index e9b7162cda070..9c37c10827c21 100644 --- a/presto-benchmark/src/main/java/com/facebook/presto/benchmark/OrderByBenchmark.java +++ b/presto-benchmark/src/main/java/com/facebook/presto/benchmark/OrderByBenchmark.java @@ -19,6 +19,7 @@ import com.facebook.presto.operator.OrderByOperator.OrderByOperatorFactory; import com.facebook.presto.operator.PagesIndex; import com.facebook.presto.spi.plan.PlanNodeId; +import com.facebook.presto.sql.gen.OrderingCompiler; import com.facebook.presto.testing.LocalQueryRunner; import com.google.common.collect.ImmutableList; @@ -56,7 +57,8 @@ protected List createOperatorFactories() ImmutableList.of(ASC_NULLS_LAST), new PagesIndex.TestingFactory(false), false, - Optional.empty()); + Optional.empty(), + new OrderingCompiler()); return ImmutableList.of(tableScanOperator, limitOperator, orderByOperator); } diff --git a/presto-main/src/main/java/com/facebook/presto/operator/OrderByOperator.java b/presto-main/src/main/java/com/facebook/presto/operator/OrderByOperator.java index 83037b8462f84..2340be0e94cae 100644 --- a/presto-main/src/main/java/com/facebook/presto/operator/OrderByOperator.java +++ b/presto-main/src/main/java/com/facebook/presto/operator/OrderByOperator.java @@ -21,6 +21,7 @@ import com.facebook.presto.spi.plan.PlanNodeId; import com.facebook.presto.spiller.Spiller; import com.facebook.presto.spiller.SpillerFactory; +import com.facebook.presto.sql.gen.OrderingCompiler; import com.google.common.collect.ImmutableList; import com.google.common.primitives.Ints; import com.google.common.util.concurrent.ListenableFuture; @@ -52,10 +53,12 @@ public static class OrderByOperatorFactory private final int expectedPositions; private final List sortChannels; private final List sortOrder; - private boolean closed; private final PagesIndex.Factory pagesIndexFactory; private final boolean spillEnabled; private final Optional spillerFactory; + private final OrderingCompiler orderingCompiler; + + private boolean closed; public OrderByOperatorFactory( int operatorId, @@ -67,7 +70,8 @@ public OrderByOperatorFactory( List sortOrder, PagesIndex.Factory pagesIndexFactory, boolean spillEnabled, - Optional spillerFactory) + Optional spillerFactory, + OrderingCompiler orderingCompiler) { this.operatorId = operatorId; this.planNodeId = requireNonNull(planNodeId, "planNodeId is null"); @@ -80,6 +84,7 @@ public OrderByOperatorFactory( this.pagesIndexFactory = requireNonNull(pagesIndexFactory, "pagesIndexFactory is null"); this.spillEnabled = spillEnabled; this.spillerFactory = requireNonNull(spillerFactory, "spillerFactory is null"); + this.orderingCompiler = requireNonNull(orderingCompiler, "orderingCompiler is null"); checkArgument(!spillEnabled || spillerFactory.isPresent(), "Spiller Factory is not present when spill is enabled"); } @@ -98,7 +103,8 @@ public Operator createOperator(DriverContext driverContext) sortOrder, pagesIndexFactory, spillEnabled, - spillerFactory); + spillerFactory, + orderingCompiler); } @Override @@ -120,7 +126,8 @@ public OperatorFactory duplicate() sortOrder, pagesIndexFactory, spillEnabled, - spillerFactory); + spillerFactory, + orderingCompiler); } } @@ -144,6 +151,7 @@ private enum State private final boolean spillEnabled; private final Optional spillerFactory; + private final OrderingCompiler orderingCompiler; private Optional spiller = Optional.empty(); private ListenableFuture spillInProgress = immediateFuture(null); @@ -162,7 +170,8 @@ public OrderByOperator( List sortOrder, PagesIndex.Factory pagesIndexFactory, boolean spillEnabled, - Optional spillerFactory) + Optional spillerFactory, + OrderingCompiler orderingCompiler) { requireNonNull(pagesIndexFactory, "pagesIndexFactory is null"); @@ -177,6 +186,7 @@ public OrderByOperator( this.pageIndex = pagesIndexFactory.newPagesIndex(sourceTypes, expectedPositions); this.spillEnabled = spillEnabled; this.spillerFactory = requireNonNull(spillerFactory, "spillerFactory is null"); + this.orderingCompiler = requireNonNull(orderingCompiler, "orderingCompiler is null"); checkArgument(!spillEnabled || spillerFactory.isPresent(), "Spiller Factory is not present when spill is enabled"); } @@ -324,8 +334,7 @@ private WorkProcessor mergeSpilledAndMemoryPages(List> return mergeSortedPages( sortedStreams, - // TODO use compiled comparator, like PagesIndex's OrderingCompiler - new SimplePageWithPositionComparator(sourceTypes, sortChannels, sortOrder), + orderingCompiler.compilePageWithPositionComparator(sourceTypes, sortChannels, sortOrder), sourceTypes, operatorContext.aggregateUserMemoryContext(), operatorContext.getDriverContext().getYieldSignal()); diff --git a/presto-main/src/main/java/com/facebook/presto/sql/gen/OrderingCompiler.java b/presto-main/src/main/java/com/facebook/presto/sql/gen/OrderingCompiler.java index f54d26c533251..3e1a4d401d372 100644 --- a/presto-main/src/main/java/com/facebook/presto/sql/gen/OrderingCompiler.java +++ b/presto-main/src/main/java/com/facebook/presto/sql/gen/OrderingCompiler.java @@ -248,7 +248,7 @@ private PageWithPositionComparator internalCompilePageWithPositionComparator(Lis comparator = pageWithPositionsComparatorClass.getConstructor().newInstance(); } catch (Throwable t) { - log.error(t, "Error compiling merge sort comparator for channels %s with order %s", sortChannels, sortChannels); + log.error(t, "Error compiling comparator for channels %s with order %s", sortChannels, sortChannels); comparator = new SimplePageWithPositionComparator(types, sortChannels, sortOrders); } return comparator; diff --git a/presto-main/src/main/java/com/facebook/presto/sql/planner/LocalExecutionPlanner.java b/presto-main/src/main/java/com/facebook/presto/sql/planner/LocalExecutionPlanner.java index 790c39196ae3b..491e9ee1a0427 100644 --- a/presto-main/src/main/java/com/facebook/presto/sql/planner/LocalExecutionPlanner.java +++ b/presto-main/src/main/java/com/facebook/presto/sql/planner/LocalExecutionPlanner.java @@ -1070,7 +1070,8 @@ public PhysicalOperation visitSort(SortNode node, LocalExecutionPlanContext cont sortOrder.build(), pagesIndexFactory, spillEnabled, - Optional.of(spillerFactory)); + Optional.of(spillerFactory), + orderingCompiler); return new PhysicalOperation(operator, source.getLayout(), context, source); } diff --git a/presto-main/src/test/java/com/facebook/presto/operator/TestOrderByOperator.java b/presto-main/src/test/java/com/facebook/presto/operator/TestOrderByOperator.java index 133f63cfd791d..e0867bf4494c6 100644 --- a/presto-main/src/test/java/com/facebook/presto/operator/TestOrderByOperator.java +++ b/presto-main/src/test/java/com/facebook/presto/operator/TestOrderByOperator.java @@ -17,6 +17,7 @@ import com.facebook.presto.common.Page; import com.facebook.presto.operator.OrderByOperator.OrderByOperatorFactory; import com.facebook.presto.spi.plan.PlanNodeId; +import com.facebook.presto.sql.gen.OrderingCompiler; import com.facebook.presto.testing.MaterializedResult; import com.google.common.collect.ImmutableList; import io.airlift.units.DataSize; @@ -97,7 +98,8 @@ public void testSingleFieldKey(boolean spillEnabled) ImmutableList.of(ASC_NULLS_LAST), new PagesIndex.TestingFactory(false), spillEnabled, - Optional.of(new DummySpillerFactory())); + Optional.of(new DummySpillerFactory()), + new OrderingCompiler()); MaterializedResult expected = resultBuilder(driverContext.getSession(), DOUBLE) .row(-0.1) @@ -130,7 +132,8 @@ public void testMultiFieldKey(boolean spillEnabled) ImmutableList.of(ASC_NULLS_LAST, DESC_NULLS_LAST), new PagesIndex.TestingFactory(false), spillEnabled, - Optional.of(new DummySpillerFactory())); + Optional.of(new DummySpillerFactory()), + new OrderingCompiler()); MaterializedResult expected = MaterializedResult.resultBuilder(driverContext.getSession(), VARCHAR, BIGINT) .row("a", 4L) @@ -163,7 +166,8 @@ public void testReverseOrder(boolean spillEnabled) ImmutableList.of(DESC_NULLS_LAST), new PagesIndex.TestingFactory(false), spillEnabled, - Optional.of(new DummySpillerFactory())); + Optional.of(new DummySpillerFactory()), + new OrderingCompiler()); MaterializedResult expected = resultBuilder(driverContext.getSession(), BIGINT) .row(4L) @@ -200,7 +204,8 @@ public void testMemoryLimit() ImmutableList.of(ASC_NULLS_LAST), new PagesIndex.TestingFactory(false), false, - Optional.of(new DummySpillerFactory())); + Optional.of(new DummySpillerFactory()), + new OrderingCompiler()); toPages(operatorFactory, driverContext, input); } From 28962d5b23751ee4289698876c01c333fb5255e4 Mon Sep 17 00:00:00 2001 From: Saksham Sachdev Date: Mon, 13 Jul 2020 15:21:27 -0700 Subject: [PATCH 08/10] Convert revocable memory to user memory on OrderBy finish Cherry-pick of https://github.com/prestosql/presto/commit/d35e5fb834ef93ccadea5bf76739ab9e814e5eac Co-authored-by: Karol Sobczak --- .../presto/operator/OrderByOperator.java | 28 ++++-- .../presto/operator/OperatorAssertion.java | 26 +++++- .../presto/operator/TestOrderByOperator.java | 93 ++++++++++++++++--- 3 files changed, 123 insertions(+), 24 deletions(-) diff --git a/presto-main/src/main/java/com/facebook/presto/operator/OrderByOperator.java b/presto-main/src/main/java/com/facebook/presto/operator/OrderByOperator.java index 2340be0e94cae..4b16e73bd97d7 100644 --- a/presto-main/src/main/java/com/facebook/presto/operator/OrderByOperator.java +++ b/presto-main/src/main/java/com/facebook/presto/operator/OrderByOperator.java @@ -31,6 +31,7 @@ import java.util.Optional; import static com.facebook.airlift.concurrent.MoreFutures.checkSuccess; +import static com.facebook.airlift.concurrent.MoreFutures.getFutureValue; import static com.facebook.presto.util.MergeSortedPages.mergeSortedPages; import static com.google.common.base.Preconditions.checkArgument; import static com.google.common.base.Preconditions.checkState; @@ -207,9 +208,20 @@ public void finish() if (state == State.NEEDS_INPUT) { state = State.HAS_OUTPUT; - // transition the revocable memory to regular memory, since we no longer can give the memory back - // TODO when we have more memory than fits in regular memory, we should spill instead - updateMemoryUsage(); + // Convert revocable memory to user memory as sortedPages holds on to memory so we no longer can revoke. + if (revocableMemoryContext.getBytes() > 0) { + long currentRevocableBytes = revocableMemoryContext.getBytes(); + revocableMemoryContext.setBytes(0); + if (!localUserMemoryContext.trySetBytes(localUserMemoryContext.getBytes() + currentRevocableBytes)) { + // TODO: this might fail (even though we have just released memory), but we don't + // have a proper way to atomically convert memory reservations + revocableMemoryContext.setBytes(currentRevocableBytes); + // spill since revocable memory could not be converted to user memory immediately + // TODO: this should be asynchronous + getFutureValue(spillToDisk()); + finishMemoryRevoke.run(); + } + } pageIndex.sort(sortChannels, sortOrder); Iterator sortedPagesIndex = pageIndex.getSortedPages(); @@ -275,19 +287,21 @@ public Page getOutput() @Override public ListenableFuture startMemoryRevoke() + { + verify(state == State.NEEDS_INPUT || revocableMemoryContext.getBytes() == 0, "Cannot spill in state: %s", state); + return spillToDisk(); + } + + private ListenableFuture spillToDisk() { checkSuccess(spillInProgress, "spilling failed"); if (revocableMemoryContext.getBytes() == 0) { - // This must be stale revoke request - verify(pageIndex.getPositionCount() == 0 || state == State.HAS_OUTPUT); finishMemoryRevoke = () -> {}; return immediateFuture(null); } - verify(state == State.NEEDS_INPUT, "Cannot spill in %s state", state); - // TODO try pageIndex.compact(); before spilling, as in com.facebook.presto.operator.HashBuilderOperator.startMemoryRevoke if (!spiller.isPresent()) { diff --git a/presto-main/src/test/java/com/facebook/presto/operator/OperatorAssertion.java b/presto-main/src/test/java/com/facebook/presto/operator/OperatorAssertion.java index edca881047394..133ac6a4ee7c6 100644 --- a/presto-main/src/test/java/com/facebook/presto/operator/OperatorAssertion.java +++ b/presto-main/src/test/java/com/facebook/presto/operator/OperatorAssertion.java @@ -211,12 +211,34 @@ public static void assertOperatorEquals(OperatorFactory operatorFactory, List input, MaterializedResult expected) { - assertOperatorEquals(operatorFactory, driverContext, input, expected, false, ImmutableList.of()); + assertOperatorEquals(operatorFactory, driverContext, input, expected, true); + } + + public static void assertOperatorEquals( + OperatorFactory operatorFactory, + DriverContext driverContext, + List input, + MaterializedResult expected, + boolean revokeMemoryWhenAddingPages) + { + assertOperatorEquals(operatorFactory, driverContext, input, expected, false, ImmutableList.of(), revokeMemoryWhenAddingPages); } public static void assertOperatorEquals(OperatorFactory operatorFactory, DriverContext driverContext, List input, MaterializedResult expected, boolean hashEnabled, List hashChannels) { - List pages = toPages(operatorFactory, driverContext, input); + assertOperatorEquals(operatorFactory, driverContext, input, expected, hashEnabled, hashChannels, true); + } + + public static void assertOperatorEquals( + OperatorFactory operatorFactory, + DriverContext driverContext, + List input, + MaterializedResult expected, + boolean hashEnabled, + List hashChannels, + boolean revokeMemoryWhenAddingPages) + { + List pages = toPages(operatorFactory, driverContext, input, revokeMemoryWhenAddingPages); if (hashEnabled && !hashChannels.isEmpty()) { // Drop the hashChannel for all pages pages = dropChannel(pages, hashChannels); diff --git a/presto-main/src/test/java/com/facebook/presto/operator/TestOrderByOperator.java b/presto-main/src/test/java/com/facebook/presto/operator/TestOrderByOperator.java index e0867bf4494c6..74cd44555d1c6 100644 --- a/presto-main/src/test/java/com/facebook/presto/operator/TestOrderByOperator.java +++ b/presto-main/src/test/java/com/facebook/presto/operator/TestOrderByOperator.java @@ -19,6 +19,7 @@ import com.facebook.presto.spi.plan.PlanNodeId; import com.facebook.presto.sql.gen.OrderingCompiler; import com.facebook.presto.testing.MaterializedResult; +import com.facebook.presto.testing.TestingTaskContext; import com.google.common.collect.ImmutableList; import io.airlift.units.DataSize; import io.airlift.units.DataSize.Unit; @@ -33,6 +34,7 @@ import java.util.concurrent.ScheduledExecutorService; import static com.facebook.airlift.concurrent.Threads.daemonThreadsNamed; +import static com.facebook.airlift.testing.Assertions.assertGreaterThan; import static com.facebook.presto.RowPagesBuilder.rowPagesBuilder; import static com.facebook.presto.SessionTestUtils.TEST_SESSION; import static com.facebook.presto.common.block.SortOrder.ASC_NULLS_LAST; @@ -41,23 +43,32 @@ import static com.facebook.presto.common.type.DoubleType.DOUBLE; import static com.facebook.presto.common.type.VarcharType.VARCHAR; import static com.facebook.presto.operator.OperatorAssertion.assertOperatorEquals; +import static com.facebook.presto.operator.OperatorAssertion.toMaterializedResult; import static com.facebook.presto.operator.OperatorAssertion.toPages; import static com.facebook.presto.testing.MaterializedResult.resultBuilder; import static com.facebook.presto.testing.TestingTaskContext.createTaskContext; +import static io.airlift.units.DataSize.succinctBytes; +import static java.lang.String.format; import static java.util.concurrent.Executors.newCachedThreadPool; import static java.util.concurrent.Executors.newScheduledThreadPool; +import static org.testng.Assert.assertEquals; @Test(singleThreaded = true) public class TestOrderByOperator { private ExecutorService executor; private ScheduledExecutorService scheduledExecutor; - private DriverContext driverContext; + private DummySpillerFactory spillerFactory; @DataProvider public static Object[][] spillEnabled() { - return new Object[][] {{false}, {true}}; + return new Object[][] { + {false, false, 0}, + {true, false, 8}, + {true, true, 8}, + {true, false, 0}, + {true, true, 0}}; } @BeforeMethod @@ -65,9 +76,7 @@ public void setUp() { executor = newCachedThreadPool(daemonThreadsNamed("test-executor-%s")); scheduledExecutor = newScheduledThreadPool(2, daemonThreadsNamed("test-scheduledExecutor-%s")); - driverContext = createTaskContext(executor, scheduledExecutor, TEST_SESSION) - .addPipelineContext(0, true, true, false) - .addDriverContext(); + spillerFactory = new DummySpillerFactory(); } @AfterMethod @@ -75,10 +84,52 @@ public void tearDown() { executor.shutdownNow(); scheduledExecutor.shutdownNow(); + spillerFactory = null; + } + + @Test(dataProvider = "spillEnabled") + public void testMultipleOutputPages(boolean spillEnabled, boolean revokeMemoryWhenAddingPages, long memoryLimit) + { + // make operator produce multiple pages during finish phase + int numberOfRows = 80_000; + List input = rowPagesBuilder(BIGINT, DOUBLE) + .addSequencePage(numberOfRows, 0, 0) + .build(); + + OrderByOperatorFactory operatorFactory = new OrderByOperatorFactory( + 0, + new PlanNodeId("test"), + ImmutableList.of(BIGINT, DOUBLE), + ImmutableList.of(1), + 10, + ImmutableList.of(0), + ImmutableList.of(DESC_NULLS_LAST), + new PagesIndex.TestingFactory(false), + spillEnabled, + Optional.of(spillerFactory), + new OrderingCompiler()); + + DriverContext driverContext = createDriverContext(memoryLimit); + MaterializedResult.Builder expectedBuilder = resultBuilder(driverContext.getSession(), DOUBLE); + for (int i = 0; i < numberOfRows; i++) { + expectedBuilder.row((double) numberOfRows - i - 1); + } + MaterializedResult expected = expectedBuilder.build(); + + List pages = toPages(operatorFactory, driverContext, input, revokeMemoryWhenAddingPages); + assertGreaterThan(pages.size(), 1, "Expected more than one output page"); + + MaterializedResult actual = toMaterializedResult(driverContext.getSession(), expected.getTypes(), pages); + assertEquals(actual.getMaterializedRows(), expected.getMaterializedRows()); + + assertEquals( + spillEnabled, + spillerFactory.getSpillsCount() > 0, + format("Spill state mismatch. Expected spill: %s, spill count: %s", spillEnabled, spillerFactory.getSpillsCount())); } @Test(dataProvider = "spillEnabled") - public void testSingleFieldKey(boolean spillEnabled) + public void testSingleFieldKey(boolean spillEnabled, boolean revokeMemoryWhenAddingPages, long memoryLimit) { List input = rowPagesBuilder(BIGINT, DOUBLE) .row(1L, 0.1) @@ -98,9 +149,10 @@ public void testSingleFieldKey(boolean spillEnabled) ImmutableList.of(ASC_NULLS_LAST), new PagesIndex.TestingFactory(false), spillEnabled, - Optional.of(new DummySpillerFactory()), + Optional.of(spillerFactory), new OrderingCompiler()); + DriverContext driverContext = createDriverContext(memoryLimit); MaterializedResult expected = resultBuilder(driverContext.getSession(), DOUBLE) .row(-0.1) .row(0.1) @@ -108,11 +160,11 @@ public void testSingleFieldKey(boolean spillEnabled) .row(0.4) .build(); - assertOperatorEquals(operatorFactory, driverContext, input, expected); + assertOperatorEquals(operatorFactory, driverContext, input, expected, revokeMemoryWhenAddingPages); } @Test(dataProvider = "spillEnabled") - public void testMultiFieldKey(boolean spillEnabled) + public void testMultiFieldKey(boolean spillEnabled, boolean revokeMemoryWhenAddingPages, long memoryLimit) { List input = rowPagesBuilder(VARCHAR, BIGINT) .row("a", 1L) @@ -132,9 +184,10 @@ public void testMultiFieldKey(boolean spillEnabled) ImmutableList.of(ASC_NULLS_LAST, DESC_NULLS_LAST), new PagesIndex.TestingFactory(false), spillEnabled, - Optional.of(new DummySpillerFactory()), + Optional.of(spillerFactory), new OrderingCompiler()); + DriverContext driverContext = createDriverContext(memoryLimit); MaterializedResult expected = MaterializedResult.resultBuilder(driverContext.getSession(), VARCHAR, BIGINT) .row("a", 4L) .row("a", 1L) @@ -142,11 +195,11 @@ public void testMultiFieldKey(boolean spillEnabled) .row("b", 2L) .build(); - assertOperatorEquals(operatorFactory, driverContext, input, expected); + assertOperatorEquals(operatorFactory, driverContext, input, expected, revokeMemoryWhenAddingPages); } @Test(dataProvider = "spillEnabled") - public void testReverseOrder(boolean spillEnabled) + public void testReverseOrder(boolean spillEnabled, boolean revokeMemoryWhenAddingPages, long memoryLimit) { List input = rowPagesBuilder(BIGINT, DOUBLE) .row(1L, 0.1) @@ -166,9 +219,10 @@ public void testReverseOrder(boolean spillEnabled) ImmutableList.of(DESC_NULLS_LAST), new PagesIndex.TestingFactory(false), spillEnabled, - Optional.of(new DummySpillerFactory()), + Optional.of(spillerFactory), new OrderingCompiler()); + DriverContext driverContext = createDriverContext(memoryLimit); MaterializedResult expected = resultBuilder(driverContext.getSession(), BIGINT) .row(4L) .row(2L) @@ -176,7 +230,7 @@ public void testReverseOrder(boolean spillEnabled) .row(-1L) .build(); - assertOperatorEquals(operatorFactory, driverContext, input, expected); + assertOperatorEquals(operatorFactory, driverContext, input, expected, revokeMemoryWhenAddingPages); } @Test(expectedExceptions = ExceededMemoryLimitException.class, expectedExceptionsMessageRegExp = "Query exceeded per-node user memory limit of 10B.*") @@ -204,9 +258,18 @@ public void testMemoryLimit() ImmutableList.of(ASC_NULLS_LAST), new PagesIndex.TestingFactory(false), false, - Optional.of(new DummySpillerFactory()), + Optional.of(spillerFactory), new OrderingCompiler()); toPages(operatorFactory, driverContext, input); } + + private DriverContext createDriverContext(long memoryLimit) + { + return TestingTaskContext.builder(executor, scheduledExecutor, TEST_SESSION) + .setMemoryPoolSize(succinctBytes(memoryLimit)) + .build() + .addPipelineContext(0, true, true, false) + .addDriverContext(); + } } From 1dc65c617652f0fd9bd2cd101fbeafd4d63a097b Mon Sep 17 00:00:00 2001 From: Saksham Sachdev Date: Mon, 13 Jul 2020 15:37:44 -0700 Subject: [PATCH 09/10] Extract order by queries tests to separate class Cherry-pick of https://github.com/prestosql/presto/commit/94b4d0a42f8c86b95b6c3664c5cd83b6eab7297a Co-authored-by: Karol Sobczak --- .../TestHiveDistributedOrderByQueries.java | 28 ++ .../tests/AbstractTestOrderByQueries.java | 255 ++++++++++++++++++ .../presto/tests/AbstractTestQueries.java | 236 ---------------- .../presto/tests/TestOrderByQueries.java | 26 ++ .../tests/TestSpilledOrderByQueries.java | 24 ++ 5 files changed, 333 insertions(+), 236 deletions(-) create mode 100644 presto-hive/src/test/java/com/facebook/presto/hive/TestHiveDistributedOrderByQueries.java create mode 100644 presto-tests/src/main/java/com/facebook/presto/tests/AbstractTestOrderByQueries.java create mode 100644 presto-tests/src/test/java/com/facebook/presto/tests/TestOrderByQueries.java create mode 100644 presto-tests/src/test/java/com/facebook/presto/tests/TestSpilledOrderByQueries.java diff --git a/presto-hive/src/test/java/com/facebook/presto/hive/TestHiveDistributedOrderByQueries.java b/presto-hive/src/test/java/com/facebook/presto/hive/TestHiveDistributedOrderByQueries.java new file mode 100644 index 0000000000000..5a17181707c18 --- /dev/null +++ b/presto-hive/src/test/java/com/facebook/presto/hive/TestHiveDistributedOrderByQueries.java @@ -0,0 +1,28 @@ +/* + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package com.facebook.presto.hive; + +import com.facebook.presto.tests.AbstractTestOrderByQueries; + +import static com.facebook.presto.hive.HiveQueryRunner.createQueryRunner; +import static io.airlift.tpch.TpchTable.getTables; + +public class TestHiveDistributedOrderByQueries + extends AbstractTestOrderByQueries +{ + public TestHiveDistributedOrderByQueries() + { + super(() -> createQueryRunner(getTables())); + } +} diff --git a/presto-tests/src/main/java/com/facebook/presto/tests/AbstractTestOrderByQueries.java b/presto-tests/src/main/java/com/facebook/presto/tests/AbstractTestOrderByQueries.java new file mode 100644 index 0000000000000..0f0d64b0432f2 --- /dev/null +++ b/presto-tests/src/main/java/com/facebook/presto/tests/AbstractTestOrderByQueries.java @@ -0,0 +1,255 @@ +/* + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.facebook.presto.tests; + +import com.facebook.presto.Session; +import com.facebook.presto.testing.MaterializedResult; +import org.testng.annotations.Test; + +import static com.facebook.presto.SystemSessionProperties.DISTRIBUTED_SORT; +import static com.facebook.presto.common.type.IntegerType.INTEGER; +import static com.facebook.presto.testing.MaterializedResult.resultBuilder; +import static com.facebook.presto.testing.assertions.Assert.assertEquals; +import static com.facebook.presto.tests.QueryTemplate.parameter; +import static com.facebook.presto.tests.QueryTemplate.queryTemplate; + +public class AbstractTestOrderByQueries + extends AbstractTestQueryFramework +{ + public AbstractTestOrderByQueries(QueryRunnerSupplier supplier) + { + super(supplier); + } + + @Test + public void testOrderBy() + { + assertQueryOrdered("SELECT orderstatus FROM orders ORDER BY orderstatus"); + assertQueryOrdered("SELECT orderstatus FROM orders ORDER BY orderkey DESC"); + } + + @Test + public void testOrderByLimit() + { + assertQueryOrdered("SELECT custkey, orderstatus FROM orders ORDER BY orderkey DESC LIMIT 10"); + assertQueryOrdered("SELECT custkey, orderstatus FROM orders ORDER BY orderkey + 1 DESC LIMIT 10"); + assertQuery("SELECT custkey, totalprice FROM orders ORDER BY orderkey LIMIT 0"); + } + + @Test + public void testOrderByWithOutputColumnReference() + { + assertQueryOrdered("SELECT a*2 AS b FROM (VALUES -1, 0, 2) t(a) ORDER BY b*-1", "VALUES 4, 0, -2"); + assertQueryOrdered("SELECT a*2 AS b FROM (VALUES -1, 0, 2) t(a) ORDER BY b", "VALUES -2, 0, 4"); + assertQueryOrdered("SELECT a*-2 AS a FROM (VALUES -1, 0, 2) t(a) ORDER BY a*-1", "VALUES 2, 0, -4"); + assertQueryOrdered("SELECT a*-2 AS a FROM (VALUES -1, 0, 2) t(a) ORDER BY t.a*-1", "VALUES -4, 0, 2"); + assertQueryOrdered("SELECT a*-2 FROM (VALUES -1, 0, 2) t(a) ORDER BY a*-1", "VALUES -4, 0, 2"); + assertQueryOrdered("SELECT a*-2 FROM (VALUES -1, 0, 2) t(a) ORDER BY t.a*-1", "VALUES -4, 0, 2"); + assertQueryOrdered("SELECT a, a* -1 AS a FROM (VALUES -1, 0, 2) t(a) ORDER BY t.a", "VALUES (-1, 1), (0, 0), (2, -2)"); + assertQueryOrdered("SELECT a, a* -2 AS b FROM (VALUES -1, 0, 2) t(a) ORDER BY a + b", "VALUES (2, -4), (0, 0), (-1, 2)"); + assertQueryOrdered("SELECT a AS b, a* -2 AS a FROM (VALUES -1, 0, 2) t(a) ORDER BY a + b", "VALUES (2, -4), (0, 0), (-1, 2)"); + assertQueryOrdered("SELECT a* -2 AS a FROM (VALUES -1, 0, 2) t(a) ORDER BY a + t.a", "VALUES -4, 0, 2"); + assertQueryOrdered("SELECT k, SUM(a) a, SUM(b) a FROM (VALUES (1, 2, 3)) t(k, a, b) GROUP BY k ORDER BY k", "VALUES (1, 2, 3)"); + + // coercions + assertQueryOrdered("SELECT 1 x ORDER BY degrees(x)", "VALUES 1"); + assertQueryOrdered("SELECT a + 1 AS b FROM (VALUES 1, 2) t(a) ORDER BY -1.0 * b", "VALUES 3, 2"); + assertQueryOrdered("SELECT a AS b FROM (VALUES 1, 2) t(a) ORDER BY -1.0 * b", "VALUES 2, 1"); + assertQueryOrdered("SELECT a AS a FROM (VALUES 1, 2) t(a) ORDER BY -1.0 * a", "VALUES 2, 1"); + assertQueryOrdered("SELECT 1 x ORDER BY degrees(x)", "VALUES 1"); + + // groups + assertQueryOrdered("SELECT max(a+b), min(a+b) AS a FROM (values (1,2),(3,2),(1,5)) t(a,b) GROUP BY a ORDER BY max(t.a+t.b)", "VALUES (5, 5), (6, 3)"); + assertQueryOrdered("SELECT max(a+b), min(a+b) AS a FROM (values (1,2),(3,2),(1,5)) t(a,b) GROUP BY a ORDER BY max(t.a+t.b)*-0.1", "VALUES (6, 3), (5, 5)"); + assertQueryOrdered("SELECT max(a) FROM (values (1,2), (2,1)) t(a,b) GROUP BY b ORDER BY max(b*1.0)", "VALUES 2, 1"); + assertQueryOrdered("SELECT max(a) AS b FROM (values (1,2), (2,1)) t(a,b) GROUP BY b ORDER BY b", "VALUES 1, 2"); + assertQueryOrdered("SELECT max(a) FROM (values (1,2), (2,1)) t(a,b) GROUP BY b ORDER BY b*1.0", "VALUES 2, 1"); + assertQueryOrdered("SELECT max(a)*100 AS c FROM (values (1,2), (2,1)) t(a,b) GROUP BY b ORDER BY max(b) + c", "VALUES 100, 200"); + assertQueryOrdered("SELECT max(a) FROM (values (1,2), (2,1)) t(a,b) GROUP BY b ORDER BY b", "VALUES 2, 1"); + assertQueryOrdered("SELECT max(a) FROM (values (1,2), (2,1)) t(a,b) GROUP BY t.b ORDER BY t.b*1.0", "VALUES 2, 1"); + assertQueryOrdered("SELECT -(a+b) AS a, -(a+b) AS b, a+b FROM (values (41, 42), (-41, -42)) t(a,b) GROUP BY a+b ORDER BY a+b", "VALUES (-83, -83, 83), (83, 83, -83)"); + assertQueryOrdered("SELECT c.a FROM (SELECT CAST(ROW(-a.a) AS ROW(a BIGINT)) a FROM (VALUES (2), (1)) a(a) GROUP BY a.a ORDER BY a.a) t(c)", "VALUES -2, -1"); + assertQueryOrdered("SELECT -a AS a FROM (values (1,2),(3,2)) t(a,b) GROUP BY GROUPING SETS ((a), (a, b)) ORDER BY -a", "VALUES -1, -1, -3, -3"); + assertQueryOrdered("SELECT a AS foo FROM (values (1,2),(3,2)) t(a,b) GROUP BY GROUPING SETS ((a), (a, b)) HAVING b IS NOT NULL ORDER BY -a", "VALUES 3, 1"); + assertQueryOrdered("SELECT max(a) FROM (values (1,2),(3,2)) t(a,b) ORDER BY max(-a)", "VALUES 3"); + assertQueryFails("SELECT max(a) AS a FROM (values (1,2)) t(a,b) GROUP BY b ORDER BY max(a+b)", ".*Invalid reference to output projection attribute from ORDER BY aggregation"); + assertQueryOrdered("SELECT -a AS a, a AS b FROM (VALUES 1, 2) t(a) GROUP BY t.a ORDER BY a", "VALUES (-2, 2), (-1, 1)"); + assertQueryOrdered("SELECT -a AS a, a AS b FROM (VALUES 1, 2) t(a) GROUP BY t.a ORDER BY t.a", "VALUES (-1, 1), (-2, 2)"); + assertQueryOrdered("SELECT -a AS a, a AS b FROM (VALUES 1, 2) t(a) GROUP BY a ORDER BY t.a", "VALUES (-1, 1), (-2, 2)"); + assertQueryOrdered("SELECT -a AS a, a AS b FROM (VALUES 1, 2) t(a) GROUP BY a ORDER BY t.a+2*a", "VALUES (-2, 2), (-1, 1)"); + assertQueryOrdered("SELECT -a AS a, a AS b FROM (VALUES 1, 2) t(a) GROUP BY t.a ORDER BY t.a+2*a", "VALUES (-2, 2), (-1, 1)"); + + // lambdas + assertQueryOrdered("SELECT x AS y FROM (values (1,2), (2,3)) t(x, y) GROUP BY x ORDER BY apply(x, x -> -x) + 2*x", "VALUES 1, 2"); + assertQueryOrdered("SELECT -y AS x FROM (values (1,2), (2,3)) t(x, y) GROUP BY y ORDER BY apply(x, x -> -x)", "VALUES -2, -3"); + assertQueryOrdered("SELECT -y AS x FROM (values (1,2), (2,3)) t(x, y) GROUP BY y ORDER BY sum(apply(-y, x -> x * 1.0))", "VALUES -3, -2"); + + // distinct + assertQueryOrdered("SELECT DISTINCT -a AS b FROM (VALUES 1, 2) t(a) ORDER BY b", "VALUES -2, -1"); + assertQueryOrdered("SELECT DISTINCT -a AS b FROM (VALUES 1, 2) t(a) ORDER BY 1", "VALUES -2, -1"); + assertQueryOrdered("SELECT DISTINCT max(a) AS b FROM (values (1,2), (2,1)) t(a,b) GROUP BY b ORDER BY b", "VALUES 1, 2"); + assertQueryFails("SELECT DISTINCT -a AS b FROM (VALUES (1, 2), (3, 4)) t(a, c) ORDER BY c", ".*For SELECT DISTINCT, ORDER BY expressions must appear in select list"); + assertQueryFails("SELECT DISTINCT -a AS b FROM (VALUES (1, 2), (3, 4)) t(a, c) ORDER BY 2", ".*ORDER BY position 2 is not in select list"); + + // window + assertQueryOrdered("SELECT a FROM (VALUES 1, 2) t(a) ORDER BY -row_number() OVER ()", "VALUES 2, 1"); + assertQueryOrdered("SELECT -a AS a, first_value(-a) OVER (ORDER BY a ROWS 0 PRECEDING) AS b FROM (VALUES 1, 2) t(a) ORDER BY first_value(a) OVER (ORDER BY a ROWS 0 PRECEDING)", "VALUES (-2, -2), (-1, -1)"); + assertQueryOrdered("SELECT -a AS a FROM (VALUES 1, 2) t(a) ORDER BY first_value(a+t.a*2) OVER (ORDER BY a ROWS 0 PRECEDING)", "VALUES -1, -2"); + + assertQueryFails("SELECT a, a* -1 AS a FROM (VALUES -1, 0, 2) t(a) ORDER BY a", ".*'a' is ambiguous"); + } + + @Test + public void testOrderByWithAggregation() + { + assertQuery("" + + "SELECT x, sum(cast(x AS double))\n" + + "FROM (VALUES '1.0') t(x)\n" + + "GROUP BY x\n" + + "ORDER BY sum(cast(t.x AS double))", + "VALUES ('1.0', 1.0)"); + + queryTemplate("SELECT count(*) %output% FROM (SELECT substr(name,1,1) letter FROM nation) x GROUP BY %groupBy% ORDER BY %orderBy%") + .replaceAll( + parameter("output").of("", ", letter", ", letter AS y"), + parameter("groupBy").of("x.letter", "letter"), + parameter("orderBy").of("x.letter", "letter")) + .forEach(this::assertQueryOrdered); + } + + @Test + public void testOrderByLimitAll() + { + assertQuery("SELECT custkey, totalprice FROM orders ORDER BY orderkey LIMIT ALL", "SELECT custkey, totalprice FROM orders ORDER BY orderkey"); + } + + @Test + public void testOrderByMultipleFields() + { + assertQueryOrdered("SELECT custkey, orderstatus FROM orders ORDER BY custkey DESC, orderstatus"); + } + + @Test + public void testDuplicateColumnsInOrderByClause() + { + MaterializedResult actual = computeActual("SELECT * FROM (VALUES INTEGER '3', INTEGER '2', INTEGER '1') t(a) ORDER BY a ASC, a DESC"); + + MaterializedResult expected = resultBuilder(getSession(), INTEGER) + .row(1) + .row(2) + .row(3) + .build(); + + assertEquals(actual, expected); + } + + @Test + public void testOrderByWithNulls() + { + // nulls first + assertQueryOrdered("SELECT orderkey, custkey, orderstatus FROM orders ORDER BY nullif(orderkey, 3) ASC NULLS FIRST, custkey ASC"); + assertQueryOrdered("SELECT orderkey, custkey, orderstatus FROM orders ORDER BY nullif(orderkey, 3) DESC NULLS FIRST, custkey ASC"); + + // nulls last + assertQueryOrdered("SELECT orderkey, custkey, orderstatus FROM orders ORDER BY nullif(orderkey, 3) ASC NULLS LAST, custkey ASC"); + assertQueryOrdered("SELECT orderkey, custkey, orderstatus FROM orders ORDER BY nullif(orderkey, 3) DESC NULLS LAST, custkey ASC"); + + // assure that default is nulls last + assertQueryOrdered( + "SELECT orderkey, custkey, orderstatus FROM orders ORDER BY nullif(orderkey, 3) ASC, custkey ASC", + "SELECT orderkey, custkey, orderstatus FROM orders ORDER BY nullif(orderkey, 3) ASC NULLS LAST, custkey ASC"); + } + + @Test + public void testOrderByAlias() + { + assertQueryOrdered("SELECT orderstatus x FROM orders ORDER BY x ASC"); + } + + @Test + public void testOrderByAliasWithSameNameAsUnselectedColumn() + { + assertQueryOrdered("SELECT orderstatus orderdate FROM orders ORDER BY orderdate ASC"); + } + + @Test + public void testOrderByOrdinal() + { + assertQueryOrdered("SELECT orderstatus, orderdate FROM orders ORDER BY 2, 1"); + } + + @Test + public void testOrderByOrdinalWithWildcard() + { + assertQueryOrdered("SELECT * FROM orders ORDER BY 1"); + } + + @Test + public void testOrderByWithSimilarExpressions() + { + assertQuery( + "WITH t AS (SELECT 1 x, 2 y) SELECT x, y FROM t ORDER BY x, y", + "SELECT 1, 2"); + assertQuery( + "WITH t AS (SELECT 1 x, 2 y) SELECT x, y FROM t ORDER BY x, y LIMIT 1", + "SELECT 1, 2"); + assertQuery( + "WITH t AS (SELECT 1 x, 1 y) SELECT x, y FROM t ORDER BY x, y LIMIT 1", + "SELECT 1, 1"); + assertQuery( + "WITH t AS (SELECT orderkey x, orderkey y FROM orders) SELECT x, y FROM t ORDER BY x, y LIMIT 1", + "SELECT 1, 1"); + assertQuery( + "WITH t AS (SELECT orderkey x, orderkey y FROM orders) SELECT x, y FROM t ORDER BY x, y DESC LIMIT 1", + "SELECT 1, 1"); + assertQuery( + "WITH t AS (SELECT orderkey x, totalprice y, orderkey z FROM orders) SELECT x, y, z FROM t ORDER BY x, y, z LIMIT 1", + "SELECT 1, 172799.49, 1"); + } + + @Test + public void testOrderByUnderManyProjections() + { + assertQuery("SELECT nationkey, arbitrary_column + arbitrary_column " + + "FROM " + + "( " + + " SELECT nationkey, COALESCE(arbitrary_column, 0) arbitrary_column " + + " FROM ( " + + " SELECT nationkey, 1 arbitrary_column " + + " FROM nation " + + " ORDER BY 1 ASC))"); + } + + @Test + public void testUndistributedOrderBy() + { + Session undistributedOrderBy = Session.builder(getSession()) + .setSystemProperty(DISTRIBUTED_SORT, "false") + .build(); + assertQueryOrdered(undistributedOrderBy, "SELECT orderstatus FROM orders ORDER BY orderstatus"); + } + + @Test + public void testOrderLimitCompaction() + { + assertQueryOrdered("SELECT * FROM (SELECT * FROM orders ORDER BY orderkey) LIMIT 10"); + } + + @Test + public void testCaseInsensitiveOutputAliasInOrderBy() + { + assertQueryOrdered("SELECT orderkey X FROM orders ORDER BY x"); + } +} diff --git a/presto-tests/src/main/java/com/facebook/presto/tests/AbstractTestQueries.java b/presto-tests/src/main/java/com/facebook/presto/tests/AbstractTestQueries.java index 66193e28abfc7..33dd98e84b9d7 100644 --- a/presto-tests/src/main/java/com/facebook/presto/tests/AbstractTestQueries.java +++ b/presto-tests/src/main/java/com/facebook/presto/tests/AbstractTestQueries.java @@ -53,7 +53,6 @@ import java.util.Set; import java.util.stream.IntStream; -import static com.facebook.presto.SystemSessionProperties.DISTRIBUTED_SORT; import static com.facebook.presto.SystemSessionProperties.JOIN_DISTRIBUTION_TYPE; import static com.facebook.presto.SystemSessionProperties.JOIN_REORDERING_STRATEGY; import static com.facebook.presto.SystemSessionProperties.OPTIMIZE_NULLS_IN_JOINS; @@ -950,99 +949,6 @@ public void testDistinctWithOrderByNotInSelect() "line 1:1: For SELECT DISTINCT, ORDER BY expressions must appear in select list"); } - @Test - public void testOrderByLimit() - { - assertQueryOrdered("SELECT custkey, orderstatus FROM orders ORDER BY orderkey DESC LIMIT 10"); - } - - @Test - public void testOrderByExpressionWithLimit() - { - assertQueryOrdered("SELECT custkey, orderstatus FROM orders ORDER BY orderkey + 1 DESC LIMIT 10"); - } - - @Test - public void testOrderByWithOutputColumnReference() - { - assertQueryOrdered("SELECT a*2 AS b FROM (VALUES -1, 0, 2) t(a) ORDER BY b*-1", "VALUES 4, 0, -2"); - assertQueryOrdered("SELECT a*2 AS b FROM (VALUES -1, 0, 2) t(a) ORDER BY b", "VALUES -2, 0, 4"); - assertQueryOrdered("SELECT a*-2 AS a FROM (VALUES -1, 0, 2) t(a) ORDER BY a*-1", "VALUES 2, 0, -4"); - assertQueryOrdered("SELECT a*-2 AS a FROM (VALUES -1, 0, 2) t(a) ORDER BY t.a*-1", "VALUES -4, 0, 2"); - assertQueryOrdered("SELECT a*-2 FROM (VALUES -1, 0, 2) t(a) ORDER BY a*-1", "VALUES -4, 0, 2"); - assertQueryOrdered("SELECT a*-2 FROM (VALUES -1, 0, 2) t(a) ORDER BY t.a*-1", "VALUES -4, 0, 2"); - assertQueryOrdered("SELECT a, a* -1 AS a FROM (VALUES -1, 0, 2) t(a) ORDER BY t.a", "VALUES (-1, 1), (0, 0), (2, -2)"); - assertQueryOrdered("SELECT a, a* -2 AS b FROM (VALUES -1, 0, 2) t(a) ORDER BY a + b", "VALUES (2, -4), (0, 0), (-1, 2)"); - assertQueryOrdered("SELECT a AS b, a* -2 AS a FROM (VALUES -1, 0, 2) t(a) ORDER BY a + b", "VALUES (2, -4), (0, 0), (-1, 2)"); - assertQueryOrdered("SELECT a* -2 AS a FROM (VALUES -1, 0, 2) t(a) ORDER BY a + t.a", "VALUES -4, 0, 2"); - assertQueryOrdered("SELECT k, SUM(a) a, SUM(b) a FROM (VALUES (1, 2, 3)) t(k, a, b) GROUP BY k ORDER BY k", "VALUES (1, 2, 3)"); - - // coercions - assertQueryOrdered("SELECT 1 x ORDER BY degrees(x)", "VALUES 1"); - assertQueryOrdered("SELECT a + 1 AS b FROM (VALUES 1, 2) t(a) ORDER BY -1.0 * b", "VALUES 3, 2"); - assertQueryOrdered("SELECT a AS b FROM (VALUES 1, 2) t(a) ORDER BY -1.0 * b", "VALUES 2, 1"); - assertQueryOrdered("SELECT a AS a FROM (VALUES 1, 2) t(a) ORDER BY -1.0 * a", "VALUES 2, 1"); - assertQueryOrdered("SELECT 1 x ORDER BY degrees(x)", "VALUES 1"); - - // groups - assertQueryOrdered("SELECT max(a+b), min(a+b) AS a FROM (values (1,2),(3,2),(1,5)) t(a,b) GROUP BY a ORDER BY max(t.a+t.b)", "VALUES (5, 5), (6, 3)"); - assertQueryOrdered("SELECT max(a+b), min(a+b) AS a FROM (values (1,2),(3,2),(1,5)) t(a,b) GROUP BY a ORDER BY max(t.a+t.b)*-0.1", "VALUES (6, 3), (5, 5)"); - assertQueryOrdered("SELECT max(a) FROM (values (1,2), (2,1)) t(a,b) GROUP BY b ORDER BY max(b*1.0)", "VALUES 2, 1"); - assertQueryOrdered("SELECT max(a) AS b FROM (values (1,2), (2,1)) t(a,b) GROUP BY b ORDER BY b", "VALUES 1, 2"); - assertQueryOrdered("SELECT max(a) FROM (values (1,2), (2,1)) t(a,b) GROUP BY b ORDER BY b*1.0", "VALUES 2, 1"); - assertQueryOrdered("SELECT max(a)*100 AS c FROM (values (1,2), (2,1)) t(a,b) GROUP BY b ORDER BY max(b) + c", "VALUES 100, 200"); - assertQueryOrdered("SELECT max(a) FROM (values (1,2), (2,1)) t(a,b) GROUP BY b ORDER BY b", "VALUES 2, 1"); - assertQueryOrdered("SELECT max(a) FROM (values (1,2), (2,1)) t(a,b) GROUP BY t.b ORDER BY t.b*1.0", "VALUES 2, 1"); - assertQueryOrdered("SELECT -(a+b) AS a, -(a+b) AS b, a+b FROM (values (41, 42), (-41, -42)) t(a,b) GROUP BY a+b ORDER BY a+b", "VALUES (-83, -83, 83), (83, 83, -83)"); - assertQueryOrdered("SELECT c.a FROM (SELECT CAST(ROW(-a.a) AS ROW(a BIGINT)) a FROM (VALUES (2), (1)) a(a) GROUP BY a.a ORDER BY a.a) t(c)", "VALUES -2, -1"); - assertQueryOrdered("SELECT -a AS a FROM (values (1,2),(3,2)) t(a,b) GROUP BY GROUPING SETS ((a), (a, b)) ORDER BY -a", "VALUES -1, -1, -3, -3"); - assertQueryOrdered("SELECT a AS foo FROM (values (1,2),(3,2)) t(a,b) GROUP BY GROUPING SETS ((a), (a, b)) HAVING b IS NOT NULL ORDER BY -a", "VALUES 3, 1"); - assertQueryOrdered("SELECT max(a) FROM (values (1,2),(3,2)) t(a,b) ORDER BY max(-a)", "VALUES 3"); - assertQueryFails("SELECT max(a) AS a FROM (values (1,2)) t(a,b) GROUP BY b ORDER BY max(a+b)", ".*Invalid reference to output projection attribute from ORDER BY aggregation"); - assertQueryOrdered("SELECT -a AS a, a AS b FROM (VALUES 1, 2) t(a) GROUP BY t.a ORDER BY a", "VALUES (-2, 2), (-1, 1)"); - assertQueryOrdered("SELECT -a AS a, a AS b FROM (VALUES 1, 2) t(a) GROUP BY t.a ORDER BY t.a", "VALUES (-1, 1), (-2, 2)"); - assertQueryOrdered("SELECT -a AS a, a AS b FROM (VALUES 1, 2) t(a) GROUP BY a ORDER BY t.a", "VALUES (-1, 1), (-2, 2)"); - assertQueryOrdered("SELECT -a AS a, a AS b FROM (VALUES 1, 2) t(a) GROUP BY a ORDER BY t.a+2*a", "VALUES (-2, 2), (-1, 1)"); - assertQueryOrdered("SELECT -a AS a, a AS b FROM (VALUES 1, 2) t(a) GROUP BY t.a ORDER BY t.a+2*a", "VALUES (-2, 2), (-1, 1)"); - - // lambdas - assertQueryOrdered("SELECT x AS y FROM (values (1,2), (2,3)) t(x, y) GROUP BY x ORDER BY apply(x, x -> -x) + 2*x", "VALUES 1, 2"); - assertQueryOrdered("SELECT -y AS x FROM (values (1,2), (2,3)) t(x, y) GROUP BY y ORDER BY apply(x, x -> -x)", "VALUES -2, -3"); - assertQueryOrdered("SELECT -y AS x FROM (values (1,2), (2,3)) t(x, y) GROUP BY y ORDER BY sum(apply(-y, x -> x * 1.0))", "VALUES -3, -2"); - - // distinct - assertQueryOrdered("SELECT DISTINCT -a AS b FROM (VALUES 1, 2) t(a) ORDER BY b", "VALUES -2, -1"); - assertQueryOrdered("SELECT DISTINCT -a AS b FROM (VALUES 1, 2) t(a) ORDER BY 1", "VALUES -2, -1"); - assertQueryOrdered("SELECT DISTINCT max(a) AS b FROM (values (1,2), (2,1)) t(a,b) GROUP BY b ORDER BY b", "VALUES 1, 2"); - assertQueryFails("SELECT DISTINCT -a AS b FROM (VALUES (1, 2), (3, 4)) t(a, c) ORDER BY c", ".*For SELECT DISTINCT, ORDER BY expressions must appear in select list"); - assertQueryFails("SELECT DISTINCT -a AS b FROM (VALUES (1, 2), (3, 4)) t(a, c) ORDER BY 2", ".*ORDER BY position 2 is not in select list"); - - // window - assertQueryOrdered("SELECT a FROM (VALUES 1, 2) t(a) ORDER BY -row_number() OVER ()", "VALUES 2, 1"); - assertQueryOrdered("SELECT -a AS a, first_value(-a) OVER (ORDER BY a ROWS 0 PRECEDING) AS b FROM (VALUES 1, 2) t(a) ORDER BY first_value(a) OVER (ORDER BY a ROWS 0 PRECEDING)", "VALUES (-2, -2), (-1, -1)"); - assertQueryOrdered("SELECT -a AS a FROM (VALUES 1, 2) t(a) ORDER BY first_value(a+t.a*2) OVER (ORDER BY a ROWS 0 PRECEDING)", "VALUES -1, -2"); - - assertQueryFails("SELECT a, a* -1 AS a FROM (VALUES -1, 0, 2) t(a) ORDER BY a", ".*'a' is ambiguous"); - } - - @Test - public void testOrderByWithAggregation() - { - assertQuery("" + - "SELECT x, sum(cast(x AS double))\n" + - "FROM (VALUES '1.0') t(x)\n" + - "GROUP BY x\n" + - "ORDER BY sum(cast(t.x AS double))", - "VALUES ('1.0', 1.0)"); - - queryTemplate("SELECT count(*) %output% FROM (SELECT substr(name,1,1) letter FROM nation) x GROUP BY %groupBy% ORDER BY %orderBy%") - .replaceAll( - parameter("output").of("", ", letter", ", letter AS y"), - parameter("groupBy").of("x.letter", "letter"), - parameter("orderBy").of("x.letter", "letter")) - .forEach(this::assertQueryOrdered); - } - @Test public void testGroupByOrderByLimit() { @@ -1061,18 +967,6 @@ public void testLimitAll() assertQuery("SELECT custkey, totalprice FROM orders LIMIT ALL", "SELECT custkey, totalprice FROM orders"); } - @Test - public void testOrderByLimitZero() - { - assertQuery("SELECT custkey, totalprice FROM orders ORDER BY orderkey LIMIT 0"); - } - - @Test - public void testOrderByLimitAll() - { - assertQuery("SELECT custkey, totalprice FROM orders ORDER BY orderkey LIMIT ALL", "SELECT custkey, totalprice FROM orders ORDER BY orderkey"); - } - @Test public void testRepeatedAggregations() { @@ -3013,124 +2907,6 @@ public void testJoinWithStatefulFilterFunction() "VALUES (1, null), (2, 2), (null, 3)"); } - @Test - public void testOrderBy() - { - assertQueryOrdered("SELECT orderstatus FROM orders ORDER BY orderstatus"); - } - - @Test - public void testOrderBy2() - { - assertQueryOrdered("SELECT orderstatus FROM orders ORDER BY orderkey DESC"); - } - - @Test - public void testOrderByMultipleFields() - { - assertQueryOrdered("SELECT custkey, orderstatus FROM orders ORDER BY custkey DESC, orderstatus"); - } - - @Test - public void testDuplicateColumnsInOrderByClause() - { - MaterializedResult actual = computeActual("SELECT * FROM (VALUES INTEGER '3', INTEGER '2', INTEGER '1') t(a) ORDER BY a ASC, a DESC"); - - MaterializedResult expected = resultBuilder(getSession(), INTEGER) - .row(1) - .row(2) - .row(3) - .build(); - - assertEquals(actual, expected); - } - - @Test - public void testOrderByWithNulls() - { - // nulls first - assertQueryOrdered("SELECT orderkey, custkey, orderstatus FROM orders ORDER BY nullif(orderkey, 3) ASC NULLS FIRST, custkey ASC"); - assertQueryOrdered("SELECT orderkey, custkey, orderstatus FROM orders ORDER BY nullif(orderkey, 3) DESC NULLS FIRST, custkey ASC"); - - // nulls last - assertQueryOrdered("SELECT orderkey, custkey, orderstatus FROM orders ORDER BY nullif(orderkey, 3) ASC NULLS LAST, custkey ASC"); - assertQueryOrdered("SELECT orderkey, custkey, orderstatus FROM orders ORDER BY nullif(orderkey, 3) DESC NULLS LAST, custkey ASC"); - - // assure that default is nulls last - assertQueryOrdered( - "SELECT orderkey, custkey, orderstatus FROM orders ORDER BY nullif(orderkey, 3) ASC, custkey ASC", - "SELECT orderkey, custkey, orderstatus FROM orders ORDER BY nullif(orderkey, 3) ASC NULLS LAST, custkey ASC"); - } - - @Test - public void testOrderByAlias() - { - assertQueryOrdered("SELECT orderstatus x FROM orders ORDER BY x ASC"); - } - - @Test - public void testOrderByAliasWithSameNameAsUnselectedColumn() - { - assertQueryOrdered("SELECT orderstatus orderdate FROM orders ORDER BY orderdate ASC"); - } - - @Test - public void testOrderByOrdinal() - { - assertQueryOrdered("SELECT orderstatus, orderdate FROM orders ORDER BY 2, 1"); - } - - @Test - public void testOrderByOrdinalWithWildcard() - { - assertQueryOrdered("SELECT * FROM orders ORDER BY 1"); - } - - @Test - public void testOrderByWithSimilarExpressions() - { - assertQuery( - "WITH t AS (SELECT 1 x, 2 y) SELECT x, y FROM t ORDER BY x, y", - "SELECT 1, 2"); - assertQuery( - "WITH t AS (SELECT 1 x, 2 y) SELECT x, y FROM t ORDER BY x, y LIMIT 1", - "SELECT 1, 2"); - assertQuery( - "WITH t AS (SELECT 1 x, 1 y) SELECT x, y FROM t ORDER BY x, y LIMIT 1", - "SELECT 1, 1"); - assertQuery( - "WITH t AS (SELECT orderkey x, orderkey y FROM orders) SELECT x, y FROM t ORDER BY x, y LIMIT 1", - "SELECT 1, 1"); - assertQuery( - "WITH t AS (SELECT orderkey x, orderkey y FROM orders) SELECT x, y FROM t ORDER BY x, y DESC LIMIT 1", - "SELECT 1, 1"); - assertQuery( - "WITH t AS (SELECT orderkey x, totalprice y, orderkey z FROM orders) SELECT x, y, z FROM t ORDER BY x, y, z LIMIT 1", - "SELECT 1, 172799.49, 1"); - } - - @Test - public void testOrderByUnderManyProjections() - { - assertQuery("SELECT nationkey, arbitrary_column + arbitrary_column " + - "FROM " + - "( " + - " SELECT nationkey, COALESCE(arbitrary_column, 0) arbitrary_column " + - " FROM ( " + - " SELECT nationkey, 1 arbitrary_column " + - " FROM nation " + - " ORDER BY 1 ASC))"); - } - - @Test - public void testUndistributedOrderBy() - { - Session undistributedOrderBy = Session.builder(getSession()) - .setSystemProperty(DISTRIBUTED_SORT, "false") - .build(); - assertQueryOrdered(undistributedOrderBy, "SELECT orderstatus FROM orders ORDER BY orderstatus"); - } - @Test public void testChecksum() { @@ -4479,12 +4255,6 @@ public void testWildcardFromSubquery() assertQuery("SELECT * FROM (SELECT orderkey X FROM orders)"); } - @Test - public void testCaseInsensitiveOutputAliasInOrderBy() - { - assertQueryOrdered("SELECT orderkey X FROM orders ORDER BY x"); - } - @Test public void testCaseInsensitiveAttribute() { @@ -5469,12 +5239,6 @@ public void testLimitPushDown() assertContains(all, actual); } - @Test - public void testOrderLimitCompaction() - { - assertQueryOrdered("SELECT * FROM (SELECT * FROM orders ORDER BY orderkey) LIMIT 10"); - } - @Test public void testUnaliasSymbolReferencesWithUnion() { diff --git a/presto-tests/src/test/java/com/facebook/presto/tests/TestOrderByQueries.java b/presto-tests/src/test/java/com/facebook/presto/tests/TestOrderByQueries.java new file mode 100644 index 0000000000000..9b4d3b1629afd --- /dev/null +++ b/presto-tests/src/test/java/com/facebook/presto/tests/TestOrderByQueries.java @@ -0,0 +1,26 @@ +/* + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.facebook.presto.tests; + +import com.facebook.presto.tests.tpch.TpchQueryRunnerBuilder; + +public class TestOrderByQueries + extends AbstractTestOrderByQueries +{ + public TestOrderByQueries() + { + super(() -> TpchQueryRunnerBuilder.builder().build()); + } +} diff --git a/presto-tests/src/test/java/com/facebook/presto/tests/TestSpilledOrderByQueries.java b/presto-tests/src/test/java/com/facebook/presto/tests/TestSpilledOrderByQueries.java new file mode 100644 index 0000000000000..01b6c3eee37e5 --- /dev/null +++ b/presto-tests/src/test/java/com/facebook/presto/tests/TestSpilledOrderByQueries.java @@ -0,0 +1,24 @@ +/* + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.facebook.presto.tests; + +public class TestSpilledOrderByQueries + extends AbstractTestOrderByQueries +{ + public TestSpilledOrderByQueries() + { + super(TestDistributedSpilledQueries::createQueryRunner); + } +} From 13db5e5b137e06d6bee7081b8bd760b02040381e Mon Sep 17 00:00:00 2001 From: Saksham Sachdev Date: Wed, 22 Jul 2020 14:41:57 -0700 Subject: [PATCH 10/10] Close spiller on Operator#close for ORDER BY spilling --- .../main/java/com/facebook/presto/operator/OrderByOperator.java | 1 + 1 file changed, 1 insertion(+) diff --git a/presto-main/src/main/java/com/facebook/presto/operator/OrderByOperator.java b/presto-main/src/main/java/com/facebook/presto/operator/OrderByOperator.java index 4b16e73bd97d7..c6419288f7e22 100644 --- a/presto-main/src/main/java/com/facebook/presto/operator/OrderByOperator.java +++ b/presto-main/src/main/java/com/facebook/presto/operator/OrderByOperator.java @@ -380,5 +380,6 @@ public void close() { pageIndex.clear(); sortedPages = null; + spiller.ifPresent(Spiller::close); } }