CSHARP-4706: Support for $out to Time-series collections #1223

adelinowona · 2023-11-28T00:51:18Z

No description provided.

BorisDog

Looks great, some comments about testing.

src/MongoDB.Driver/PipelineDefinitionBuilder.cs

src/MongoDB.Driver/PipelineStageDefinitionBuilder.cs

tests/MongoDB.Driver.Core.Tests/Core/Operations/AggregateToCollectionOperationTests.cs

tests/MongoDB.Driver.Tests/Jira/CSharp3397Tests.cs

src/MongoDB.Driver/IAggregateFluent.cs

tests/MongoDB.Driver.Core.Tests/Core/Operations/AggregateToCollectionOperationTests.cs

BorisDog · 2023-12-04T19:21:38Z

src/MongoDB.Driver.Core/Core/Operations/AggregateToCollectionOperation.cs

@@ -330,7 +330,7 @@ private IReadOnlyList<BsonDocument> SimplifyOutStageIfOutputDatabaseIsSameAsInpu
        {
            var lastStage = pipeline.Last();
            var lastStageName = lastStage.GetElement(0).Name;
-            if (lastStageName == "$out" && lastStage["$out"] is BsonDocument outDocument)
+            if (lastStageName == "$out" && lastStage["$out"] is BsonDocument outDocument && !outDocument.Contains("timeseries"))


Do we have this tested directly or indirectly somewhere?

This is tested indirectly through the Aggregate_out_to_time_series_collection_on_secondary_should_work test and the AggregateToCollection time series tests. Without this, the simplification of the out stage will just remove the timeseries options resulting in the time series tests failing.

BorisDog · 2023-12-06T20:36:47Z

tests/MongoDB.Driver.Tests/PipelineDefinitionBuilderTests.cs

+        [Fact]
+        public void Out_with_time_series_options_should_add_expected_stage()
+        {
+            var client = DriverTestConfiguration.Client;


Looks like we just need the collection namespace for rendering.
Maybe we can just mock the collection, and then the test would not be dependent on server?

BorisDog · 2023-12-06T20:37:56Z

tests/MongoDB.Driver.Tests/PipelineStageDefinitionBuilderTests.cs

+
+            var result = PipelineStageDefinitionBuilder.Out(outputCollection, timeSeriesOptions);
+
+            RenderStage(result).Document.Should().Be("{ $out: { db: 'database', coll: 'collection', timeseries: { timeField: 'time', metaField: 'symbol' } } }");


Same comment about mocking collection.

BorisDog · 2023-12-06T20:38:22Z

src/MongoDB.Driver.Core/Core/Misc/Feature.cs

        private static readonly Feature __aggregateOutOnSecondary = new Feature("AggregateOutOnSecondary", WireVersion.Server50);
+        private static readonly Feature __aggregateOutTimeSeriesOnSecondary = new Feature("AggregateOutTimeSeriesOnSecondary", WireVersion.Server70);


Not sure that we need the secondary feature.

BorisDog · 2023-12-06T20:47:13Z

src/MongoDB.Driver/PipelineDefinitionBuilder.cs

+            IMongoCollection<TOutput> outputCollection,
+            TimeSeriesOptions timeSeriesOptions)
+        {
+            Ensure.IsNotNull(pipeline, nameof(pipeline));


We should either merge overloads (add timeSeriesOptions = null parameter) or forbid null here.
I personally would prefer having less overloads for simplicity.

Optional parameters are problematic and I think we should avoid them in our public API. We can use them internally where it makes sense, but not in our public API.

Reason 1: The optional value is inserted into the calling code at compile-time for the calling code. So if you change your default value, but don't recompile the calling code, you'll still use the same value.

Reason 2: Even more insidious is that the C# compiler doesn't enforce consistency among default values. The following is perfectly valid:

var oops1 = new EvilClass(); var oops2 = oops1 as IOops; Console.WriteLine(oops1.GetValue()); Console.WriteLine(oops2.GetValue()); interface IOops { int GetValue(int x = 42); } class EvilClass : IOops { public int GetValue(int x = 99) => x; }

The output is:

99 42

I see that we are already doing this elsewhere in our Fluent Aggregate API. Sigh. I guess we can use TimeSeriesOptions timeSeriesOptions = null here to reduce the number of overloads.

BorisDog · 2023-12-06T23:11:03Z

src/MongoDB.Driver.Core/Core/Misc/Feature.cs

@@ -40,6 +40,7 @@ public class Feature
        private static readonly Feature __aggregateLet = new Feature("AggregateLet", WireVersion.Server36);
        private static readonly Feature __aggregateMerge = new Feature("AggregateMerge", WireVersion.Server42);
        private static readonly Feature __aggregateOut = new Feature("AggregateOut", WireVersion.Server26);
+        private static readonly Feature __aggregateOutTimeSeries = new Feature("AggregateOutTimeSeries", WireVersion.Server70);


Alphabetical order (field and property).

JamesKovacs

While I don't like optional parameters (see comment below for why), we use them elsewhere. I'm fine with using an optional parameter for timeSeriesOptions and not adding more overloads. Fix that and some minor comment issues and you're good to go.

JamesKovacs · 2023-12-07T00:39:55Z

src/MongoDB.Driver/PipelineDefinitionBuilder.cs

+        /// <typeparam name="TOutput">The type of the output documents.</typeparam>
+        /// <param name="pipeline">The pipeline.</param>
+        /// <param name="outputCollection">The output collection.</param>
+        /// <param name="timeSeriesOptions">The time-series options</param>


time-series => time series

And add period at end of the param description.

JamesKovacs · 2023-12-07T00:57:20Z

src/MongoDB.Driver/PipelineDefinitionBuilder.cs

+            IMongoCollection<TOutput> outputCollection,
+            TimeSeriesOptions timeSeriesOptions)
+        {
+            Ensure.IsNotNull(pipeline, nameof(pipeline));


Optional parameters are problematic and I think we should avoid them in our public API. We can use them internally where it makes sense, but not in our public API.

Reason 1: The optional value is inserted into the calling code at compile-time for the calling code. So if you change your default value, but don't recompile the calling code, you'll still use the same value.

Reason 2: Even more insidious is that the C# compiler doesn't enforce consistency among default values. The following is perfectly valid:

var oops1 = new EvilClass(); var oops2 = oops1 as IOops; Console.WriteLine(oops1.GetValue()); Console.WriteLine(oops2.GetValue()); interface IOops { int GetValue(int x = 42); } class EvilClass : IOops { public int GetValue(int x = 99) => x; }

The output is:

99 42

JamesKovacs · 2023-12-07T00:59:09Z

src/MongoDB.Driver/PipelineStageDefinitionBuilder.cs

@@ -1244,17 +1244,36 @@ public static class PipelineStageDefinitionBuilder
        /// </summary>
        /// <typeparam name="TInput">The type of the input documents.</typeparam>
        /// <param name="outputCollection">The output collection.</param>
+        /// <param name="timeSeriesOptions">The time series options</param>


Period at end of sentence.

JamesKovacs · 2023-12-07T01:04:16Z

src/MongoDB.Driver/PipelineDefinitionBuilder.cs

+            IMongoCollection<TOutput> outputCollection,
+            TimeSeriesOptions timeSeriesOptions)
+        {
+            Ensure.IsNotNull(pipeline, nameof(pipeline));


I see that we are already doing this elsewhere in our Fluent Aggregate API. Sigh. I guess we can use TimeSeriesOptions timeSeriesOptions = null here to reduce the number of overloads.

src/MongoDB.Driver/PipelineStageDefinitionBuilder.cs

BorisDog

LGTM!

CSHARP-4706: Support for to Time-series collections

030b6cb

adelinowona requested a review from a team as a code owner November 28, 2023 00:51

adelinowona requested review from rstam, JamesKovacs and BorisDog and removed request for a team and rstam November 28, 2023 00:51

adelinowona added 3 commits November 30, 2023 16:00

Add tests for new $out timeseries option

ddab8bb

Cleanup

0f2978e

add version to run tests against

44a3fc4

BorisDog requested changes Dec 4, 2023

View reviewed changes

Fix tests and use Feature

6edcc4b

adelinowona changed the title ~~CSHARP-4706: Support for to Time-series collections~~ CSHARP-4706: Support for $out to Time-series collections Dec 6, 2023

Incorporate Review feedback

696c827

adelinowona requested a review from BorisDog December 6, 2023 18:16

BorisDog requested changes Dec 6, 2023

View reviewed changes

use mock for collection in tests and remove extra feature

1776038

BorisDog requested changes Dec 6, 2023

View reviewed changes

JamesKovacs requested changes Dec 7, 2023

View reviewed changes

Incorporate review feedback

659e1c9

adelinowona requested a review from BorisDog December 7, 2023 15:48

BorisDog approved these changes Dec 7, 2023

View reviewed changes

adelinowona merged commit 33d356a into mongodb:master Dec 7, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CSHARP-4706: Support for $out to Time-series collections #1223

CSHARP-4706: Support for $out to Time-series collections #1223

adelinowona commented Nov 28, 2023

BorisDog left a comment

BorisDog Dec 4, 2023

adelinowona Dec 6, 2023

BorisDog Dec 6, 2023

BorisDog Dec 6, 2023

BorisDog Dec 6, 2023

BorisDog Dec 6, 2023

JamesKovacs Dec 7, 2023

JamesKovacs Dec 7, 2023

BorisDog Dec 6, 2023

JamesKovacs left a comment

JamesKovacs Dec 7, 2023

JamesKovacs Dec 7, 2023

JamesKovacs Dec 7, 2023

JamesKovacs Dec 7, 2023

BorisDog left a comment


		var result = PipelineStageDefinitionBuilder.Out(outputCollection, timeSeriesOptions);

		RenderStage(result).Document.Should().Be("{ $out: { db: 'database', coll: 'collection', timeseries: { timeField: 'time', metaField: 'symbol' } } }");

		private static readonly Feature __aggregateOutOnSecondary = new Feature("AggregateOutOnSecondary", WireVersion.Server50);
		private static readonly Feature __aggregateOutTimeSeriesOnSecondary = new Feature("AggregateOutTimeSeriesOnSecondary", WireVersion.Server70);

CSHARP-4706: Support for $out to Time-series collections #1223

CSHARP-4706: Support for $out to Time-series collections #1223

Conversation

adelinowona commented Nov 28, 2023

BorisDog 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

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

JamesKovacs 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

Choose a reason for hiding this comment

BorisDog left a comment

Choose a reason for hiding this comment