Add support for PK lookup if additional filters are combined by AND #10708

seut · 2020-10-27T09:37:14Z

If filters are combined by AND operators in addition to filters
on primary key columns, the optimized PK lookup plan is used now.

Closes #10298.

seut · 2020-10-27T16:16:53Z

moved back to draft mode, as I completely forgot to add the filters to the execution (filter projections) 🤦

server/src/main/java/io/crate/execution/dsl/projection/EvalProjection.java

server/src/main/java/io/crate/planner/operators/Get.java

mfussenegger · 2020-10-30T08:08:05Z

server/src/main/java/io/crate/planner/operators/Get.java

+        var toCollect = new LinkedHashSet<>(boundOutputs);
+        Consumer<Reference> addRefIfMatch = ref -> {
+            if (ref.ident().tableIdent().equals(tableRelation.relationName())
+                && docKeyColumns.contains(ref.column()) == false) {


I also don't quite understand why this check is necessary. toCollect is a set, so duplicates won't be added anyways?

Is this under the assumption that the symbol might not be needed because it is only used in the docKeys part? I'm not sure if that assertion is safe. Couldn't the query still use columns from docKeyColumns ?

Maybe it would make sense to use something like SymbolVisitors.intersection(query, allPossibleTableOutputs, toCollect::add); ?

I think that would handle functions better. If you have a SELECT substr(x, ...) WHERE .. (pk = 1 AND substr(x, ..) it wouldn't add x to the outputs I think.

Yep right, good catch. This logic which is intended to only add a filter projection when needed (other columns than DocKeys are part of the query) was not correct. I'll push a new safe logic.

Pushed a fixup before I saw your recent suggestion with the intersection. If you still think I should go in this direction instead, please shout.

And related to WHERE .. (pk = 1 AND substr(x, ..), as the RevVisitor traverses the tree x will be added to the outputs. This query works as expected. Do I miss anything?

I thought something along the lines of

create table tbl (id int primary key, x int); select x from tbl where id = 1 and (_seq_no = 1 or x = 1)

But that is forbidden.

Or

select x from tbl where id = 1 and (id = 3 or x = 1);

but that works. So all good 👍

server/src/main/java/io/crate/planner/statement/DeletePlanner.java

server/src/test/java/io/crate/integrationtests/SeqNoBasedOCCIntegrationTest.java

server/src/test/java/io/crate/integrationtests/WherePKIntegrationTest.java

mfussenegger · 2020-11-02T08:07:40Z

server/src/test/java/io/crate/testing/Asserts.java

@@ -40,4 +42,14 @@ public static void assertThrows(Executable executable, Matcher<? super Throwable
            assertThat(t, matcher);
        }
    }
+
+    public static void assertThrows(Executable executable, Class<? extends Throwable> type, String subString) {


Isn't this the same as org.junit.jupiter.api.Assertions.assertThrows ?

On the methods I know of/found, the given message is used as a prefix for a failure output instead of matching the thrown exception message. Or do other assertThrows implementation exists which I'm not aware of?

Oh right 👍

If filters are combined by AND operators in addition to filters on primary key columns, the optimized PK lookup plan is used now. Closes #10298.

mfussenegger · 2020-11-02T09:18:30Z

As a follow up - we should probably tweak the explain output so that the query part is visible?

seut · 2020-11-02T09:19:53Z

As a follow up - we should probably tweak the explain output so that the query part is visible?

Ah good point, will do.

Add the query symbol to the print output of the GET operators so `explain` statements will show them. Follow up of #10708.

Add the query symbol to the print output of the GET operators so `explain` statements will show it. Follow up of #10708.

#10708 changed the EqualityExtractor to allow in-exact matches, this change wasn't reflected by the update and delete planner, until #14361 tried to fix it, but it didn't account for AND clauses without references.

#10708 changed the EqualityExtractor to allow in-exact matches, this change wasn't reflected by the update and delete planner, until #14361 tried to fix it, but it didn't account for AND clauses without references. (cherry picked from commit 8251d26)

seut requested review from mfussenegger and removed request for mfussenegger October 27, 2020 11:24

seut marked this pull request as draft October 27, 2020 16:04

seut force-pushed the s/pk-filter branch 2 times, most recently from 3b50f69 to 514fe79 Compare October 29, 2020 16:27

seut marked this pull request as ready for review October 30, 2020 07:12

seut requested a review from mfussenegger October 30, 2020 07:12

mfussenegger reviewed Oct 30, 2020

View reviewed changes

seut requested a review from mfussenegger October 30, 2020 16:38

mfussenegger approved these changes Nov 2, 2020

View reviewed changes

Add support for PK lookup if additional filters are combined by AND

770bd3d

If filters are combined by AND operators in addition to filters on primary key columns, the optimized PK lookup plan is used now. Closes #10298.

seut force-pushed the s/pk-filter branch from eda372b to 770bd3d Compare November 2, 2020 08:49

seut added the ready-to-merge Let Mergify merge the PR once approved and checks pass label Nov 2, 2020

Merge branch 'master' into s/pk-filter

c1a9c80

mergify bot merged commit ab5e0a4 into master Nov 2, 2020

mergify bot deleted the s/pk-filter branch November 2, 2020 15:51

seut mentioned this pull request Nov 2, 2020

PRIMARY KEY lookup with additional filters #10298

Closed

seut added a commit that referenced this pull request Nov 20, 2020

Print the query symbol on GET logical plans

e95a515

Add the query symbol to the print output of the GET operators so `explain` statements will show them. Follow up of #10708.

seut added a commit that referenced this pull request Nov 20, 2020

Print the query symbol on GET logical plans

88544cc

Add the query symbol to the print output of the GET operators so `explain` statements will show it. Follow up of #10708.

seut added a commit that referenced this pull request Nov 20, 2020

Print the query symbol on GET logical plans

72c0c24

Add the query symbol to the print output of the GET operators so `explain` statements will show it. Follow up of #10708.

seut mentioned this pull request Nov 20, 2020

Print the query symbol on GET logical plans #10791

Merged

seut added a commit that referenced this pull request Nov 20, 2020

Print the query symbol on GET logical plans

4718e18

Add the query symbol to the print output of the GET operators so `explain` statements will show it. Follow up of #10708.

seut added a commit that referenced this pull request Nov 20, 2020

Print the query symbol on GET logical plans

428f477

Add the query symbol to the print output of the GET operators so `explain` statements will show it. Follow up of #10708.

mergify bot pushed a commit that referenced this pull request Nov 20, 2020

Print the query symbol on GET logical plans

6119a66

Add the query symbol to the print output of the GET operators so `explain` statements will show it. Follow up of #10708.

mfussenegger mentioned this pull request Jan 8, 2024

Fix handling of query conditions in delete/update with WHERE doc-keys #15293

Merged

mfussenegger mentioned this pull request Feb 26, 2024

Fix PK lookup with OR and non-PK column #15601

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for PK lookup if additional filters are combined by AND #10708

Add support for PK lookup if additional filters are combined by AND #10708

seut commented Oct 27, 2020

seut commented Oct 27, 2020

mfussenegger Oct 30, 2020

mfussenegger Oct 30, 2020 •

edited

seut Oct 30, 2020

seut Oct 30, 2020 •

edited

seut Oct 30, 2020

mfussenegger Nov 2, 2020

mfussenegger Nov 2, 2020

seut Nov 2, 2020

mfussenegger Nov 2, 2020

mfussenegger commented Nov 2, 2020

seut commented Nov 2, 2020

Add support for PK lookup if additional filters are combined by AND #10708

Add support for PK lookup if additional filters are combined by AND #10708

Conversation

seut commented Oct 27, 2020

seut commented Oct 27, 2020

mfussenegger Oct 30, 2020

Choose a reason for hiding this comment

mfussenegger Oct 30, 2020 • edited

Choose a reason for hiding this comment

seut Oct 30, 2020

Choose a reason for hiding this comment

seut Oct 30, 2020 • edited

Choose a reason for hiding this comment

seut Oct 30, 2020

Choose a reason for hiding this comment

mfussenegger Nov 2, 2020

Choose a reason for hiding this comment

mfussenegger Nov 2, 2020

Choose a reason for hiding this comment

seut Nov 2, 2020

Choose a reason for hiding this comment

mfussenegger Nov 2, 2020

Choose a reason for hiding this comment

mfussenegger commented Nov 2, 2020

seut commented Nov 2, 2020

mfussenegger Oct 30, 2020 •

edited

seut Oct 30, 2020 •

edited