Filter out Iceberg splits that do not match predicates not expressible by tuple domain #9830

homar · 2021-11-02T11:42:02Z

fixes #9309
fixes #7608
The idea is to push predicate down to split source and not generate splits
that would be filtered out by this predicate.

homar · 2021-11-03T08:24:39Z

Failing tests are
TestDruidConnectorTest>AbstractTestQueryFramework.init:97->createQueryRunner:34 » Runtime TestDruidLatestConnectorTest>AbstractTestQueryFramework.init:97->createQueryRunner:36 » Runtime
and the failures don't seem to be related

findepi · 2021-11-04T11:14:54Z

core/trino-main/src/main/java/io/trino/execution/SqlQueryExecution.java

@@ -511,7 +511,7 @@ private PlanRoot doPlanQuery()
    private void planDistribution(PlanRoot plan)
    {
        // plan the execution on the active nodes
-        DistributedExecutionPlanner distributedPlanner = new DistributedExecutionPlanner(splitManager, metadata, dynamicFilterService);
+        DistributedExecutionPlanner distributedPlanner = new DistributedExecutionPlanner(splitManager, metadata, dynamicFilterService, new TypeAnalyzer(sqlParser, metadata));


inject TypeAnalyzer from guice

I was thinking about that but I noticed that in the doPlanQuery method it is created the same way so I thought it is better to be consisted. On the other hand I can replace both occurrences with an injected value.
WDYT ?

make sense to change both and not construct TypeAnalyzer manually.

this should be a prep commit

findepi · 2021-11-04T11:16:39Z

.../trino-main/src/main/java/io/trino/sql/planner/iterative/rule/LayoutConstraintEvaluator.java

+
+    public LayoutConstraintEvaluator(Metadata metadata, TypeAnalyzer typeAnalyzer, Session session, TypeProvider types, Map<Symbol, ColumnHandle> assignments, Expression expression)
+    {
+        this.assignments = assignments;


rnn and ImmutableMap.copyOf (now that the class is public, it's more important)

.../trino-main/src/main/java/io/trino/sql/planner/iterative/rule/LayoutConstraintEvaluator.java

core/trino-spi/src/main/java/io/trino/spi/connector/ConstraintEvaluator.java

core/trino-spi/src/main/java/io/trino/spi/connector/ConnectorSplitManager.java

findepi · 2021-11-04T11:28:10Z

plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/BaseIcebergConnectorTest.java

@@ -2429,9 +2429,7 @@ public void testSplitPruningForFilterOnPartitionColumn()
        verifySplitCount("SELECT * FROM " + tableName + " WHERE regionkey < 2", 2);
        verifySplitCount("SELECT * FROM " + tableName + " WHERE regionkey < 0", 0);
        verifySplitCount("SELECT * FROM " + tableName + " WHERE regionkey > 1 AND regionkey < 4", 2);
-
-        // TODO(https://github.com/trinodb/trino/issues/9309) should be 1


Mark your PR as fixing this issue (and #7608 too)

How do I do that? I already have #9309 in the description

add "fixes #nnnn" to PR description. then take a look at this section on the sidebar:

findepi · 2021-11-04T11:28:50Z

plugin/trino-prometheus/src/test/java/io/trino/plugin/prometheus/TestPrometheusSplit.java

@@ -158,7 +159,8 @@ public void testQueryDividedIntoSplitsFirstSplitHasRightTime()
                null,
                new PrometheusTableHandle("default", table.getName()),
                null,
-                (DynamicFilter) null);
+                (DynamicFilter) null,
+                null);


pass ALWAYS_TRUE (aka EMPTY)

(here & below)

ok, again I followed DynamicFiltering example ;)

findepi · 2021-11-04T11:31:27Z

core/trino-main/src/main/java/io/trino/sql/planner/DistributedExecutionPlanner.java

@@ -203,12 +207,21 @@ private Visitor(
                dynamicFilter = dynamicFilterService.createDynamicFilter(session.getQueryId(), dynamicFilters, node.getAssignments(), typeProvider);
            }

+            ConstraintEvaluator constraintEvaluator = ConstraintEvaluator.EMPTY;
+            Optional<ConstraintEvaluator> existingEvaluator = filter.map(FilterNode::getPredicate)
+                    .filter(expression -> !DynamicFilters.isDynamicFilter(expression))


You need to disassemble the expression into conjuncts, filtering out dynamic filters

Expression nonDynamicPredicate = [ExpressionUtils.]filterConjuncts(metadata, predicate, expression -> !DynamicFilters.isDynamicFilter(expression));

core/trino-main/src/main/java/io/trino/sql/planner/DistributedExecutionPlanner.java

plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergSplitSource.java

homar · 2021-11-08T09:02:28Z

@findepi please take another look ;)

