Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Record Type comparison not included as able to satisfy a sort #744

Open
alecgrieser opened this issue Oct 3, 2019 · 5 comments
Open

Record Type comparison not included as able to satisfy a sort #744

alecgrieser opened this issue Oct 3, 2019 · 5 comments
Labels
planner Related to the query planner

Comments

@alecgrieser
Copy link
Contributor

If one has a record type with a primary key like:

concat(recordType(), field("id"))

The the query:

RecordQuery.newBuilder()
    .setRecordType(recordType)
    .setSort(concat(recordType(), field("id")))
    .build();

Can be satisfied by scanning over records with a primary key beginning with that record type's record type key. This plan would look like:

[IS recordType]

In the RecordQueryPlan's toString representation. However, the actual plan is:

Scan(<,>) | [IS recordType]

That is, it does a full scan over everything and then filters out the other types. Practically speaking, the result you could back would be identical as if the sort were not specified (in this case), but (a) the planner isn't guaranteeing it and (b) there is no way to specify reverse-ness.


Adding this test to RecordTypeKeyTest demonstrates this:

    @ParameterizedTest(name = "testSort [reverse = 0]")
    @BooleanSource
    public void testSort(boolean reverse) throws Exception {
        List<FDBStoredRecord<Message>> recs = saveSomeRecords(BASIC_HOOK);

        try (FDBRecordContext context = openContext()) {
            openSimpleRecordStore(context, BASIC_HOOK);

            RecordQuery query = RecordQuery.newBuilder()
                    .setRecordType("MySimpleRecord")
                    .setSort(recordStore.getRecordMetaData().getRecordType("MySimpleRecord").getPrimaryKey(), reverse)
                    // .setSort(recordStore.getRecordMetaData().getRecordType("MySimpleRecord").getPrimaryKey().getSubKey(1, 2), reverse)
                    .build();
            RecordQueryPlan plan = planner.plan(query);

            List<FDBStoredRecord<Message>> expectedResults = recs.subList(0, 2);
            if (reverse) {
                expectedResults = Lists.reverse(expectedResults);
            }

            assertEquals(expectedResults, recordStore.executeQuery(query)
                    .map(FDBQueriedRecord::getStoredRecord).asList().join());
            assertThat(plan, scan(bounds(hasTupleString("[IS MySimpleRecord"))));
        }
    }

Note that if one includes the record type key in the sort predicate, then it will do a full scan. If one omits the record type key (which it should be able to), then it throws an error.

@alecgrieser
Copy link
Contributor Author

This behavior is seen with the current planner (i.e., RecordQueryPlanner). I'm not sure if the new planner has this behavior, though I actually think it doesn't support record type key expressions (yet), so it's somewhat of a moot point.

@alecgrieser alecgrieser added the planner Related to the query planner label Oct 3, 2019
alecgrieser added a commit to alecgrieser/fdb-record-layer that referenced this issue Oct 9, 2019
This adds test to demonstrate a deficiency in the planner pointed out in FoundationDB#744. The failing tests are disabled here so that they do not break the build but so that (a) there is a record in the code of the kind of cases that are broken here and (b) once the problem is fixed that PR can just enable these tests. This also makes the RecordTypeKeyTest extend from `RecordQueryTestBase` so that the `@DualPlannerTest` annotations can be applied, though it does not actually make any tests use the new planner (as they appear to not work yet with the way the planner is on master).
@alecgrieser
Copy link
Contributor Author

Oh, interesting. I apparently put "fix" too close to "#744" in the PR title of #750, which accidentally closed this issue. Re-opening.

@alecgrieser alecgrieser reopened this Oct 10, 2019
@rahul-nitkkr
Copy link

@alecgrieser is there any timeline or target release as to when this might be fixed ?

@alecgrieser
Copy link
Contributor Author

There aren't really any updates for this, no. I don't believe anyone has yet started on it yet, with people currently all assigned to other issues.

@alecgrieser
Copy link
Contributor Author

For future reference, this issue was also referenced in this forum post: https://forums.foundationdb.org/t/full-range-scan-performed-in-sort-when-not-required/1995

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
planner Related to the query planner
Projects
None yet
Development

No branches or pull requests

2 participants