Copilot commented on code in PR #5793:
URL: https://github.com/apache/ignite-3/pull/5793#discussion_r2081031417
##########
modules/platforms/dotnet/Apache.Ignite/Internal/Table/DataStreamerWithReceiver.cs:
##########
@@ -408,9 +408,32 @@ private static void WriteReceiverPayload<T>(ref
MsgPackWriter w, string classNam
using var builder = new BinaryTupleBuilder(binaryTupleSize);
builder.AppendString(className);
- builder.AppendObjectWithType(arg);
- builder.AppendObjectCollectionWithType(items);
+ if (arg is IIgniteTuple tupleArg)
+ {
+ builder.AppendInt(TupleWithSchemaMarshalling.TypeIdTuple);
+ builder.AppendInt(0); // Scale.
+ builder.AppendBytes(static (bufWriter, arg) =>
TupleWithSchemaMarshalling.Pack(bufWriter, arg), tupleArg);
+ }
+ else
+ {
+ builder.AppendObjectWithType(arg);
+ }
+
Review Comment:
[nitpick] Since this branch assumes that all items are of type IIgniteTuple,
consider adding a comment to document this assumption for better clarity and
maintainability.
```suggestion
// Assumption: If the first item in the collection is of type
IIgniteTuple,
// all items in the collection are also of type IIgniteTuple.
```
##########
modules/platforms/dotnet/Apache.Ignite.Tests/Compute/PlatformComputeTests.cs:
##########
@@ -216,6 +246,35 @@ public async Task TestManyJobsAssemblyLoadContextUnload()
Assert.AreEqual(2, assemblyLoadContextCount);
}
+ [Test]
+ public async Task TestTupleWithSchemaRoundTrip()
+ {
+ var tuple = TestCases.GetTupleWithAllFieldTypes();
+ tuple["nested_tuple"] = TestCases.GetTupleWithAllFieldTypes(x => x is
not decimal);
+
+ var expectedTuple = Enumerable.Range(0, tuple.FieldCount).Aggregate(
+ seed: new IgniteTuple(),
+ (acc, i) =>
+ {
+ acc[tuple.GetName(i)] = tuple[i] is decimal d ? new
BigDecimal(d) : tuple[i];
+ return acc;
+ });
+
+ var res = (IIgniteTuple)(await ExecJobAsync(DotNetJobs.Echo, tuple))!;
+
+ Assert.AreEqual(expectedTuple, res);
+ }
+
+ [Test]
+ public async Task TestDeepNestedTupleWithSchemaRoundTrip()
+ {
+ var tuple = TestCases.GetNestedTuple(100);
+ var res = await ExecJobAsync(DotNetJobs.Echo, tuple);
+
+ Assert.AreEqual(tuple, res);
+ StringAssert.Contains("CHILD99 = IgniteTuple { ID = 99, CHILD100 =
IgniteTuple { ID = 100 } } } } } } } }", res?.ToString());
Review Comment:
[nitpick] Hardcoding parts of the tuple's string representation may lead to
brittle tests if the formatting changes; consider deriving the expected string
from tuple properties or using a more robust equality check.
```suggestion
var expectedString = $"CHILD99 = IgniteTuple {{ ID = 99, CHILD100 =
IgniteTuple {{ ID = 100 }} }}";
StringAssert.Contains(expectedString, res?.ToString());
```
##########
modules/platforms/dotnet/Apache.Ignite/Internal/Compute/ComputePacker.cs:
##########
@@ -68,13 +69,14 @@ internal static void PackArgOrResult<T>(ref MsgPackWriter
w, T obj, IMarshaller<
return;
}
- if (obj is IIgniteTuple)
+ if (obj is IIgniteTuple tuple)
{
- // TODO: IGNITE-23033 .NET: Thin 3.0: Support tuples with schemas
in Compute
w.Write(Tuple);
Review Comment:
[nitpick] Consider adding a brief comment explaining that the lambda-based
call to TupleWithSchemaMarshalling.Pack replaces the previous unimplemented
branch, to aid future maintainers.
```suggestion
w.Write(Tuple);
// Serialize IIgniteTuple using TupleWithSchemaMarshalling.Pack.
This replaces a previously unimplemented branch.
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscr...@ignite.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
