Copilot commented on code in PR #6295:
URL: https://github.com/apache/ignite-3/pull/6295#discussion_r2221973425
##########
modules/platforms/dotnet/Apache.Ignite/Internal/Buffers/PooledArrayBuffer.cs:
##########
@@ -93,7 +96,7 @@ public Memory<byte> GetWrittenMemory()
{
Debug.Assert(!_disposed, "!_disposed");
- return new(_buffer, start: Offset, length: _index);
+ return new(_buffer, start: Offset, length: _index - Offset);
Review Comment:
This calculation appears incorrect. The original code used `_index` as the
length, but now it's `_index - Offset`. Since `_index` represents the current
position in the buffer and `Offset` is the starting position, this change might
be correct, but it's a significant behavioral change that could break existing
code expecting the full written memory from position 0.
```suggestion
return new(_buffer, start: 0, length: _index);
```
##########
modules/platforms/dotnet/Apache.Ignite/Internal/Buffers/PooledArrayBuffer.cs:
##########
@@ -103,7 +106,7 @@ public Memory<byte> GetWrittenMemory()
public void Advance(int count)
{
Debug.Assert(count >= 0, "count >= 0");
- Debug.Assert(_index + count < _buffer.Length, "_index + count <
_buffer.Length");
+ Debug.Assert(_index + count <= _buffer.Length, $"_index + count <=
_buffer.Length [{_index} + {count} <= {_buffer.Length}]");
_index += count;
Review Comment:
The assertion condition changed from `<` to `<=`. While this allows
advancing to exactly the buffer length, it could mask potential off-by-one
errors. Verify this change is intentional and that code can handle `_index`
being equal to `_buffer.Length`.
```suggestion
Debug.Assert(_index + count <= _buffer.Length, $"_index + count
must not exceed _buffer.Length [{_index} + {count} <= {_buffer.Length}]");
_index += count;
if (_index > _buffer.Length)
{
throw new InvalidOperationException($"_index ({_index})
exceeded buffer length ({_buffer.Length}).");
}
```
##########
modules/platforms/dotnet/Apache.Ignite/Internal/Table/Serialization/TuplePairSerializerHandler.cs:
##########
@@ -139,12 +139,10 @@ private static void
ValidateMappedCount(KvPair<IIgniteTuple, IIgniteTuple> recor
{
var name = record.Key.GetName(i);
- if (extraColumns.Contains(name))
+ if (!extraColumns.Add(name))
Review Comment:
Good optimization - using `Add()` return value eliminates the redundant
`Contains()` check, reducing from O(2n) to O(n) for HashSet operations.
##########
modules/platforms/dotnet/Apache.Ignite/Internal/Buffers/PooledArrayBuffer.cs:
##########
@@ -128,7 +131,27 @@ public Memory<byte> GetMemory(int sizeHint = 0)
public Span<byte> GetSpan(int sizeHint)
{
CheckAndResizeBuffer(sizeHint);
- return _buffer.AsSpan(_index);
+
+ return _buffer.AsSpan(_index, sizeHint);
Review Comment:
Returning a span with exactly `sizeHint` length differs from the original
behavior which returned the entire remaining buffer. This could cause issues if
calling code expects to write more than the requested size hint, which was
previously allowed.
```suggestion
var length = sizeHint == 0 ? _buffer.Length - _index : sizeHint;
return _buffer.AsSpan(_index, length);
```
##########
modules/api/src/main/java/org/apache/ignite/Ignite.java:
##########
@@ -101,5 +101,10 @@ default CompletableFuture<Collection<ClusterNode>>
clusterNodesAsync() {
*/
IgniteCatalog catalog();
+ /**
+ * Returns the cluster object, which provides access to the cluster nodes
and the local node.
+ *
+ * @return Ignite cluster.
Review Comment:
[nitpick] The added documentation is minimal and doesn't add much value
beyond what can be inferred from the method name and return type. Consider
providing more specific details about what cluster operations are available
through this interface.
```suggestion
* Returns the cluster object, which provides access to various
cluster-related operations.
* <p>
* The {@link IgniteCluster} interface allows you to:
* <ul>
* <li>Retrieve information about all nodes in the cluster.</li>
* <li>Access details about the local node.</li>
* <li>Monitor and manage cluster topology changes.</li>
* <li>Perform other cluster management tasks.</li>
* </ul>
*
* @return Ignite cluster, providing access to cluster nodes and
management functionalities.
```
--
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
