Skip to content

Commit 9916172

Browse files
davve14davve14
committed
fix(native): move optimize-hash-generation to Java-worker-specific property
 Move optimizer.optimize-hash-generation from system-wide property to JavaWorkerSessionPropertyProvider to ensure it's automatically hidden and disabled for native execution. Changes: - Move OPTIMIZE_HASH_GENERATION from SystemSessionProperties to JavaWorkerSessionPropertyProvider - Update HashGenerationOptimizer to read from JavaWorkerSessionPropertyProvider - Add conditional in PlanOptimizers to skip HashGenerationOptimizer for native execution - Update LocalQueryRunner.getPlanOptimizers to accept both FeaturesConfig and JavaFeaturesConfig - Update HashBuildAndJoinBenchmark to use new property location - Update tests including expected plan for tests running on native engine - Override createFeaturesConfig in AbstractTestAggregationsNative.java to not use optimize_hash_generation in native tests
1 parent 5efa463 commit 9916172

File tree

15 files changed

+60
-64
lines changed

15 files changed

+60
-64
lines changed

‎presto-benchmark/src/main/java/com/facebook/presto/benchmark/BenchmarkSuite.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,7 @@
2626
import java.util.List;
2727
import java.util.Map;
2828

29-
import static com.facebook.presto.SystemSessionProperties.OPTIMIZE_HASH_GENERATION;
29+
import static com.facebook.presto.sessionpropertyproviders.JavaWorkerSessionPropertyProvider.OPTIMIZE_HASH_GENERATION;
3030
import static java.util.Objects.requireNonNull;
3131

3232
public class BenchmarkSuite

‎presto-benchmark/src/main/java/com/facebook/presto/benchmark/HashBuildAndJoinBenchmark.java‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,6 @@
1414
package com.facebook.presto.benchmark;
1515

