diff options
author | Jacob Evans <jacob.evans@10gen.com> | 2021-06-25 15:54:11 -0400 |
---|---|---|
committer | Jacob Evans <jacob.evans@10gen.com> | 2021-06-25 18:03:47 -0400 |
commit | 77ca833f40024000c351605ad7c96a0cc147fc20 (patch) | |
tree | 31780df846588d8d39171f4b2f3ff1d86315fc9a | |
parent | 3b919918b26883d38f80cc2c374ccd1207efa6e1 (diff) | |
download | mongo-77ca833f40024000c351605ad7c96a0cc147fc20.tar.gz |
SERVER-57629 Change $integral&$derivative args
14 files changed, 368 insertions, 139 deletions
diff --git a/jstests/aggregation/sources/setWindowFields/comprehensive_parse.js b/jstests/aggregation/sources/setWindowFields/comprehensive_parse.js new file mode 100644 index 00000000000..6c5632798b9 --- /dev/null +++ b/jstests/aggregation/sources/setWindowFields/comprehensive_parse.js @@ -0,0 +1,238 @@ +(function() { +"use strict"; + +const coll = db[jsTestName()]; +coll.drop(); + +const nDocs = 10; +let currentDate = new Date(); +for (let i = 0; i < nDocs; i++) { + assert.commandWorked(coll.insert({ + a: i, + date: currentDate, + partition: i % 2, + partitionSeq: Math.trunc(i / 2), + })); + + const nextDate = currentDate.getDate() + 1; + currentDate.setDate(nextDate); +} + +// The list of window functions to test. +const functions = { + sum: {$sum: '$a'}, + avg: {$avg: '$a'}, + stdDevSamp: {$stdDevSamp: '$a'}, + stdDevPop: {$stdDevPop: '$a'}, + min: {$min: '$a'}, + max: {$max: '$a'}, + count: {$count: {}}, + derivative: {$derivative: {input: '$a'}}, + derivative_date: {$derivative: {input: '$date', unit: 'millisecond'}}, + integral: {$integral: {input: '$a'}}, + integral_date: {$integral: {input: '$a', unit: 'millisecond'}}, + covariancePop: {$covariancePop: ['$a', '$date']}, + covarianceSamp: {$covarianceSamp: ['$a', '$date']}, + expMovingAvgN: {$expMovingAvg: {input: '$a', N: 3}}, + expMovingAvgAlpha: {$expMovingAvg: {input: '$a', alpha: 0.1}}, + push: {$push: '$a'}, + addToSet: {$addToSet: '$a'}, + first: {$first: '$a'}, + last: {$last: '$a'}, + shift: {$shift: {output: '$a', by: 1, default: 0}}, + documentNumber: {$documentNumber: {}}, + rank: {$rank: {}}, + denseRank: {$denseRank: {}} +}; + +// The list of window definitions to test. +const windows = { + none: null, + left_unbounded_doc: {documents: ['unbounded', 'current']}, + right_unbounded_doc: {documents: ['current', 'unbounded']}, + past_doc: {documents: [-1, 'current']}, + future_doc: {documents: ['current', 1]}, + centered_doc: {documents: [-1, 1]}, + full_unbounded_range: {range: ['unbounded', 'unbounded']}, + left_unbounded_range: {range: ['unbounded', 'current']}, + right_unbounded_range: {range: ['current', 'unbounded']}, + past_range: {range: [-2, 'current']}, + future_range: {range: ['current', 2]}, + centered_range: {range: [-2, 2]}, +}; + +// The list of sort definitions to test. +const sortBys = { + none: null, + asc: {partitionSeq: 1}, + desc: {partitionSeq: -1}, + asc_date: {date: 1}, + desc_date: {date: -1}, + multi: {partitionSeq: 1, partition: 1}, +}; + +// The list of partition definitions to test. +const partitionBys = { + none: null, + field: '$partition' +}; + +// Given an element from each of the lists above, construct a +// $setWindowFields stage. +function constructQuery(wf, window, sortBy, partitionBy) { + let pathArg = Object.assign({}, wf); + if (window != null) { + Object.assign(pathArg, {window: window}); + } + + let setWindowFieldsArg = {}; + + if (sortBy != null) { + setWindowFieldsArg.sortBy = sortBy; + } + + if (partitionBy != null) { + setWindowFieldsArg.partitionBy = partitionBy; + } + + setWindowFieldsArg.output = {x: pathArg}; + + return {$setWindowFields: setWindowFieldsArg}; +} + +// Given an element of each of the lists above, what is the expected +// result. The output should be 'SKIP', 'OK' or the expected integer +// error code. +function expectedResult(wfType, windowType, sortType, partitionType) { + // Skip range windows over dates or that are over descending windows. + if (windowType.endsWith('range')) { + if (sortType.endsWith('date') || sortType.startsWith('desc')) { + return 'SKIP'; + } + } + + // Derivative and integral require an ascending sort + // and an explicit window. + if (wfType.startsWith('derivative')) { + // Derivative requires a sort and an explicit window. + if (sortType == 'none' || windowType == 'none') { + return ErrorCodes.FailedToParse; + } + + // Integral requires single column sort + if (sortType == 'multi') { + return ErrorCodes.FailedToParse; + } + + if (wfType == 'derivative_date' && !sortType.endsWith('date')) { + // "$derivative with unit expects the sortBy field to be a Date". + return 5624900; + } + + if (sortType.endsWith('date') && wfType != 'derivative_date') { + // "$derivative where the sortBy is a Date requires a 'unit'. + return 5624901; + } + } else if (wfType.startsWith('integral')) { + // Integral requires a sort. + if (sortType == 'none') { + return ErrorCodes.FailedToParse; + } + + // Integral requires single column sort + if (sortType == 'multi') { + return ErrorCodes.FailedToParse; + } + + if (wfType == 'integral_date' && !sortType.endsWith('date')) { + // "$integral with unit expects the sortBy field to be a Date" + return 5423901; + } + + if (sortType.endsWith('date') && wfType != 'integral_date') { + // $integral where the sortBy is a Date requires a 'unit' + return 5423902; + } + } else if (wfType.startsWith('expMovingAvg')) { + // $expMovingAvg doesn't accept a window. + if (windowType != 'none') { + return ErrorCodes.FailedToParse; + } + + // $expMovingAvg requires a sort. + if (sortType == 'none') { + return ErrorCodes.FailedToParse; + } + } else if (wfType == 'documentNumber' || wfType == 'rank' || wfType == 'denseRank') { + if (windowType != 'none') { + // Rank style window functions take no other arguments. + return 5371601; + } + + if (sortType == 'none') { + // %s must be specified with a top level sortBy expression with exactly one element. + return 5371602; + } + + if (sortType == 'multi') { + // %s must be specified with a top level sortBy expression with exactly one element. + return 5371602; + } + } else if (wfType == 'shift') { + // $shift requires a sortBy and can't have defined a window. + if (sortType == 'none' || windowType != 'none') { + return ErrorCodes.FailedToParse; + } + } + + // Document windows require a sortBy. + if (windowType.endsWith('doc') && sortType == 'none') { + // 'Document-based bounds require a sortBy'. + return 5339901; + } + + // Range based windows require a sort over a single field. + if (windowType.endsWith('range') && (sortType == 'none' || sortType == 'multi')) { + // 'Range-based window require sortBy a single field'. + return 5339902; + } + + return ErrorCodes.OK; +} + +// Generate all combinations of the elements in the lists above, +// one element per list. +function* makeTests() { + for (const [wfType, wfDefinition] of Object.entries(functions)) { + for (const [windowType, windowDefinition] of Object.entries(windows)) { + for (const [sortType, sortDefinition] of Object.entries(sortBys)) { + for (const [partitionType, partitionDefinition] of Object.entries(partitionBys)) { + let test = { + query: constructQuery( + wfDefinition, windowDefinition, sortDefinition, partitionDefinition), + expectedResult: expectedResult(wfType, windowType, sortType, partitionType) + }; + + if (test.expectedResult == 'SKIP') { + continue; + } + + yield test; + } + } + } + } +} + +// Run all the combinations generated in makeTests. +for (const test of makeTests()) { + if (test.expectedResult == ErrorCodes.OK) { + assert.commandWorked( + coll.runCommand({aggregate: coll.getName(), pipeline: [test.query], cursor: {}})); + } else { + assert.commandFailedWithCode( + coll.runCommand({aggregate: coll.getName(), pipeline: [test.query], cursor: {}}), + test.expectedResult); + } +} +})(); diff --git a/jstests/aggregation/sources/setWindowFields/derivative.js b/jstests/aggregation/sources/setWindowFields/derivative.js index f125bf96872..fcacf07447b 100644 --- a/jstests/aggregation/sources/setWindowFields/derivative.js +++ b/jstests/aggregation/sources/setWindowFields/derivative.js @@ -167,9 +167,9 @@ assert.docEq(result, [ {time: 20, y: 30, dy: (30 - 12) / (20 - 10)}, ]); -// 'outputUnit' only supports 'week' and smaller. +// 'unit' only supports 'week' and smaller. coll.drop(); -function derivativeStage(outputUnit) { +function derivativeStage(unit) { const stage = { $setWindowFields: { sortBy: {time: 1}, @@ -183,15 +183,14 @@ function derivativeStage(outputUnit) { } } }; - if (outputUnit) { - stage.$setWindowFields.output.dy.$derivative.outputUnit = outputUnit; + if (unit) { + stage.$setWindowFields.output.dy.$derivative.unit = unit; } return stage; } -function explainUnit(outputUnit) { - return coll.runCommand({ - explain: {aggregate: coll.getName(), cursor: {}, pipeline: [derivativeStage(outputUnit)]} - }); +function explainUnit(unit) { + return coll.runCommand( + {explain: {aggregate: coll.getName(), cursor: {}, pipeline: [derivativeStage(unit)]}}); } assert.commandFailedWithCode(explainUnit('year'), 5490704); assert.commandFailedWithCode(explainUnit('quarter'), 5490704); @@ -203,7 +202,7 @@ assert.commandWorked(explainUnit('minute')); assert.commandWorked(explainUnit('second')); assert.commandWorked(explainUnit('millisecond')); -// When the time field is numeric, 'outputUnit' is not allowed. +// When the time field is numeric, 'unit' is not allowed. coll.drop(); assert.commandWorked(coll.insert([ {time: 0, y: 100}, @@ -219,7 +218,7 @@ assert.sameMembers(result, [ {time: 2, y: 100, dy: 0}, ]); -// When the time field is a Date, 'outputUnit' is required. +// When the time field is a Date, 'unit' is required. coll.drop(); assert.commandWorked(coll.insert([ {time: ISODate("2020-01-01T00:00:00.000Z"), y: 5}, @@ -240,7 +239,7 @@ assert.sameMembers(result, [ // The change per minute is 60*1000 larger than the change per millisecond. result = coll.aggregate([derivativeStage('minute'), {$unset: "_id"}]).toArray(); assert.sameMembers(result, [ - // 'outputUnit' applied to an ISODate expresses the output in terms of that unit. + // 'unit' applied to an ISODate expresses the output in terms of that unit. {time: ISODate("2020-01-01T00:00:00.000Z"), y: 5, dy: null}, {time: ISODate("2020-01-01T00:00:00.001Z"), y: 4, dy: -1 * 60 * 1000}, {time: ISODate("2020-01-01T00:00:00.002Z"), y: 6, dy: +2 * 60 * 1000}, @@ -262,7 +261,7 @@ result = coll.aggregate([ sortBy: {time: 1}, output: { dy: { - $derivative: {input: "$y", outputUnit: 'millisecond'}, + $derivative: {input: "$y", unit: 'millisecond'}, window: {documents: [-1, 0]} }, } @@ -279,7 +278,7 @@ assert.sameMembers(result, [ ]); // When the sortBy field is a mixture of dates and numbers, it's an error: -// whether or not you specify outputUnit, either the date or the number values +// whether or not you specify unit, either the date or the number values // will be an invalid type. coll.drop(); assert.commandWorked(coll.insert([ @@ -338,7 +337,7 @@ result = coll.aggregate([ sortBy: {time: 1}, output: { dy: { - $derivative: {input: "$y", outputUnit: 'second'}, + $derivative: {input: "$y", unit: 'second'}, window: { documents: ['unbounded', 'unbounded'], } @@ -372,7 +371,7 @@ result = coll.aggregate([ sortBy: {time: 1}, output: { dy: { - $derivative: {input: "$y", outputUnit: 'second'}, + $derivative: {input: "$y", unit: 'second'}, window: {range: [-10, 0], unit: 'second'} }, } diff --git a/jstests/aggregation/sources/setWindowFields/integral.js b/jstests/aggregation/sources/setWindowFields/integral.js index 196c3d463a8..a62033e2b4f 100644 --- a/jstests/aggregation/sources/setWindowFields/integral.js +++ b/jstests/aggregation/sources/setWindowFields/integral.js @@ -91,9 +91,9 @@ const resultDesc = coll.aggregate([ .toArray(); assert.sameMembers(result, resultDesc); -// 'outputUnit' only supports 'week' and smaller. +// 'unit' only supports 'week' and smaller. coll.drop(); -function explainUnit(outputUnit) { +function explainUnit(unit) { return coll.runCommand({ explain: { aggregate: coll.getName(), @@ -105,7 +105,7 @@ function explainUnit(outputUnit) { integral: { $integral: { input: "$y", - outputUnit: outputUnit, + unit: unit, }, window: {documents: [-1, 1]} }, @@ -125,7 +125,7 @@ assert.commandWorked(explainUnit('minute')); assert.commandWorked(explainUnit('second')); assert.commandWorked(explainUnit('millisecond')); -// Test if 'outputUnit' is specified. Date type input is supported. +// Test if 'unit' is specified. Date type input is supported. coll.drop(); assert.commandWorked(coll.insert([ {x: ISODate("2020-01-01T00:00:00.000Z"), y: 0}, @@ -134,19 +134,18 @@ assert.commandWorked(coll.insert([ {x: ISODate("2020-01-01T00:00:00.006Z"), y: 6}, ])); -const pipelineWithOutputUnit = [ +const pipelineWithUnit = [ { $setWindowFields: { sortBy: {x: 1}, output: { - integral: - {$integral: {input: "$y", outputUnit: 'second'}, window: {documents: [-1, 1]}}, + integral: {$integral: {input: "$y", unit: 'second'}, window: {documents: [-1, 1]}}, } } }, {$unset: "_id"}, ]; -result = coll.aggregate(pipelineWithOutputUnit).toArray(); +result = coll.aggregate(pipelineWithUnit).toArray(); assert.sameMembers(result, [ // We should scale the result by 'millisecond/second'. {x: ISODate("2020-01-01T00:00:00.000Z"), y: 0, integral: 0.002}, @@ -155,7 +154,7 @@ assert.sameMembers(result, [ {x: ISODate("2020-01-01T00:00:00.006Z"), y: 6, integral: 0.010}, ]); -const pipelineWithNoOutputUnit = [ +const pipelineWithNoUnit = [ { $setWindowFields: { sortBy: {x: 1}, @@ -166,8 +165,8 @@ const pipelineWithNoOutputUnit = [ }, {$unset: "_id"}, ]; -// 'outputUnit' is only valid if the 'sortBy' values are ISODate objects. -// Dates are only valid if 'outputUnit' is specified. +// 'unit' is only valid if the 'sortBy' values are ISODate objects. +// Dates are only valid if 'unit' is specified. coll.drop(); assert.commandWorked(coll.insert([ {x: 0, y: 100}, @@ -177,14 +176,14 @@ assert.commandWorked(coll.insert([ ])); assert.commandFailedWithCode(db.runCommand({ aggregate: "setWindowFields_integral", - pipeline: pipelineWithOutputUnit, + pipeline: pipelineWithUnit, cursor: {}, }), 5423901); assert.commandFailedWithCode(db.runCommand({ aggregate: "setWindowFields_integral", - pipeline: pipelineWithNoOutputUnit, + pipeline: pipelineWithNoUnit, cursor: {}, }), 5423902); @@ -234,7 +233,7 @@ function runRangeBasedIntegral(bounds) { sortBy: {time: 1}, output: { integral: { - $integral: {input: "$y", outputUnit: "second"}, + $integral: {input: "$y", unit: "second"}, window: {range: bounds, unit: "second"} }, } diff --git a/jstests/aggregation/sources/setWindowFields/window_functions_on_timeseries_coll.js b/jstests/aggregation/sources/setWindowFields/window_functions_on_timeseries_coll.js index a9de018218b..29b8717fdec 100644 --- a/jstests/aggregation/sources/setWindowFields/window_functions_on_timeseries_coll.js +++ b/jstests/aggregation/sources/setWindowFields/window_functions_on_timeseries_coll.js @@ -139,7 +139,7 @@ assertExplainBehaviorAndCorrectResults( posAvgTemp: {$avg: "$temperature", window: {documents: [-1, 1]}}, timeAvgTemp: {$avg: "$temperature", window: {range: [-5, 0], unit: "day"}}, tempRateOfChange: { - $derivative: {input: "$temperature", outputUnit: "hour"}, + $derivative: {input: "$temperature", unit: "hour"}, window: {documents: [-1, 0]} } } @@ -338,4 +338,4 @@ assertExplainBehaviorAndCorrectResults( {_id: 5, rank: 2}, {_id: 3, rank: 3}, ]); -})();
\ No newline at end of file +})(); diff --git a/src/mongo/db/pipeline/accumulator_for_window_functions.h b/src/mongo/db/pipeline/accumulator_for_window_functions.h index 058d45f62f2..c3205a70e2e 100644 --- a/src/mongo/db/pipeline/accumulator_for_window_functions.h +++ b/src/mongo/db/pipeline/accumulator_for_window_functions.h @@ -132,7 +132,7 @@ public: class AccumulatorIntegral : public AccumulatorForWindowFunctions { public: explicit AccumulatorIntegral(ExpressionContext* const expCtx, - boost::optional<long long> outputUnitMillis = boost::none); + boost::optional<long long> unitMillis = boost::none); void processInternal(const Value& input, bool merging) final; Value getValue(bool toBeMerged) final; @@ -141,7 +141,7 @@ public: const char* getOpName() const final; static boost::intrusive_ptr<AccumulatorState> create( - ExpressionContext* const expCtx, boost::optional<long long> outputUnitMillis = boost::none); + ExpressionContext* const expCtx, boost::optional<long long> unitMillis = boost::none); private: WindowFunctionIntegral _integralWF; diff --git a/src/mongo/db/pipeline/accumulator_integral.cpp b/src/mongo/db/pipeline/accumulator_integral.cpp index 83ba68c40d1..f17908ddeb2 100644 --- a/src/mongo/db/pipeline/accumulator_integral.cpp +++ b/src/mongo/db/pipeline/accumulator_integral.cpp @@ -41,9 +41,9 @@ namespace mongo { REGISTER_WINDOW_FUNCTION(integral, mongo::window_function::ExpressionIntegral::parse); AccumulatorIntegral::AccumulatorIntegral(ExpressionContext* const expCtx, - boost::optional<long long> outputUnitMillis) + boost::optional<long long> unitMillis) : AccumulatorForWindowFunctions(expCtx), - _integralWF(expCtx, outputUnitMillis, true /* isNonremovable */) { + _integralWF(expCtx, unitMillis, true /* isNonremovable */) { _memUsageBytes = sizeof(*this); } @@ -64,8 +64,8 @@ void AccumulatorIntegral::reset() { } boost::intrusive_ptr<AccumulatorState> AccumulatorIntegral::create( - ExpressionContext* const expCtx, boost::optional<long long> outputUnitMillis) { - return new AccumulatorIntegral(expCtx, outputUnitMillis); + ExpressionContext* const expCtx, boost::optional<long long> unitMillis) { + return new AccumulatorIntegral(expCtx, unitMillis); } const char* AccumulatorIntegral::getOpName() const { diff --git a/src/mongo/db/pipeline/window_function/window_function_exec.cpp b/src/mongo/db/pipeline/window_function/window_function_exec.cpp index 4619ef46bdd..56753a5252b 100644 --- a/src/mongo/db/pipeline/window_function/window_function_exec.cpp +++ b/src/mongo/db/pipeline/window_function/window_function_exec.cpp @@ -104,7 +104,7 @@ std::unique_ptr<mongo::WindowFunctionExec> translateDerivative( expr->expCtx()->variablesParseState); return std::make_unique<WindowFunctionExecDerivative>( - iter, expr->input(), sortExpr, expr->bounds(), expr->outputUnit(), memTracker); + iter, expr->input(), sortExpr, expr->bounds(), expr->unit(), memTracker); } diff --git a/src/mongo/db/pipeline/window_function/window_function_exec_derivative.cpp b/src/mongo/db/pipeline/window_function/window_function_exec_derivative.cpp index 1adfeb81dad..ec1b1300d43 100644 --- a/src/mongo/db/pipeline/window_function/window_function_exec_derivative.cpp +++ b/src/mongo/db/pipeline/window_function/window_function_exec_derivative.cpp @@ -42,36 +42,36 @@ Value WindowFunctionExecDerivative::getNext() { // Conceptually, $derivative computes 'rise/run' where 'rise' is dimensionless and 'run' is // a time. The result has dimension 1/time, which doesn't correspond to any BSON type, so - // 'outputUnit' tells us how to express the result as a dimensionless BSON number. + // 'unit' tells us how to express the result as a dimensionless BSON number. // // However, BSON also can't represent a time (duration) directly. BSONType::Date represents // a point in time, but there is no type that represents an amount of time. Subtracting two // Date values implicitly converts them to milliseconds. // So, when we compute 'rise/run', the answer is expressed in units '1/millisecond'. If an - // 'outputUnit' is specified, we scale the answer by 'millisecond/outputUnit' to - // re-express it in '1/outputUnit'. + // 'unit' is specified, we scale the answer by 'millisecond/unit' to + // re-express it in '1/unit'. Value leftTime = _time->evaluate(leftDoc, &_time->getExpressionContext()->variables); Value rightTime = _time->evaluate(rightDoc, &_time->getExpressionContext()->variables); - if (_outputUnitMillis) { - // If an outputUnit is specified, we require both endpoints to be dates. We don't + if (_unitMillis) { + // If a unit is specified, we require both endpoints to be dates. We don't // want to interpret bare numbers as milliseconds, when we don't know what unit they // really represent. // // For example: imagine the '_time' field contains floats representing seconds: then // 'rise/run' will already be expressed in units of 1/second. If you think "my data is - // seconds" and write 'outputUnit: "second"', and we applied the scale factor of - // 'millisecond/outputUnit', then the final answer would be wrong by a factor of 1000. + // seconds" and write 'unit: "second"', and we applied the scale factor of + // 'millisecond/unit', then the final answer would be wrong by a factor of 1000. uassert(5624900, - "$derivative with 'outputUnit' expects the sortBy field to be a Date", + "$derivative with 'unit' expects the sortBy field to be a Date", leftTime.getType() == BSONType::Date && rightTime.getType() == BSONType::Date); } else { - // Without outputUnit, we require both time values to be numeric. + // Without unit, we require both time values to be numeric. uassert(5624901, - "$derivative where the sortBy is a Date requires an 'outputUnit'", + "$derivative where the sortBy is a Date requires an 'unit'", leftTime.getType() != BSONType::Date && rightTime.getType() != BSONType::Date); uassert(5624902, - "$derivative (with no 'outputUnit') expects the sortBy field to be numeric", + "$derivative (with no 'unit') expects the sortBy field to be numeric", leftTime.numeric() && rightTime.numeric()); } // Now leftTime and rightTime are either both numeric, or both dates. @@ -92,13 +92,13 @@ Value WindowFunctionExecDerivative::getNext() { } Value result = uassertStatusOK(divideStatus); - if (_outputUnitMillis) { - // 'result' has units 1/millisecond; scale by millisecond/outputUnit to express in - // 1/outputUnit. + if (_unitMillis) { + // 'result' has units 1/millisecond; scale by millisecond/unit to express in + // 1/unit. // tassert because at this point the result should already be numeric, so if // ExpressionMultiply returns a non-OK Status then something has gone wrong. - auto statusWithResult = ExpressionMultiply::apply(result, Value(*_outputUnitMillis)); + auto statusWithResult = ExpressionMultiply::apply(result, Value(*_unitMillis)); tassert(statusWithResult); result = statusWithResult.getValue(); } diff --git a/src/mongo/db/pipeline/window_function/window_function_exec_derivative.h b/src/mongo/db/pipeline/window_function/window_function_exec_derivative.h index ed13364a18c..c9de780d443 100644 --- a/src/mongo/db/pipeline/window_function/window_function_exec_derivative.h +++ b/src/mongo/db/pipeline/window_function/window_function_exec_derivative.h @@ -56,18 +56,18 @@ public: boost::intrusive_ptr<Expression> position, boost::intrusive_ptr<Expression> time, WindowBounds bounds, - boost::optional<TimeUnit> outputUnit, + boost::optional<TimeUnit> unit, MemoryUsageTracker::PerFunctionMemoryTracker* memTracker) : WindowFunctionExec(PartitionAccessor(iter, PartitionAccessor::Policy::kEndpoints), memTracker), _position(std::move(position)), _time(std::move(time)), _bounds(std::move(bounds)), - _outputUnitMillis([&]() -> boost::optional<long long> { - if (!outputUnit) + _unitMillis([&]() -> boost::optional<long long> { + if (!unit) return boost::none; - auto status = timeUnitTypicalMilliseconds(*outputUnit); + auto status = timeUnitTypicalMilliseconds(*unit); tassert(status); return status.getValue(); }()) {} @@ -79,7 +79,7 @@ private: boost::intrusive_ptr<Expression> _position; boost::intrusive_ptr<Expression> _time; WindowBounds _bounds; - boost::optional<long long> _outputUnitMillis; + boost::optional<long long> _unitMillis; }; } // namespace mongo diff --git a/src/mongo/db/pipeline/window_function/window_function_exec_derivative_test.cpp b/src/mongo/db/pipeline/window_function/window_function_exec_derivative_test.cpp index 8968e474395..c0e2e2d29ee 100644 --- a/src/mongo/db/pipeline/window_function/window_function_exec_derivative_test.cpp +++ b/src/mongo/db/pipeline/window_function/window_function_exec_derivative_test.cpp @@ -74,15 +74,12 @@ public: Value eval(std::pair<Value, Value> start, std::pair<Value, Value> end, - boost::optional<TimeUnit> outputUnit = {}) { + boost::optional<TimeUnit> unit = {}) { const std::deque<DocumentSource::GetNextResult> docs{ Document{{"t", start.first}, {"y", start.second}}, Document{{"t", end.first}, {"y", end.second}}}; - auto mgr = createForFieldPath(std::move(docs), - "$y", - "$t", - {WindowBounds::DocumentBased{0, 1}}, - std::move(outputUnit)); + auto mgr = createForFieldPath( + std::move(docs), "$y", "$t", {WindowBounds::DocumentBased{0, 1}}, std::move(unit)); return mgr.getNext(); } @@ -265,7 +262,7 @@ TEST_F(WindowFunctionExecDerivativeTest, NonNumbers) { } TEST_F(WindowFunctionExecDerivativeTest, DatesAreNonNumbers) { - // When no outputUnit is specified, dates are considered an invalid type (an error). + // When no unit is specified, dates are considered an invalid type (an error). auto t0 = Value{Date_t::fromMillisSinceEpoch(0)}; auto t1 = Value{Date_t::fromMillisSinceEpoch(8)}; @@ -274,7 +271,7 @@ TEST_F(WindowFunctionExecDerivativeTest, DatesAreNonNumbers) { ASSERT_THROWS_CODE(eval({t0, y0}, {t1, y1}), DBException, 5624901); } -TEST_F(WindowFunctionExecDerivativeTest, OutputUnit) { +TEST_F(WindowFunctionExecDerivativeTest, Unit) { // 'y' increases by 1, over 8ms. auto t0 = Value{Date_t::fromMillisSinceEpoch(0)}; auto t1 = Value{Date_t::fromMillisSinceEpoch(8)}; @@ -296,8 +293,8 @@ TEST_F(WindowFunctionExecDerivativeTest, OutputUnit) { ASSERT_VALUE_EQ(Value{(1.0 / 8) * 1000 * 60 * 60 * 24 * 7}, calc(TimeUnit::week)); } -TEST_F(WindowFunctionExecDerivativeTest, OutputUnitNonDate) { - // outputUnit requires the time input to be a datetime: non-datetimes throw an error. +TEST_F(WindowFunctionExecDerivativeTest, UnitNonDate) { + // unit requires the time input to be a datetime: non-datetimes throw an error. auto t0 = Value{Date_t::fromMillisSinceEpoch(0)}; auto t1 = Value{Date_t::fromMillisSinceEpoch(1000)}; diff --git a/src/mongo/db/pipeline/window_function/window_function_expression.h b/src/mongo/db/pipeline/window_function/window_function_expression.h index 0cdc2f13456..6214ec4d338 100644 --- a/src/mongo/db/pipeline/window_function/window_function_expression.h +++ b/src/mongo/db/pipeline/window_function/window_function_expression.h @@ -413,28 +413,27 @@ protected: boost::optional<Decimal128> _alpha; }; -class ExpressionWithOutputUnit : public Expression { +class ExpressionWithUnit : public Expression { public: static constexpr StringData kArgInput = "input"_sd; - static constexpr StringData kArgOutputUnit = "outputUnit"_sd; + static constexpr StringData kArgUnit = "unit"_sd; - ExpressionWithOutputUnit(ExpressionContext* expCtx, - std::string accumulatorName, - boost::intrusive_ptr<::mongo::Expression> input, - WindowBounds bounds, - boost::optional<TimeUnit> outputUnit) - : Expression(expCtx, accumulatorName, std::move(input), std::move(bounds)), - _outputUnit(outputUnit) {} + ExpressionWithUnit(ExpressionContext* expCtx, + std::string accumulatorName, + boost::intrusive_ptr<::mongo::Expression> input, + WindowBounds bounds, + boost::optional<TimeUnit> unit) + : Expression(expCtx, accumulatorName, std::move(input), std::move(bounds)), _unit(unit) {} - boost::optional<TimeUnit> outputUnit() const { - return _outputUnit; + boost::optional<TimeUnit> unit() const { + return _unit; } Value serialize(boost::optional<ExplainOptions::Verbosity> explain) const final { MutableDocument result; result[_accumulatorName][kArgInput] = _input->serialize(static_cast<bool>(explain)); - if (_outputUnit) { - result[_accumulatorName][kArgOutputUnit] = Value(serializeTimeUnit(*_outputUnit)); + if (_unit) { + result[_accumulatorName][kArgUnit] = Value(serializeTimeUnit(*_unit)); } MutableDocument windowField; @@ -444,20 +443,20 @@ public: } protected: - static boost::optional<TimeUnit> parseOutputUnit(const BSONElement& arg) { - boost::optional<TimeUnit> outputUnit; + static boost::optional<TimeUnit> parseUnit(const BSONElement& arg) { + boost::optional<TimeUnit> unit; { uassert(ErrorCodes::FailedToParse, - str::stream() << kArgOutputUnit << "' must be a string, but got " << arg.type(), + str::stream() << kArgUnit << "' must be a string, but got " << arg.type(), arg.type() == String); - outputUnit = parseTimeUnit(arg.valueStringData()); - switch (*outputUnit) { + unit = parseTimeUnit(arg.valueStringData()); + switch (*unit) { // These larger time units vary so much, it doesn't make sense to define a // fixed conversion from milliseconds. (See 'timeUnitTypicalMilliseconds'.) case TimeUnit::year: case TimeUnit::quarter: case TimeUnit::month: - uasserted(5490704, "outputUnit must be 'week' or smaller"); + uasserted(5490704, "unit must be 'week' or smaller"); // Only these time units are allowed. case TimeUnit::week: case TimeUnit::day: @@ -468,7 +467,7 @@ protected: break; } } - return outputUnit; + return unit; } static void validateSortBy(const boost::optional<SortPattern>& sortBy, @@ -484,27 +483,26 @@ protected: !sortBy->begin()->expression); } - boost::optional<long long> convertTimeUnitToMillis(boost::optional<TimeUnit> outputUnit) const { - if (!outputUnit) + boost::optional<long long> convertTimeUnitToMillis(boost::optional<TimeUnit> unit) const { + if (!unit) return boost::none; - auto status = timeUnitTypicalMilliseconds(*outputUnit); + auto status = timeUnitTypicalMilliseconds(*unit); tassert(status); return status.getValue(); } - boost::optional<TimeUnit> _outputUnit; + boost::optional<TimeUnit> _unit; }; -class ExpressionDerivative : public ExpressionWithOutputUnit { +class ExpressionDerivative : public ExpressionWithUnit { public: ExpressionDerivative(ExpressionContext* expCtx, boost::intrusive_ptr<::mongo::Expression> input, WindowBounds bounds, - boost::optional<TimeUnit> outputUnit) - : ExpressionWithOutputUnit( - expCtx, "$derivative", std::move(input), std::move(bounds), outputUnit) {} + boost::optional<TimeUnit> unit) + : ExpressionWithUnit(expCtx, "$derivative", std::move(input), std::move(bounds), unit) {} static boost::intrusive_ptr<Expression> parse(BSONObj obj, const boost::optional<SortPattern>& sortBy, @@ -512,7 +510,7 @@ public: // { // $derivative: { // input: <expr>, - // outputUnit: <string>, // optional + // unit: <string>, // optional // } // window: {...} // optional // } @@ -543,13 +541,13 @@ public: derivativeArgs.type() == BSONType::Object); boost::intrusive_ptr<::mongo::Expression> input; - boost::optional<TimeUnit> outputUnit; + boost::optional<TimeUnit> unit; for (const auto& arg : derivativeArgs.Obj()) { auto argName = arg.fieldNameStringData(); if (argName == kArgInput) { input = ::mongo::Expression::parseOperand(expCtx, arg, expCtx->variablesParseState); - } else if (argName == kArgOutputUnit) { - outputUnit = parseOutputUnit(arg); + } else if (argName == kArgUnit) { + unit = parseUnit(arg); } else { uasserted(ErrorCodes::FailedToParse, str::stream() << "$derivative got unexpected argument: " << argName); @@ -564,7 +562,7 @@ public: bounds != boost::none); return make_intrusive<ExpressionDerivative>( - expCtx, std::move(input), std::move(*bounds), outputUnit); + expCtx, std::move(input), std::move(*bounds), unit); } boost::intrusive_ptr<AccumulatorState> buildAccumulatorOnly() const final { @@ -576,14 +574,13 @@ public: } }; -class ExpressionIntegral : public ExpressionWithOutputUnit { +class ExpressionIntegral : public ExpressionWithUnit { public: ExpressionIntegral(ExpressionContext* expCtx, boost::intrusive_ptr<::mongo::Expression> input, WindowBounds bounds, - boost::optional<TimeUnit> outputUnit) - : ExpressionWithOutputUnit( - expCtx, "$integral", std::move(input), std::move(bounds), outputUnit) {} + boost::optional<TimeUnit> unit) + : ExpressionWithUnit(expCtx, "$integral", std::move(input), std::move(bounds), unit) {} static boost::intrusive_ptr<Expression> parse(BSONObj obj, const boost::optional<SortPattern>& sortBy, @@ -591,7 +588,7 @@ public: // { // $integral: { // input: <expr>, - // outputUnit: <string>, // optional + // unit: <string>, // optional // } // window: {...} // optional // } @@ -625,16 +622,16 @@ public: integralArgs.type() == BSONType::Object); boost::intrusive_ptr<::mongo::Expression> input; - boost::optional<TimeUnit> outputUnit = boost::none; + boost::optional<TimeUnit> unit = boost::none; for (const auto& arg : integralArgs.Obj()) { auto argName = arg.fieldNameStringData(); if (argName == kArgInput) { input = ::mongo::Expression::parseOperand(expCtx, arg, expCtx->variablesParseState); - } else if (argName == kArgOutputUnit) { + } else if (argName == kArgUnit) { uassert(ErrorCodes::FailedToParse, - "There can be only one 'outputUnit' field for $integral", - outputUnit == boost::none); - outputUnit = parseOutputUnit(arg); + "There can be only one 'unit' field for $integral", + unit == boost::none); + unit = parseUnit(arg); } else { uasserted(ErrorCodes::FailedToParse, str::stream() << "$integral got unexpected argument: " << argName); @@ -643,15 +640,15 @@ public: uassert(ErrorCodes::FailedToParse, "$integral requires an 'input' expression", input); return make_intrusive<ExpressionIntegral>( - expCtx, std::move(input), bounds ? *bounds : WindowBounds(), outputUnit); + expCtx, std::move(input), bounds ? *bounds : WindowBounds(), unit); } boost::intrusive_ptr<AccumulatorState> buildAccumulatorOnly() const final { - return AccumulatorIntegral::create(_expCtx, convertTimeUnitToMillis(_outputUnit)); + return AccumulatorIntegral::create(_expCtx, convertTimeUnitToMillis(_unit)); } std::unique_ptr<WindowFunctionState> buildRemovable() const final { - return WindowFunctionIntegral::create(_expCtx, convertTimeUnitToMillis(_outputUnit)); + return WindowFunctionIntegral::create(_expCtx, convertTimeUnitToMillis(_unit)); } }; diff --git a/src/mongo/db/pipeline/window_function/window_function_integral.cpp b/src/mongo/db/pipeline/window_function/window_function_integral.cpp index d38e65d97cd..2ffe63d5998 100644 --- a/src/mongo/db/pipeline/window_function/window_function_integral.cpp +++ b/src/mongo/db/pipeline/window_function/window_function_integral.cpp @@ -63,13 +63,13 @@ void WindowFunctionIntegral::assertValueType(const Value& value) { (value.getArray()[0].numeric() || value.getArray()[0].getType() == BSONType::Date)); auto arr = value.getArray(); - if (_outputUnitMillis) { + if (_unitMillis) { uassert(5423901, - "$integral with 'outputUnit' expects the sortBy field to be a Date", + "$integral with 'unit' expects the sortBy field to be a Date", arr[0].getType() == BSONType::Date); } else { uassert(5423902, - "$integral (with no 'outputUnit') expects the sortBy field to be numeric", + "$integral (with no 'unit') expects the sortBy field to be numeric", arr[0].numeric()); } } diff --git a/src/mongo/db/pipeline/window_function/window_function_integral.h b/src/mongo/db/pipeline/window_function/window_function_integral.h index 10b970a4c5f..919c9030f2c 100644 --- a/src/mongo/db/pipeline/window_function/window_function_integral.h +++ b/src/mongo/db/pipeline/window_function/window_function_integral.h @@ -39,15 +39,14 @@ public: static inline const Value kDefault = Value{BSONNULL}; static std::unique_ptr<WindowFunctionState> create( - ExpressionContext* const expCtx, - boost::optional<long long> outputUnitMillis = boost::none) { - return std::make_unique<WindowFunctionIntegral>(expCtx, outputUnitMillis); + ExpressionContext* const expCtx, boost::optional<long long> unitMillis = boost::none) { + return std::make_unique<WindowFunctionIntegral>(expCtx, unitMillis); } explicit WindowFunctionIntegral(ExpressionContext* const expCtx, - boost::optional<long long> outputUnitMillis = boost::none, + boost::optional<long long> unitMillis = boost::none, bool isNonremovable = false) - : WindowFunctionState(expCtx), _integral(expCtx), _outputUnitMillis(outputUnitMillis) { + : WindowFunctionState(expCtx), _integral(expCtx), _unitMillis(unitMillis) { _memUsageBytes = sizeof(*this); } @@ -75,9 +74,9 @@ public: return Value(std::numeric_limits<double>::quiet_NaN()); - return _outputUnitMillis ? uassertStatusOK(ExpressionDivide::apply( - _integral.getValue(), Value(*_outputUnitMillis))) - : _integral.getValue(); + return _unitMillis + ? uassertStatusOK(ExpressionDivide::apply(_integral.getValue(), Value(*_unitMillis))) + : _integral.getValue(); } private: @@ -93,7 +92,7 @@ private: WindowFunctionSum _integral; std::deque<Value> _values; - boost::optional<long long> _outputUnitMillis; + boost::optional<long long> _unitMillis; int _nanCount = 0; bool isNonremovable = false; }; diff --git a/src/mongo/db/pipeline/window_function/window_function_integral_test.cpp b/src/mongo/db/pipeline/window_function/window_function_integral_test.cpp index 30dd7c47062..c929bfa478c 100644 --- a/src/mongo/db/pipeline/window_function/window_function_integral_test.cpp +++ b/src/mongo/db/pipeline/window_function/window_function_integral_test.cpp @@ -47,8 +47,8 @@ public: integral.add(val); } - void createWindowFunctionIntegralWithOutputUnit(long long kOutputUnitMillis) { - integral = WindowFunctionIntegral(expCtx.get(), kOutputUnitMillis); + void createWindowFunctionIntegralWithUnit(long long kUnitMillis) { + integral = WindowFunctionIntegral(expCtx.get(), kUnitMillis); } boost::intrusive_ptr<ExpressionContext> expCtx; @@ -202,15 +202,15 @@ TEST_F(WindowFunctionIntegralTest, ShouldWidenToDecimalOnlyIfNeeded) { ASSERT_TRUE(val.getType() == NumberDouble); } -TEST_F(WindowFunctionIntegralTest, CanHandleDateTypeWithOutputUnit) { +TEST_F(WindowFunctionIntegralTest, CanHandleDateTypeWithUnit) { const std::vector<Value> values = { Value(std::vector<Value>({Value(Date_t::fromMillisSinceEpoch(1000)), Value(0)})), Value(std::vector<Value>({Value(Date_t::fromMillisSinceEpoch(1002)), Value(2)})), Value(std::vector<Value>({Value(Date_t::fromMillisSinceEpoch(1004)), Value(4)})), }; - const long long kOutputUnitMillis = 1000; - createWindowFunctionIntegralWithOutputUnit(kOutputUnitMillis); + const long long kUnitMillis = 1000; + createWindowFunctionIntegralWithUnit(kUnitMillis); integral.add(values[0]); integral.add(values[1]); @@ -227,7 +227,7 @@ TEST_F(WindowFunctionIntegralTest, CanHandleDateTypeWithOutputUnit) { ASSERT_VALUE_EQ(integral.getValue(), Value(expectedIntegral)); } -TEST_F(WindowFunctionIntegralTest, DatesWithoutOutputUnitShouldFail) { +TEST_F(WindowFunctionIntegralTest, DatesWithoutUnitShouldFail) { const std::vector<Value> values = { Value(std::vector<Value>({Value(Date_t::fromMillisSinceEpoch(1000)), Value(2)})), Value(std::vector<Value>({Value(Date_t::fromMillisSinceEpoch(1002)), Value(4)})), @@ -237,7 +237,7 @@ TEST_F(WindowFunctionIntegralTest, DatesWithoutOutputUnitShouldFail) { ASSERT_THROWS_CODE(addValuesToWindow(values), AssertionException, 5423902); } -TEST_F(WindowFunctionIntegralTest, NumbersWithOutputUnitShouldFail) { +TEST_F(WindowFunctionIntegralTest, NumbersWithUnitShouldFail) { const std::vector<Value> values = { Value(std::vector<Value>({Value(3), Value(2)})), Value(std::vector<Value>({Value(5), Value(4)})), @@ -245,13 +245,13 @@ TEST_F(WindowFunctionIntegralTest, NumbersWithOutputUnitShouldFail) { Value(std::vector<Value>({Value(Date_t::fromMillisSinceEpoch(1002)), Value(4)})), }; - const long long kOutputUnitMillis = 1000; - createWindowFunctionIntegralWithOutputUnit(kOutputUnitMillis); + const long long kUnitMillis = 1000; + createWindowFunctionIntegralWithUnit(kUnitMillis); ASSERT_THROWS_CODE(addValuesToWindow(values), AssertionException, 5423901); } -TEST_F(WindowFunctionIntegralTest, ResetShouldNotResetOutputUnit) { +TEST_F(WindowFunctionIntegralTest, ResetShouldNotResetUnit) { const std::vector<Value> dateValues = { Value(std::vector<Value>({Value(Date_t::fromMillisSinceEpoch(1000)), Value(0)})), Value(std::vector<Value>({Value(Date_t::fromMillisSinceEpoch(1002)), Value(2)})), @@ -262,8 +262,8 @@ TEST_F(WindowFunctionIntegralTest, ResetShouldNotResetOutputUnit) { Value(std::vector<Value>({Value(2), Value(2)})), }; - const long long kOutputUnitMillis = 1000; - createWindowFunctionIntegralWithOutputUnit(kOutputUnitMillis); + const long long kUnitMillis = 1000; + createWindowFunctionIntegralWithUnit(kUnitMillis); addValuesToWindow(dateValues); double expectedIntegral = (0 + 2) * (1002 - 1000) / 2.0 / 1000.0; @@ -271,7 +271,7 @@ TEST_F(WindowFunctionIntegralTest, ResetShouldNotResetOutputUnit) { integral.reset(); - // Because 'outputUnit' is not reset and is still specified, dates input are expected. + // Because 'unit' is not reset and is still specified, dates input are expected. ASSERT_THROWS_CODE(addValuesToWindow(numericValues), AssertionException, 5423901); } |