core/trino-spi/src/main/java/io/trino/spi/connector/ConstraintEvaluator.java

losipiuk · 2021-11-08T10:45:12Z

core/trino-spi/src/main/java/io/trino/spi/connector/ConstraintEvaluator.java

+import java.util.Map;
+import java.util.Set;
+
+public interface ConstraintEvaluator


some javadoc?

i can give it a try ;)

Maybe just:

Represents a constraint passed to {@link ConnectorSplitManager#getSplits}. If {@link ConstraintEvaluator#isCandidate} returns false given actual partial column values binding, for given split, it means split can be safely dropped without processing.

i think i know why we do not pass Constraint into split manager -- we don't want to pass a TupleDomain so that connector doesn't want to intersect DynamicFilter's domain with Constraint's domain.

but then, the two classes: Constraint and ConstraintEvaluator have naturally similar names, and the second looks like the evaluator of the first.

i am not sure about the naming. maybe we can find a better name, like TestableConstraint or something

should Constraint reuse this new class?

cc @losipiuk ^

core/trino-main/src/main/java/io/trino/sql/planner/iterative/rule/ExtractSpatialJoins.java

losipiuk

LGTM

homar · 2021-11-09T08:34:42Z

@losipiuk @findepi comments addressed please take a look

core/trino-main/src/main/java/io/trino/sql/planner/DistributedExecutionPlanner.java

core/trino-main/src/main/java/io/trino/testing/LocalQueryRunner.java

plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/TestIcebergSplitSource.java

plugin/trino-kudu/src/test/java/io/trino/plugin/kudu/TestKuduIntegrationDynamicFilter.java

testing/trino-benchmark/src/main/java/io/trino/benchmark/AbstractOperatorBenchmark.java

losipiuk · 2021-11-09T09:59:02Z

Nothing important

findepi

lgtm except 49491a2#r745486692

nits

findepi · 2021-11-09T10:38:23Z

.../trino-main/src/main/java/io/trino/sql/planner/iterative/rule/LayoutConstraintEvaluator.java

+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package io.trino.sql.planner.iterative.rule;


io.trino.sql.planner, as it's going to be used outside of iterative optimizer

core/trino-main/src/main/java/io/trino/execution/SqlQueryExecution.java

findepi · 2021-11-09T10:44:18Z

plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergSplitSource.java

+    {
+        Map<ColumnHandle, NullableValue> bindings = new HashMap<>();
+        for (IcebergColumnHandle partitionColumn : identityPartitionColumns) {
+            Object partitionValue = deserializePartitionValue(


losipiuk · 2021-11-10T11:44:24Z

core/trino-main/src/main/java/io/trino/sql/planner/DistributedExecutionPlanner.java

@@ -203,12 +212,19 @@ private Visitor(
                dynamicFilter = dynamicFilterService.createDynamicFilter(session.getQueryId(), dynamicFilters, node.getAssignments(), typeProvider);
            }

+            Constraint constraint = filterPredicate
+                    .map(predicate -> filterConjuncts(metadata, predicate, expression -> !DynamicFilters.isDynamicFilter(expression)))
+                    .map(expression -> new LayoutConstraintEvaluator(metadata, typeAnalyzer, session, typeProvider, node.getAssignments(), expression))


nit: can you use predicate here again?

losipiuk

Looks good to me

losipiuk · 2021-11-10T11:45:26Z

core/trino-main/src/main/java/io/trino/sql/planner/DistributedExecutionPlanner.java

+            Constraint constraint = filterPredicate
+                    .map(predicate -> filterConjuncts(metadata, predicate, expression -> !DynamicFilters.isDynamicFilter(expression)))
+                    .map(expression -> new LayoutConstraintEvaluator(metadata, typeAnalyzer, session, typeProvider, node.getAssignments(), expression))
+                    .map(evaluator -> new Constraint(TupleDomain.all(), evaluator::isCandidate, evaluator.getArguments()))


add a comment
// we are interested only in functional predicate here so we set the summary to ALL.

plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergSplitSource.java

alexjo2144

Commit message nits:
Inject type analyzer int SqlQueryExecution instead of creating it "int" -> "into"
Filter out splits that do not match predicates not expressible by tup… is too long

alexjo2144 · 2021-11-17T21:03:23Z

core/trino-main/src/main/java/io/trino/execution/SqlQueryExecution.java

    {
        try (SetThreadName ignored = new SetThreadName("Query-%s", stateMachine.getQueryId())) {
            this.slug = requireNonNull(slug, "slug is null");
            this.metadata = requireNonNull(metadata, "metadata is null");
            this.typeOperators = requireNonNull(typeOperators, "typeOperators is null");
-            this.sqlParser = requireNonNull(sqlParser, "sqlParser is null");


If the sqlParser parameter is unused now, remove it

It is used, in line 193. it is passed to analyze method

alexjo2144 · 2021-11-17T21:24:49Z

plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergSplitSource.java

@@ -167,6 +190,9 @@ public IcebergSplitSource(
                    continue;
                }
            }
+            if (!partitionMatchesConstraint(identityPartitionColumns, partitionValues.get(), constraint)) {


The Supplier's get is always getting called here, which kinda defeats the purpose. You can add an constraint.predicate().isPresent() check first, before calling partitionMatchesConstraint, or pass the entire Supplier to partitionMatchesConstraint rather than calling get here to prolong constructing the Map until actually necessary.

…le domain fixes trinodb#9309 fixes trinodb#7608 The idea is to push constraint down to split source and not generate splits that would be filtered out by this constraint's predicate.

homar · 2021-11-18T12:31:23Z

@alexjo2144 @losipiuk @findepi comments addressed

cla-bot bot added the cla-signed label Nov 2, 2021

homar requested a review from findepi November 2, 2021 11:42

homar force-pushed the homar/partition_pruning_for_predicates_not_expressible_by_tupple_domain branch 2 times, most recently from af3f1f3 to c27e6c9 Compare November 2, 2021 14:54

homar requested a review from losipiuk November 3, 2021 13:25

homar marked this pull request as ready for review November 3, 2021 13:26

findepi reviewed Nov 4, 2021

View reviewed changes

homar force-pushed the homar/partition_pruning_for_predicates_not_expressible_by_tupple_domain branch 2 times, most recently from 7778b5a to 5394f6b Compare November 5, 2021 09:14

losipiuk reviewed Nov 8, 2021

View reviewed changes

core/trino-spi/src/main/java/io/trino/spi/connector/ConstraintEvaluator.java Outdated Show resolved Hide resolved

losipiuk reviewed Nov 8, 2021

View reviewed changes

core/trino-main/src/main/java/io/trino/sql/planner/iterative/rule/ExtractSpatialJoins.java Outdated Show resolved Hide resolved

losipiuk reviewed Nov 8, 2021

View reviewed changes

homar force-pushed the homar/partition_pruning_for_predicates_not_expressible_by_tupple_domain branch from 5394f6b to 60c6a8e Compare November 9, 2021 08:34

homar force-pushed the homar/partition_pruning_for_predicates_not_expressible_by_tupple_domain branch from 60c6a8e to e3ba940 Compare November 9, 2021 09:34

losipiuk reviewed Nov 9, 2021

View reviewed changes

core/trino-main/src/main/java/io/trino/sql/planner/DistributedExecutionPlanner.java Outdated Show resolved Hide resolved

losipiuk reviewed Nov 9, 2021

View reviewed changes

homar force-pushed the homar/partition_pruning_for_predicates_not_expressible_by_tupple_domain branch from e3ba940 to 49491a2 Compare November 9, 2021 10:27

findepi reviewed Nov 9, 2021

View reviewed changes

homar force-pushed the homar/partition_pruning_for_predicates_not_expressible_by_tupple_domain branch from 49491a2 to fc3644d Compare November 9, 2021 11:53

Extract LayoutConstraintEvaluator from PushPredicateIntoTableScan

d3af6f2

homar force-pushed the homar/partition_pruning_for_predicates_not_expressible_by_tupple_domain branch 2 times, most recently from 1ad71f2 to 32dc400 Compare November 10, 2021 11:13

losipiuk reviewed Nov 10, 2021

View reviewed changes

homar force-pushed the homar/partition_pruning_for_predicates_not_expressible_by_tupple_domain branch from 32dc400 to dd64c8f Compare November 10, 2021 14:01

alexjo2144 reviewed Nov 17, 2021

View reviewed changes

homar added 2 commits November 18, 2021 09:28

Inject type analyzer into SqlQueryExecution instead of creating it

0feeffe

Filter out splits that do not match predicates not expressible by tup…

a901d2c

…le domain fixes trinodb#9309 fixes trinodb#7608 The idea is to push constraint down to split source and not generate splits that would be filtered out by this constraint's predicate.

homar force-pushed the homar/partition_pruning_for_predicates_not_expressible_by_tupple_domain branch from dd64c8f to a901d2c Compare November 18, 2021 08:48

findepi approved these changes Nov 22, 2021

View reviewed changes

findepi merged commit c973251 into trinodb:master Nov 22, 2021

findepi mentioned this pull request Nov 22, 2021

Release notes for 365 #9826

Closed

12 tasks

github-actions bot added this to the 365 milestone Nov 22, 2021

findepi changed the title ~~Filter out splits that do not match predicates not expressible by tuple domain~~ Filter out Iceberg splits that do not match predicates not expressible by tuple domain Mar 3, 2022

findepi mentioned this pull request Jun 23, 2022

Derive predicate over base column for CAST(a_varchar AS date) > a_date_constant condition #12925

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Filter out Iceberg splits that do not match predicates not expressible by tuple domain #9830

Filter out Iceberg splits that do not match predicates not expressible by tuple domain #9830

homar commented Nov 2, 2021 •

edited

Loading

homar commented Nov 3, 2021

findepi Nov 4, 2021

homar Nov 4, 2021

losipiuk Nov 8, 2021

homar Nov 9, 2021

findepi Nov 9, 2021

findepi Nov 4, 2021

findepi Nov 4, 2021

homar Nov 4, 2021

findepi Nov 9, 2021

findepi Nov 4, 2021

homar Nov 4, 2021

findepi Nov 4, 2021

homar commented Nov 8, 2021

losipiuk Nov 8, 2021

homar Nov 9, 2021

losipiuk Nov 9, 2021 •

edited

Loading

findepi Nov 9, 2021

findepi Nov 9, 2021

losipiuk left a comment

homar commented Nov 9, 2021

losipiuk commented Nov 9, 2021

findepi left a comment

findepi Nov 9, 2021

findepi Nov 9, 2021

losipiuk Nov 10, 2021

losipiuk left a comment

losipiuk Nov 10, 2021

alexjo2144 left a comment

alexjo2144 Nov 17, 2021

homar Nov 18, 2021

alexjo2144 Nov 17, 2021

homar commented Nov 18, 2021

Filter out Iceberg splits that do not match predicates not expressible by tuple domain #9830

Filter out Iceberg splits that do not match predicates not expressible by tuple domain #9830

Conversation

homar commented Nov 2, 2021 • edited Loading

homar commented Nov 3, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

homar commented Nov 8, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

losipiuk Nov 9, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

losipiuk left a comment

Choose a reason for hiding this comment

homar commented Nov 9, 2021

losipiuk commented Nov 9, 2021

findepi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

losipiuk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexjo2144 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

homar commented Nov 18, 2021

homar commented Nov 2, 2021 •

edited

Loading

losipiuk Nov 9, 2021 •

edited

Loading