Copilot commented on code in PR #6306:
URL: https://github.com/apache/ignite-3/pull/6306#discussion_r2225674470
##########
modules/platforms/dotnet/Apache.Ignite/Internal/Sql/Sql.cs:
##########
@@ -252,10 +306,51 @@ private static IgniteTuple
ReadTuple(IReadOnlyList<IColumnMetadata> cols, ref Bi
private static RowReader<T>
GetReaderFactory<T>(IReadOnlyList<IColumnMetadata> cols) =>
ResultSelector.Get<T>(cols, selectorExpression: null,
ResultSelectorOptions.None);
- private void WriteStatement(
+ private static void WriteBatchArgs(PooledArrayBuffer writer,
IEnumerable<IEnumerable<object>> args)
+ {
+ int rowSize = -1;
+ int rowCountPos = -1;
+ int rowCount = 0;
+
+ var w = writer.MessageWriter;
+
+ foreach (var arg in args)
+ {
+ IgniteArgumentCheck.NotNull(arg);
+ rowCount++;
+
+ ICollection<object> argCol = arg as ICollection<object> ??
arg.ToList();
+
+ if (rowSize < 0)
Review Comment:
[nitpick] Using -1 as a sentinel value for uninitialized rowSize makes the
code less readable. Consider using a boolean flag like 'isFirstRow' or checking
'rowCount == 1' instead.
```suggestion
if (isFirstRow)
```
##########
modules/platforms/dotnet/Apache.Ignite/Internal/Sql/Sql.cs:
##########
@@ -252,10 +306,51 @@ private static IgniteTuple
ReadTuple(IReadOnlyList<IColumnMetadata> cols, ref Bi
private static RowReader<T>
GetReaderFactory<T>(IReadOnlyList<IColumnMetadata> cols) =>
ResultSelector.Get<T>(cols, selectorExpression: null,
ResultSelectorOptions.None);
- private void WriteStatement(
+ private static void WriteBatchArgs(PooledArrayBuffer writer,
IEnumerable<IEnumerable<object>> args)
+ {
+ int rowSize = -1;
+ int rowCountPos = -1;
+ int rowCount = 0;
+
+ var w = writer.MessageWriter;
+
+ foreach (var arg in args)
+ {
+ IgniteArgumentCheck.NotNull(arg);
+ rowCount++;
+
+ ICollection<object> argCol = arg as ICollection<object> ??
arg.ToList();
Review Comment:
The ToList() call creates an unnecessary copy when the argument is already a
collection but not ICollection<object>. Consider checking for
IReadOnlyCollection<object> first or using Count() extension method to get the
count without materializing the entire collection.
```suggestion
IReadOnlyCollection<object> argCol = arg as
IReadOnlyCollection<object> ?? arg.ToList();
```
##########
modules/platforms/dotnet/Apache.Ignite/Internal/Sql/Sql.cs:
##########
@@ -252,10 +306,51 @@ private static IgniteTuple
ReadTuple(IReadOnlyList<IColumnMetadata> cols, ref Bi
private static RowReader<T>
GetReaderFactory<T>(IReadOnlyList<IColumnMetadata> cols) =>
ResultSelector.Get<T>(cols, selectorExpression: null,
ResultSelectorOptions.None);
- private void WriteStatement(
+ private static void WriteBatchArgs(PooledArrayBuffer writer,
IEnumerable<IEnumerable<object>> args)
+ {
+ int rowSize = -1;
+ int rowCountPos = -1;
+ int rowCount = 0;
+
+ var w = writer.MessageWriter;
+
+ foreach (var arg in args)
+ {
+ IgniteArgumentCheck.NotNull(arg);
+ rowCount++;
+
+ ICollection<object> argCol = arg as ICollection<object> ??
arg.ToList();
+
+ if (rowSize < 0)
+ {
+ // First row, write header.
+ rowSize = argCol.Count;
+
+ w.Write(rowSize);
+
+ rowCountPos = writer.ReserveMsgPackInt32();
+
+ w.Write(false); // Paged args.
+ }
+ else
+ {
+ if (rowSize != argCol.Count)
+ {
+ throw new ArgumentException($"Inconsistent batch
argument size: expected {rowSize}, got {argCol.Count}.");
+ }
+ }
+
+ w.WriteObjectCollectionAsBinaryTuple(argCol!);
+ }
+
+ IgniteArgumentCheck.Ensure(rowCount > 0, nameof(args), "Batch
arguments must not be empty.");
Review Comment:
The empty batch check happens after iterating through all arguments.
Consider checking if the enumerable is empty before starting the iteration to
fail fast and avoid unnecessary work.
--
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
