Skip to content

Commit

Permalink
SERVER-95215 Omit new distinct scan explain flags when feature flag o…
Browse files Browse the repository at this point in the history 
…ff (#28921)

GitOrigin-RevId: 9bd9e18242f038463f37aba2fe20cbd6965b1e3a
  • Loading branch information
@alyacb
alyacb authored and MongoDB Bot committed Nov 7, 2024
1 parent a18b070 commit 5f17ad2
Show file tree
Hide file tree
Showing 8 changed files with 144 additions and 84 deletions.
28 changes: 15 additions & 13 deletions jstests/core/query/collation/collation.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -70,18 +70,20 @@ var getQueryCollation = function(explainRes) {
return null;
};

function planHasFetch(winningPlan) {
// TODO SERVER-95215: Check that FETCH is pushed into DISTINCT_SCAN iff
// featureFlagShardFilteringDistinctScan is enabled.
if (planHasStage(testDb, winningPlan, "FETCH")) {
return true;
function distinctScanPlanHasFetch(winningPlan) {
// On upgrade/downgrade suites, we can't directly check the feature flag, because it might
// change between when we check its value, and when the query is executed. Instead, we check for
// the presence/absence of a flag that should only be present when the flag is on.
const ds = getPlanStages(winningPlan, "DISTINCT_SCAN")[0];
const hasFetch = planHasStage(testDb, winningPlan, "FETCH");
if (ds.hasOwnProperty("isFetching")) {
// If this flag is present + we have a separate distinct scan, we should not have a separate
// fetch stage.
assert(!hasFetch);
return ds.isFetching;
}

if (!planHasStage(testDb, winningPlan, "DISTINCT_SCAN")) {
return false;
}

return getPlanStages(winningPlan, "DISTINCT_SCAN")[0].isFetching;
return hasFetch;
}

//
Expand Down Expand Up @@ -571,13 +573,13 @@ assert.commandWorked(testDb.createCollection(coll.getName(), {collation: {locale
assert.commandWorked(coll.createIndex({a: 1}, {collation: {locale: "en_US"}}));
var explain = coll.explain("queryPlanner").distinct("a");
assert(planHasStage(testDb, getWinningPlanFromExplain(explain.queryPlanner), "DISTINCT_SCAN"));
assert(planHasFetch(getWinningPlanFromExplain(explain.queryPlanner)));
assert(distinctScanPlanHasFetch(getWinningPlanFromExplain(explain.queryPlanner)));

// Distinct scan on strings can be used over an index with a collation when the predicate has
// exact bounds.
explain = coll.explain("queryPlanner").distinct("a", {a: {$gt: "foo"}});
assert(planHasStage(testDb, getWinningPlanFromExplain(explain.queryPlanner), "DISTINCT_SCAN"));
assert(planHasFetch(getWinningPlanFromExplain(explain.queryPlanner)));
assert(distinctScanPlanHasFetch(getWinningPlanFromExplain(explain.queryPlanner)));
assert(
!planHasStage(testDb, getWinningPlanFromExplain(explain.queryPlanner), "PROJECTION_COVERED"));

Expand All @@ -592,7 +594,7 @@ assert(!planHasStage(testDb, getWinningPlanFromExplain(explain.queryPlanner), "D
explain = coll.explain("queryPlanner").distinct("a", {a: {$gt: 3}});
assert(planHasStage(testDb, getWinningPlanFromExplain(explain.queryPlanner), "DISTINCT_SCAN"));
assert(planHasStage(testDb, getWinningPlanFromExplain(explain.queryPlanner), "PROJECTION_COVERED"));
assert(!planHasFetch(getWinningPlanFromExplain(explain.queryPlanner)));
assert(!distinctScanPlanHasFetch(getWinningPlanFromExplain(explain.queryPlanner)));

// Distinct should not use index when no collation specified and collection default collation is
// incompatible with index collation.
Expand Down
78 changes: 49 additions & 29 deletions jstests/libs/query/group_conversion_to_distinct_scan.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,8 +25,9 @@ export function runGroupConversionToDistinctScanTests(database, isSharded = fals
// $group on a non-shard key field with $first and $last is currently not eligible for a
// DISTINCT_SCAN.
// TODO SERVER-96134: Investigate if this is the desired behavior.
const assertPlanUsesDistinctScanOnlyOnStandalone =
isSharded ? assertPlanDoesNotUseDistinctScan : assertPlanUsesDistinctScan;
const assertPlanUsesDistinctScanOnlyOnStandalone = isSharded
? assertPlanDoesNotUseDistinctScan
: (explain, keyPattern) => assertPlanUsesDistinctScan(database, explain, keyPattern);

//
// Verify that a $sort-$group pipeline can use DISTINCT_SCAN when the sort is available from an
Expand All @@ -35,7 +36,8 @@ export function runGroupConversionToDistinctScanTests(database, isSharded = fals
assertPipelineResultsAndExplain({
pipeline: [{$sort: {a: 1}}, {$group: {_id: "$a"}}],
expectedOutput: [{_id: null}, {_id: 1}, {_id: 2}],
validateExplain: (explain) => assertPlanUsesDistinctScan(explain, {a: 1, b: 1, c: 1}),
validateExplain: (explain) =>
assertPlanUsesDistinctScan(database, explain, {a: 1, b: 1, c: 1}),
});

//
Expand All @@ -50,7 +52,7 @@ export function runGroupConversionToDistinctScanTests(database, isSharded = fals
assertPipelineResultsAndExplain({
pipeline,
expectedOutput,
validateExplain: (explain) => assertPlanUsesDistinctScan(explain, keyPattern),
validateExplain: (explain) => assertPlanUsesDistinctScan(database, explain, keyPattern),
});

//
Expand All @@ -70,7 +72,7 @@ export function runGroupConversionToDistinctScanTests(database, isSharded = fals
pipeline,
expectedOutput,
hint: "a_1_b_1_c_1",
validateExplain: (explain) => assertPlanUsesDistinctScan(explain, keyPattern),
validateExplain: (explain) => assertPlanUsesDistinctScan(database, explain, keyPattern),
});

//
Expand All @@ -80,7 +82,7 @@ export function runGroupConversionToDistinctScanTests(database, isSharded = fals
pipeline,
expectedOutput,
hint: {a: 1, b: 1, c: 1},
validateExplain: (explain) => assertPlanUsesDistinctScan(explain, keyPattern),
validateExplain: (explain) => assertPlanUsesDistinctScan(database, explain, keyPattern),
});

//
Expand All @@ -106,7 +108,7 @@ export function runGroupConversionToDistinctScanTests(database, isSharded = fals
assertPipelineResultsAndExplain({
pipeline,
expectedOutput,
validateExplain: (explain) => assertPlanUsesDistinctScan(explain, keyPattern),
validateExplain: (explain) => assertPlanUsesDistinctScan(database, explain, keyPattern),
});

//
Expand All @@ -124,7 +126,7 @@ export function runGroupConversionToDistinctScanTests(database, isSharded = fals
((TestData.isHintsToQuerySettingsSuite &&
FeatureFlagUtil.isPresentAndEnabled(database, "ShardFilteringDistinctScan"))
? assertPlanUsesCollScan(explain)
: assertPlanUsesDistinctScan(explain, keyPattern)),
: assertPlanUsesDistinctScan(database, explain, keyPattern)),
});

//
Expand All @@ -136,7 +138,7 @@ export function runGroupConversionToDistinctScanTests(database, isSharded = fals
expectedOutput,
hint: {_id: 1},
expectsIndexFilter: !TestData.isHintsToQuerySettingsSuite,
validateExplain: (explain) => assertPlanUsesDistinctScan(explain, keyPattern),
validateExplain: (explain) => assertPlanUsesDistinctScan(database, explain, keyPattern),
});

assert.commandWorked(database.runCommand({planCacheClearFilters: coll.getName()}));
Expand All @@ -158,7 +160,8 @@ export function runGroupConversionToDistinctScanTests(database, isSharded = fals
assertPipelineResultsAndExplain({
pipeline: [{$sort: {a: 1, b: 1}}, {$group: {_id: "$a", accum: {$first: "$b"}}}],
expectedOutput: [{_id: null, accum: null}, {_id: 1, accum: 1}, {_id: 2, accum: 2}],
validateExplain: (explain) => assertPlanUsesDistinctScan(explain, {a: 1, b: 1, c: 1}),
validateExplain: (explain) =>
assertPlanUsesDistinctScan(database, explain, {a: 1, b: 1, c: 1}),
});

//
Expand All @@ -168,7 +171,8 @@ export function runGroupConversionToDistinctScanTests(database, isSharded = fals
assertPipelineResultsAndExplain({
pipeline: [{$sort: {a: -1, b: -1}}, {$group: {_id: "$a", accum: {$last: "$b"}}}],
expectedOutput: [{_id: null, accum: null}, {_id: 1, accum: 1}, {_id: 2, accum: 2}],
validateExplain: (explain) => assertPlanUsesDistinctScan(explain, {a: 1, b: 1, c: 1}),
validateExplain: (explain) =>
assertPlanUsesDistinctScan(database, explain, {a: 1, b: 1, c: 1}),
});

//
Expand All @@ -183,7 +187,8 @@ export function runGroupConversionToDistinctScanTests(database, isSharded = fals
{_id: 1, accum: {_id: 3, a: 1, b: 3, c: 2}},
{_id: 2, accum: {_id: 4, a: 2, b: 2, c: 2}}
],
validateExplain: (explain) => assertPlanUsesDistinctScan(explain, {a: 1, b: 1, c: 1}),
validateExplain: (explain) =>
assertPlanUsesDistinctScan(database, explain, {a: 1, b: 1, c: 1}),
});

//
Expand All @@ -197,7 +202,8 @@ export function runGroupConversionToDistinctScanTests(database, isSharded = fals
{_id: 1, accum: {_id: 3, a: 1, b: 3, c: 2}},
{_id: 2, accum: {_id: 4, a: 2, b: 2, c: 2}}
],
validateExplain: (explain) => assertPlanUsesDistinctScan(explain, {a: 1, b: 1, c: 1}),
validateExplain: (explain) =>
assertPlanUsesDistinctScan(database, explain, {a: 1, b: 1, c: 1}),
});

//
Expand Down Expand Up @@ -237,7 +243,8 @@ export function runGroupConversionToDistinctScanTests(database, isSharded = fals
assertPipelineResultsAndExplain({
pipeline: [{$group: {_id: "$foo.a"}}],
expectedOutput: [{_id: null}, {_id: 1}, {_id: 2}, {_id: 3}],
validateExplain: assertPlanUsesDistinctScan,
validateExplain: (explain, keyPattern) =>
assertPlanUsesDistinctScan(database, explain, keyPattern),
});

// TODO SERVER-96134: We shouldn't use DISTINCT_SCAN here.
Expand Down Expand Up @@ -469,7 +476,7 @@ export function runGroupConversionToDistinctScanTests(database, isSharded = fals
pipeline: [{$sort: {"foo.a": 1}}, {$group: {_id: "$foo.a"}}],
expectedOutput: [{_id: null}, {_id: 1}, {_id: 2}, {_id: 3}],
validateExplain: (explain) =>
assertPlanUsesDistinctScan(explain, {"foo.a": 1, "mkFoo.b": 1}),
assertPlanUsesDistinctScan(database, explain, {"foo.a": 1, "mkFoo.b": 1}),
});
}

Expand Down Expand Up @@ -505,7 +512,8 @@ export function runGroupConversionToDistinctScanTests(database, isSharded = fals
pipeline:
[{$sort: {a: 1, b: 1}}, {$group: {_id: "$a", accum: {$first: {$add: ["$b", "$c"]}}}}],
expectedOutput: [{_id: null, accum: null}, {_id: 1, accum: 2}, {_id: 2, accum: 4}],
validateExplain: (explain) => assertPlanUsesDistinctScan(explain, {a: 1, b: 1, c: 1}),
validateExplain: (explain) =>
assertPlanUsesDistinctScan(database, explain, {a: 1, b: 1, c: 1}),
});

//
Expand All @@ -523,7 +531,8 @@ export function runGroupConversionToDistinctScanTests(database, isSharded = fals
{$project: {_id: "$_id", accum: {$round: [{$add: ["$accum", 0.6]}, 0]}}}
],
expectedOutput: [{_id: null, accum: 3.0}, {_id: 1, accum: 6}, {_id: 2, accum: 5}],
validateExplain: (explain) => assertPlanUsesDistinctScan(explain, {a: 1, b: 1, c: 1}),
validateExplain: (explain) =>
assertPlanUsesDistinctScan(database, explain, {a: 1, b: 1, c: 1}),
});

//
Expand All @@ -534,7 +543,8 @@ export function runGroupConversionToDistinctScanTests(database, isSharded = fals
assertPipelineResultsAndExplain({
pipeline: [{$match: {a: 1}}, {$sort: {b: 1}}, {$group: {_id: "$b"}}],
expectedOutput: [{_id: 1}, {_id: 2}, {_id: 3}],
validateExplain: (explain) => assertPlanUsesDistinctScan(explain, {a: 1, b: 1, c: 1}),
validateExplain: (explain) =>
assertPlanUsesDistinctScan(database, explain, {a: 1, b: 1, c: 1}),
});

//
Expand All @@ -545,7 +555,8 @@ export function runGroupConversionToDistinctScanTests(database, isSharded = fals
assertPipelineResultsAndExplain({
pipeline: [{$match: {a: 1}}, {$sort: {a: 1, b: 1}}, {$group: {_id: "$b"}}],
expectedOutput: [{_id: 1}, {_id: 2}, {_id: 3}],
validateExplain: (explain) => assertPlanUsesDistinctScan(explain, {a: 1, b: 1, c: 1}),
validateExplain: (explain) =>
assertPlanUsesDistinctScan(database, explain, {a: 1, b: 1, c: 1}),
});

//
Expand All @@ -554,7 +565,8 @@ export function runGroupConversionToDistinctScanTests(database, isSharded = fals
assertPipelineResultsAndExplain({
pipeline: [{$match: {a: 1}}, {$group: {_id: "$b"}}],
expectedOutput: [{_id: 1}, {_id: 2}, {_id: 3}],
validateExplain: (explain) => assertPlanUsesDistinctScan(explain, {a: 1, b: 1, c: 1}),
validateExplain: (explain) =>
assertPlanUsesDistinctScan(database, explain, {a: 1, b: 1, c: 1}),
});

//
Expand Down Expand Up @@ -604,13 +616,15 @@ export function runGroupConversionToDistinctScanTests(database, isSharded = fals
assertPipelineResultsAndExplain({
pipeline: [{$sort: {a: -1, b: -1}}, {$group: {_id: "$a", accum: {$first: "$b"}}}],
expectedOutput: [{_id: null, accum: 1}, {_id: 1, accum: 3}, {_id: 2, accum: 2}],
validateExplain: (explain) => assertPlanUsesDistinctScan(explain, {a: 1, b: 1, c: 1}),
validateExplain: (explain) =>
assertPlanUsesDistinctScan(database, explain, {a: 1, b: 1, c: 1}),
});

assertPipelineResultsAndExplain({
pipeline: [{$sort: {a: -1, b: -1}}, {$group: {_id: "$a", accum: {$last: "$b"}}}],
expectedOutput: [{_id: null, accum: null}, {_id: 1, accum: 1}, {_id: 2, accum: 2}],
validateExplain: (explain) => assertPlanUsesDistinctScan(explain, {a: 1, b: 1, c: 1}),
validateExplain: (explain) =>
assertPlanUsesDistinctScan(database, explain, {a: 1, b: 1, c: 1}),
});

//
Expand Down Expand Up @@ -643,7 +657,8 @@ export function runGroupConversionToDistinctScanTests(database, isSharded = fals
{$group: {_id: "$a", f1: {$first: "$b"}, f2: {$first: "$b"}}}
],
expectedOutput: [{_id: null, f1: 1, f2: 1}, {_id: 1, f1: 3, f2: 3}, {_id: 2, f1: 2, f2: 2}],
validateExplain: (explain) => assertPlanUsesDistinctScan(explain, {a: 1, b: 1, c: 1}),
validateExplain: (explain) =>
assertPlanUsesDistinctScan(database, explain, {a: 1, b: 1, c: 1}),
});

assertPipelineResultsAndExplain({
Expand All @@ -653,7 +668,8 @@ export function runGroupConversionToDistinctScanTests(database, isSharded = fals
],
expectedOutput:
[{_id: null, fb: 1, fc: 1.5}, {_id: 1, fb: 3, fc: 2}, {_id: 2, fb: 2, fc: 2}],
validateExplain: (explain) => assertPlanUsesDistinctScan(explain, {a: 1, b: 1, c: 1}),
validateExplain: (explain) =>
assertPlanUsesDistinctScan(database, explain, {a: 1, b: 1, c: 1}),
});

//
Expand All @@ -664,7 +680,8 @@ export function runGroupConversionToDistinctScanTests(database, isSharded = fals
[{$sort: {a: -1, b: -1}}, {$group: {_id: "$a", l1: {$last: "$b"}, l2: {$last: "$b"}}}],
expectedOutput:
[{_id: null, l1: null, l2: null}, {_id: 1, l1: 1, l2: 1}, {_id: 2, l1: 2, l2: 2}],
validateExplain: (explain) => assertPlanUsesDistinctScan(explain, {a: 1, b: 1, c: 1}),
validateExplain: (explain) =>
assertPlanUsesDistinctScan(database, explain, {a: 1, b: 1, c: 1}),
});

assertPipelineResultsAndExplain({
Expand All @@ -674,7 +691,8 @@ export function runGroupConversionToDistinctScanTests(database, isSharded = fals
],
expectedOutput:
[{_id: null, lb: null, lc: null}, {_id: 1, lb: 1, lc: 1}, {_id: 2, lb: 2, lc: 2}],
validateExplain: (explain) => assertPlanUsesDistinctScan(explain, {a: 1, b: 1, c: 1}),
validateExplain: (explain) =>
assertPlanUsesDistinctScan(database, explain, {a: 1, b: 1, c: 1}),
});

//
Expand Down Expand Up @@ -712,7 +730,8 @@ export function runGroupConversionToDistinctScanTests(database, isSharded = fals
pipeline: [{$sort: {a: 1, b: 1}}, {$group: {_id: {v: "$a"}, accum: {$first: "$b"}}}],
expectedOutput:
[{_id: {v: null}, accum: null}, {_id: {v: 1}, accum: 1}, {_id: {v: 2}, accum: 2}],
validateExplain: (explain) => assertPlanUsesDistinctScan(explain, {a: 1, b: 1, c: 1}),
validateExplain: (explain) =>
assertPlanUsesDistinctScan(database, explain, {a: 1, b: 1, c: 1}),
});

//
Expand All @@ -723,7 +742,8 @@ export function runGroupConversionToDistinctScanTests(database, isSharded = fals
pipeline: [{$sort: {a: 1, b: 1}}, {$group: {_id: {v: "$a"}, accum: {$last: "$b"}}}],
expectedOutput:
[{_id: {v: null}, accum: 1}, {_id: {v: 1}, accum: 3}, {_id: {v: 2}, accum: 2}],
validateExplain: (explain) => assertPlanUsesDistinctScan(explain, {a: 1, b: 1, c: 1}),
validateExplain: (explain) =>
assertPlanUsesDistinctScan(database, explain, {a: 1, b: 1, c: 1}),
});

////////////////////////////////////////////////////////////////////////////////////////////////
Expand Down Expand Up @@ -815,7 +835,7 @@ export function runGroupConversionToDistinctScanTests(database, isSharded = fals
assertPipelineResultsAndExplain({
pipeline: [{$group: {_id: "$str"}}],
expectedOutput: [{_id: null}, {_id: "FoO"}, {_id: "bAr"}, {_id: "bar"}, {_id: "foo"}],
validateExplain: (explain) => assertPlanUsesDistinctScan(explain, {str: 1, d: 1}),
validateExplain: (explain) => assertPlanUsesDistinctScan(database, explain, {str: 1, d: 1}),
});

//
Expand Down
20 changes: 13 additions & 7 deletions jstests/libs/query/group_to_distinct_scan_utils.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
* Common utility functions variables for $group to DISTINCT_SCAN optimization.
*/

import {FeatureFlagUtil} from "jstests/libs/feature_flag_util.js";
import {getAggPlanStages, getQueryPlanners} from "jstests/libs/query/analyze_plan.js";

export let coll;
Expand Down Expand Up @@ -134,21 +135,26 @@ export function assertResultsMatchWithAndWithoutHintandIndexes(pipeline,
assert.sameMembers(resultsWithHint, expectedResults, "with hint != expected");
}

export function assertPlanUsesDistinctScan(explain, keyPattern, shouldFetch) {
assert.neq(0, getAggPlanStages(explain, "DISTINCT_SCAN").length, explain);
export function assertPlanUsesDistinctScan(testDB, explain, keyPattern, shouldFetch) {
const distinctScanStages = getAggPlanStages(explain, "DISTINCT_SCAN");
assert.neq(0, distinctScanStages.length, explain);
const distinctScan = distinctScanStages[0];

if (keyPattern) {
assert.eq(keyPattern, getAggPlanStages(explain, "DISTINCT_SCAN")[0].keyPattern, explain);
assert.eq(keyPattern, distinctScan.keyPattern, explain);
}

// Pipelines that use the DISTINCT_SCAN optimization should not also have a blocking sort.
assert.eq(0, getAggPlanStages(explain, "SORT").length, explain);

if (shouldFetch) {
// TODO SERVER-95215: Check that FETCH is pushed into DISTINCT_SCAN iff
// featureFlagShardFilteringDistinctScan is enabled.
const hasFetch = getAggPlanStages(explain, "FETCH").length > 0;
assert(hasFetch || getAggPlanStages(explain, "DISTINCT_SCAN")[0].isFetching);
// Check that FETCH is pushed into DISTINCT_SCAN iff featureFlagShardFilteringDistinctScan
// is enabled.
if (!FeatureFlagUtil.isEnabled(testDB, "ShardFilteringDistinctScan")) {
assert(getAggPlanStages(explain, "FETCH").length > 0);
} else {
assert(distinctScan.isFetching);
}
}
}

Expand Down
Loading

0 comments on commit 5f17ad2

Please sign in to comment.