1616
import com.facebook.presto.Session;
17-
import com.facebook.presto.SystemSessionProperties;
1817
import com.facebook.presto.common.type.Type;
1918
import com.facebook.presto.operator.Driver;
2019
import com.facebook.presto.operator.DriverFactory;
@@ -25,6 +24,7 @@
2524
import com.facebook.presto.operator.PagesIndex;
2625
import com.facebook.presto.operator.PartitionedLookupSourceFactory;
2726
import com.facebook.presto.operator.TaskContext;
27+
import com.facebook.presto.sessionpropertyproviders.JavaWorkerSessionPropertyProvider;
2828
import com.facebook.presto.spi.plan.PlanNodeId;
2929
import com.facebook.presto.spiller.SingleStreamSpillerFactory;
3030
import com.facebook.presto.testing.LocalQueryRunner;
@@ -64,7 +64,7 @@ public HashBuildAndJoinBenchmark(Session session, LocalQueryRunner localQueryRun
6464

6565
private static boolean isHashEnabled(Session session)
6666
{
67-
return SystemSessionProperties.isOptimizeHashGenerationEnabled(session);
67+
return JavaWorkerSessionPropertyProvider.isOptimizeHashGenerationEnabled(session);
6868
}
6969

7070
/*

‎presto-main-base/src/main/java/com/facebook/presto/SystemSessionProperties.java‎

Lines changed: 0 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -89,7 +89,6 @@
8989

9090
public final class SystemSessionProperties
9191
{
92-
public static final String OPTIMIZE_HASH_GENERATION = "optimize_hash_generation";
9392
public static final String JOIN_DISTRIBUTION_TYPE = "join_distribution_type";
9493
public static final String JOIN_MAX_BROADCAST_TABLE_SIZE = "join_max_broadcast_table_size";
9594
public static final String RETRY_QUERY_WITH_HISTORY_BASED_OPTIMIZATION = "retry_query_with_history_based_optimization";
@@ -402,11 +401,6 @@ public SystemSessionProperties(
402401
"Policy used for scheduling query tasks",
403402
queryManagerConfig.getQueryExecutionPolicy(),
404403
false),
405-
booleanProperty(
406-
OPTIMIZE_HASH_GENERATION,
407-
"Compute hash codes for distribution, joins, and aggregations early in query plan",
408-
featuresConfig.isOptimizeHashGeneration(),
409-
false),
410404
booleanProperty(
411405
DISTRIBUTED_JOIN,
412406
"(DEPRECATED) Use a distributed join instead of a broadcast join. If this is set, join_distribution_type is ignored.",
@@ -2049,11 +2043,6 @@ public static String getExecutionPolicy(Session session)
20492043
return session.getSystemProperty(EXECUTION_POLICY, String.class);
20502044
}
20512045

2052-
public static boolean isOptimizeHashGenerationEnabled(Session session)
2053-
{
2054-
return session.getSystemProperty(OPTIMIZE_HASH_GENERATION, Boolean.class);
2055-
}
2056-
20572046
public static JoinDistributionType getJoinDistributionType(Session session)
20582047
{
20592048
// distributed_join takes precedence until we remove it

‎presto-main-base/src/main/java/com/facebook/presto/sessionpropertyproviders/JavaWorkerSessionPropertyProvider.java‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,7 @@
3434
public class JavaWorkerSessionPropertyProvider
3535
implements WorkerSessionPropertyProvider
3636
{
37+
public static final String OPTIMIZE_HASH_GENERATION = "optimize_hash_generation";
3738
public static final String AGGREGATION_SPILL_ENABLED = "aggregation_spill_enabled";
3839
public static final String TOPN_SPILL_ENABLED = "topn_spill_enabled";
3940
public static final String DISTINCT_AGGREGATION_SPILL_ENABLED = "distinct_aggregation_spill_enabled";
@@ -53,6 +54,11 @@ public JavaWorkerSessionPropertyProvider(FeaturesConfig featuresConfig, JavaFeat
5354
{
5455
boolean nativeExecution = requireNonNull(featuresConfig, "featuresConfig is null").isNativeExecutionEnabled();
5556
sessionProperties = ImmutableList.of(
57+
booleanProperty(
58+
OPTIMIZE_HASH_GENERATION,
59+
"Compute hash codes for distribution, joins, and aggregations early in query plan",
60+
featuresConfig.isOptimizeHashGeneration(),
61+
nativeExecution),
5662
booleanProperty(
5763
TOPN_SPILL_ENABLED,
5864
"Enable topN spilling if spill_enabled",
@@ -137,6 +143,11 @@ public List<PropertyMetadata<?>> getSessionProperties()
137143
return sessionProperties;
138144
}
139145

146+
public static boolean isOptimizeHashGenerationEnabled(Session session)
147+
{
148+
return session.getSystemProperty(OPTIMIZE_HASH_GENERATION, Boolean.class);
149+
}
150+
140151
public static boolean isTopNSpillEnabled(Session session)
141152
{
142153
return session.getSystemProperty(TOPN_SPILL_ENABLED, Boolean.class) && isSpillEnabled(session);

‎presto-main-base/src/main/java/com/facebook/presto/sql/planner/PlanOptimizers.java‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1012,7 +1012,9 @@ public PlanOptimizers(
10121012
// DO NOT add optimizers that change the plan shape (computations) after this point
10131013

10141014
// Precomputed hashes - this assumes that partitioning will not change
1015-
builder.add(new HashGenerationOptimizer(metadata.getFunctionAndTypeManager()));
1015+
if (!featuresConfig.isNativeExecutionEnabled()) {
1016+
builder.add(new HashGenerationOptimizer(metadata.getFunctionAndTypeManager()));
1017+
}
10161018
builder.add(new IterativeOptimizer(
10171019
metadata,
10181020
ruleStats,

‎presto-main-base/src/main/java/com/facebook/presto/sql/planner/optimizations/HashGenerationOptimizer.java‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,6 @@
1414
package com.facebook.presto.sql.planner.optimizations;
1515

1616
import com.facebook.presto.Session;
17-
import com.facebook.presto.SystemSessionProperties;
1817
import com.facebook.presto.metadata.FunctionAndTypeManager;
1918
import com.facebook.presto.spi.VariableAllocator;
2019
import com.facebook.presto.spi.WarningCollector;
@@ -71,6 +70,7 @@
7170

7271
import static com.facebook.presto.SystemSessionProperties.skipHashGenerationForJoinWithTableScanInput;
7372
import static com.facebook.presto.common.type.BigintType.BIGINT;
73+
import static com.facebook.presto.sessionpropertyproviders.JavaWorkerSessionPropertyProvider.isOptimizeHashGenerationEnabled;
7474
import static com.facebook.presto.spi.plan.JoinType.INNER;
7575
import static com.facebook.presto.spi.plan.JoinType.LEFT;
7676
import static com.facebook.presto.spi.plan.JoinType.RIGHT;
@@ -114,7 +114,7 @@ public void setEnabledForTesting(boolean isSet)
114114
@Override
115115
public boolean isEnabled(Session session)
116116
{
117-
return isEnabledForTesting || SystemSessionProperties.isOptimizeHashGenerationEnabled(session);
117+
return isEnabledForTesting || isOptimizeHashGenerationEnabled(session);
118118
}
119119

120120
@Override

‎presto-main-base/src/test/java/com/facebook/presto/sql/planner/TestLogicalPlanner.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -68,7 +68,6 @@
6868
import static com.facebook.presto.SystemSessionProperties.MAX_LEAF_NODES_IN_PLAN;
6969
import static com.facebook.presto.SystemSessionProperties.NATIVE_EXECUTION_ENABLED;
7070
import static com.facebook.presto.SystemSessionProperties.OFFSET_CLAUSE_ENABLED;
71-
import static com.facebook.presto.SystemSessionProperties.OPTIMIZE_HASH_GENERATION;
7271
import static com.facebook.presto.SystemSessionProperties.PREFER_SORT_MERGE_JOIN;
7372
import static com.facebook.presto.SystemSessionProperties.PUSH_REMOTE_EXCHANGE_THROUGH_GROUP_ID;
7473
import static com.facebook.presto.SystemSessionProperties.REMOVE_CROSS_JOIN_WITH_CONSTANT_SINGLE_ROW_INPUT;
@@ -79,6 +78,7 @@
7978
import static com.facebook.presto.common.predicate.Domain.singleValue;
8079
import static com.facebook.presto.common.type.BigintType.BIGINT;
8180
import static com.facebook.presto.common.type.VarcharType.createVarcharType;
81+
import static com.facebook.presto.sessionpropertyproviders.JavaWorkerSessionPropertyProvider.OPTIMIZE_HASH_GENERATION;
8282
import static com.facebook.presto.spi.StandardErrorCode.INVALID_LIMIT_CLAUSE;
8383
import static com.facebook.presto.spi.plan.AggregationNode.Step.FINAL;
8484
import static com.facebook.presto.spi.plan.AggregationNode.Step.PARTIAL;

‎presto-main-base/src/test/java/com/facebook/presto/sql/planner/optimizations/TestAddExchangesPlansWithFunctions.java‎

Lines changed: 28 additions & 38 deletions
Original file line numberDiff line numberDiff line change
@@ -230,24 +230,23 @@ public void testFilterWithCppFunctionDoesNotGetPushedIntoSystemTableScan()
230230
public void testJoinWithCppFunctionDoesNotGetPushedIntoSystemTableScan()
231231
{
232232
// java_bar is a java function, it is pushed down past the exchange
233+
// Verify filter is below GATHER exchange, allowing it to execute in the system table scan fragment
233234
assertNativeDistributedPlan(
234235
"SELECT c1.table_name FROM information_schema.columns c1 JOIN information_schema.columns c2 ON c1.ordinal_position = c2.ordinal_position WHERE java_bar(c1.ordinal_position) = 1",
235236
anyTree(
236237
exchange(
237238
join(INNER, ImmutableList.of(equiJoinClause("ordinal_position", "ordinal_position_4")),
238239
anyTree(
239240
exchange(REMOTE_STREAMING, GATHER,
240-
project(
241-
filter("java_bar(ordinal_position) = BIGINT'1'",
242-
tableScan("columns", ImmutableMap.of(
243-
"ordinal_position", "ordinal_position",
244-
"table_name", "table_name")))))),
241+
filter("java_bar(ordinal_position) = BIGINT'1'",
242+
tableScan("columns", ImmutableMap.of(
243+
"ordinal_position", "ordinal_position",
244+
"table_name", "table_name"))))),
245245
anyTree(
246246
exchange(REMOTE_STREAMING, GATHER,
247-
project(
248-
filter("java_bar(ordinal_position_4) = BIGINT'1'",
249-
tableScan("columns", ImmutableMap.of(
250-
"ordinal_position_4", "ordinal_position"))))))))));
247+
filter("java_bar(ordinal_position_4) = BIGINT'1'",
248+
tableScan("columns", ImmutableMap.of(
249+
"ordinal_position_4", "ordinal_position")))))))));
251250

252251
// cpp_foo is a CPP function, it is not pushed down past the exchange because the source is a system table scan
253252
assertNativeDistributedPlan(
@@ -259,15 +258,13 @@ public void testJoinWithCppFunctionDoesNotGetPushedIntoSystemTableScan()
259258
anyTree(
260259
filter("cpp_foo(ordinal_position) = BIGINT'1'",
261260
exchange(REMOTE_STREAMING, GATHER,
262-
project(
263-
tableScan("columns", ImmutableMap.of(
264-
"ordinal_position", "ordinal_position")))))),
261+
tableScan("columns", ImmutableMap.of(
262+
"ordinal_position", "ordinal_position"))))),
265263
anyTree(
266264
filter("cpp_foo(ordinal_position_4) = BIGINT'1'",
267265
exchange(REMOTE_STREAMING, GATHER,
268-
project(
269-
tableScan("columns", ImmutableMap.of(
270-
"ordinal_position_4", "ordinal_position")))))))))));
266+
tableScan("columns", ImmutableMap.of(
267+
"ordinal_position_4", "ordinal_position"))))))))));
271268
}
272269

273270
@Test
@@ -328,12 +325,10 @@ public void testMixedSystemAndRegularTables()
328325
join(INNER, ImmutableList.of(equiJoinClause("ordinal_position", "nationkey")),
329326
filter("cpp_foo(ordinal_position) = BIGINT'1'",
330327
exchange(REMOTE_STREAMING, GATHER,
331-
project(ImmutableMap.of("ordinal_position", expression("ordinal_position")),
332-
tableScan("columns", ImmutableMap.of("ordinal_position", "ordinal_position"))))),
328+
tableScan("columns", ImmutableMap.of("ordinal_position", "ordinal_position")))),
333329
anyTree(
334-
project(ImmutableMap.of("nationkey", expression("nationkey")),
335-
filter(
336-
tableScan("nation", ImmutableMap.of("nationkey", "nationkey"))))))));
330+
filter(
331+
tableScan("nation", ImmutableMap.of("nationkey", "nationkey")))))));
337332

338333
// Regular table with CPP function (should work normally without extra exchange)
339334
assertNativeDistributedPlan(
@@ -462,20 +457,18 @@ public void testMultipleSystemTableJoins()
462457
assertNativeDistributedPlan(
463458
"SELECT * FROM information_schema.columns c1 " +
464459
"JOIN information_schema.columns c2 ON cpp_foo(c1.ordinal_position) = cpp_foo(c2.ordinal_position)",
465-
anyTree(
466-
exchange(
467-
join(INNER, ImmutableList.of(equiJoinClause("cpp_foo", "foo_4")),
468-
exchange(
469-
project(ImmutableMap.of("cpp_foo", expression("cpp_foo")),
460+
anyTree(
461+
exchange(
462+
join(INNER, ImmutableList.of(equiJoinClause("cpp_foo", "foo_4")),
463+
exchange(
470464
project(ImmutableMap.of("cpp_foo", expression("cpp_foo(ordinal_position)")),
471465
exchange(REMOTE_STREAMING, GATHER,
472-
tableScan("columns", ImmutableMap.of("ordinal_position", "ordinal_position")))))),
473-
anyTree(
474-
exchange(
475-
project(ImmutableMap.of("foo_4", expression("foo_4")),
466+
tableScan("columns", ImmutableMap.of("ordinal_position", "ordinal_position"))))),
467+
anyTree(
468+
exchange(
476469
project(ImmutableMap.of("foo_4", expression("cpp_foo(ordinal_position_4)")),
477470
exchange(REMOTE_STREAMING, GATHER,
478-
tableScan("columns", ImmutableMap.of("ordinal_position_4", "ordinal_position")))))))))));
471+
tableScan("columns", ImmutableMap.of("ordinal_position_4", "ordinal_position"))))))))));
479472
}
480473

481474
@Test
@@ -629,22 +622,19 @@ public void testComplexJoinWithMultipleCppFunctions()
629622
"SELECT c1.table_name, n.name FROM information_schema.columns c1 " +
630623
"JOIN nation n ON cpp_foo(c1.ordinal_position) = n.nationkey " +
631624
"WHERE cpp_baz(c1.ordinal_position) > 0 AND cpp_foo(n.nationkey) < 100",
632-
anyTree(
633-
join(INNER, ImmutableList.of(equiJoinClause("cpp_foo", "nationkey")),
634-
project(ImmutableMap.of("table_name", expression("table_name"), "cpp_foo", expression("cpp_foo")),
625+
anyTree(
626+
join(INNER, ImmutableList.of(equiJoinClause("cpp_foo", "nationkey")),
635627
project(ImmutableMap.of("cpp_foo", expression("cpp_foo(ordinal_position)")),
636628
filter("cpp_baz(ordinal_position) > BIGINT'0' AND cpp_foo(cpp_foo(ordinal_position)) < BIGINT'100'",
637629
exchange(REMOTE_STREAMING, GATHER,
638630
tableScan("columns", ImmutableMap.of(
639631
"ordinal_position", "ordinal_position",
640-
"table_name", "table_name")))))),
641-
anyTree(
642-
project(ImmutableMap.of("nationkey", expression("nationkey"),
643-
"name", expression("name")),
632+
"table_name", "table_name"))))),
633+
anyTree(
644634
filter("cpp_foo(nationkey) < BIGINT'100'",
645635
tableScan("nation", ImmutableMap.of(
646636
"nationkey", "nationkey",
647-
"name", "name"))))))));
637+
"name", "name")))))));
648638
}
649639

650640
@Test

‎presto-main-base/src/test/java/com/facebook/presto/sql/planner/optimizations/TestRandomizeNullKeyInOuterJoin.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -20,9 +20,9 @@
2020
import org.testng.annotations.Test;
2121

2222
import static com.facebook.presto.SystemSessionProperties.JOIN_DISTRIBUTION_TYPE;
23-
import static com.facebook.presto.SystemSessionProperties.OPTIMIZE_HASH_GENERATION;
2423
import static com.facebook.presto.SystemSessionProperties.RANDOMIZE_OUTER_JOIN_NULL_KEY;
2524
import static com.facebook.presto.SystemSessionProperties.RANDOMIZE_OUTER_JOIN_NULL_KEY_STRATEGY;
25+
import static com.facebook.presto.sessionpropertyproviders.JavaWorkerSessionPropertyProvider.OPTIMIZE_HASH_GENERATION;
2626
import static com.facebook.presto.spi.plan.JoinType.INNER;
2727
import static com.facebook.presto.spi.plan.JoinType.LEFT;
2828
import static com.facebook.presto.spi.plan.JoinType.RIGHT;

‎presto-main-base/src/test/java/com/facebook/presto/sql/query/TestPrecomputedHashes.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,7 @@
1818
import org.testng.annotations.BeforeClass;
1919
import org.testng.annotations.Test;
2020

21-
import static com.facebook.presto.SystemSessionProperties.OPTIMIZE_HASH_GENERATION;
21+
import static com.facebook.presto.sessionpropertyproviders.JavaWorkerSessionPropertyProvider.OPTIMIZE_HASH_GENERATION;
2222
import static com.facebook.presto.testing.TestingSession.testSessionBuilder;
2323

2424
public class TestPrecomputedHashes

0 commit comments

Comments
 (0)