feat: support decimal #52
Conversation
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
WalkthroughAdds Protocol V3 with binary DECIMAL support (new BufferV3 and enum entries), extends IBuffer/ISender APIs with decimal and array overloads, updates Buffer implementations and senders (HTTP/TCP) for negotiation, TLS, and auth, migrates tests/helpers for decimal validation, and removes the legacy tcp-client-test project. Changes
Sequence Diagram(s)sequenceDiagram
participant Sender as ISender
participant Factory as Buffer.Create
participant BufferV3 as BufferV3
participant Encoder as Decimal Encoder
Note over Sender,Factory: Decimal column encoding path (Protocol V3)
Sender->>Factory: Create(version: V3/Auto)
Factory-->>BufferV3: returns BufferV3
Sender->>BufferV3: Column("price", 2615.54m)
BufferV3->>BufferV3: write BINARY_FORMAT_FLAG + DECIMAL type + scale
BufferV3->>Encoder: extract sign, scale, 96-bit mantissa
alt negative mantissa
Encoder->>Encoder: compute two's-complement mantissa
end
Encoder->>Encoder: trim sign-prefix bytes (preserve sign)
Encoder-->>BufferV3: provide length + mantissa bytes
BufferV3-->>Sender: fluent ISender (this)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used🧬 Code graph analysis (4)src/net-questdb-client-tests/BufferTests.cs (1)
src/net-questdb-client-tests/DecimalTestHelpers.cs (2)
src/net-questdb-client/Buffers/BufferV3.cs (3)
src/net-questdb-client-tests/TcpTests.cs (3)
🪛 Gitleaks (8.28.0)src/net-questdb-client-tests/TcpTests.cs[high] 938-938: Detected a Generic API Key, potentially exposing access to various services and sensitive operations. (generic-api-key) 🔇 Additional comments (5)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/dummy-http-server/DummyHttpServer.cs (1)
119-129: Add DECIMAL type handling to PrintBuffer method.The addition of protocol version 3 in
StartAsync()is correct and aligns with the decimal support goal. However, thePrintBuffer()method (lines 199-255) only handles type 14 (ARRAY) and type 16 (DOUBLE), and will throwNotImplementedExceptionfor the new DECIMAL type code (23). While theDecimalColumnstest avoids callingPrintBuffer()by working directly with raw bytes, the method should be updated to handle DECIMAL values for consistency and testability.Add a
case 23:handler in the switch statement (around line 254) to decode and display DECIMAL values in the same format as the client sends them (scale, mantissa length, and unscaled value bytes).
🧹 Nitpick comments (7)
src/net-questdb-client-tests/TcpTests.cs (2)
65-107: Great coverage for DECIMAL on TCP v3.Assertions match two’s‑complement big‑endian mantissas and null semantics. Consider adding a max-scale case (e.g., 28) and a small absolute value (e.g., 0.01m) to exercise compression edges.
1045-1060: Return only the first full line to harden the helper.Today the server buffers only one line, but future tests may queue multiple. Slice to the first ‘\n’ to keep the helper deterministic.
- var bytes = server.GetReceivedBytes(); - if (bytes.Length > 0 && bytes[^1] == (byte)'\n') - { - return bytes; - } + var bytes = server.GetReceivedBytes(); + if (bytes.Length > 0) + { + var nl = Array.IndexOf(bytes, (byte)'\n'); + if (nl >= 0) + return bytes.AsSpan(0, nl + 1).ToArray(); + }src/net-questdb-client/Buffers/BufferV2.cs (1)
114-140: Chunked copy loop looks correct; small edge‑guard suggestion.Logic handles partial copies and buffer rollover. To be extra defensive, short‑circuit when availLength is 0 (shouldn’t happen for doubles, but keeps the loop future‑proof):
- var availLength = dstLength - dstLength % byteSize; + var availLength = dstLength - dstLength % byteSize; + if (availLength == 0) { NextBuffer(); continue; }src/net-questdb-client-tests/HttpTests.cs (1)
87-137: ** HTTP DECIMAL test mirrors TCP path—good parity.**Validates pos/neg/null/min/max with protocol v3 and raw payload checks. Consider adding a boundary-scale case (28) to match TCP suggestions.
src/net-questdb-client-tests/DecimalTestHelpers.cs (1)
31-71: Helpful assertions; consider stricter field anchoring.Indexing by “columnName=” is fine for controlled tests. To avoid accidental matches in tags/names, optionally anchor on “ space or comma before name” when searching.
src/net-questdb-client-tests/JsonSpecTestRunner.cs (1)
84-87: Prefer non-obsolete timestamp API.AtNowAsync is obsolete and not dedup-safe. Consider AtAsync(DateTime.UtcNow) instead; keep pragma only if you must test server-side timestamping.
src/net-questdb-client/Senders/ISender.cs (1)
279-288: Add NullableColumn(int?) for API symmetry.You have long?/double?/bool?/DateTime?/DateTimeOffset? but not int?.
Apply this diff (non-breaking, compiles against existing Column(int)):
public ISender NullableColumn(ReadOnlySpan<char> name, long? value) { if (value != null) { return Column(name, value ?? throw new InvalidOperationException()); } return this; } + /// <summary /> + public ISender NullableColumn(ReadOnlySpan<char> name, int? value) + { + if (value != null) + { + return Column(name, value.Value); + } + return this; + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (27)
README.md(3 hunks)net-questdb-client.sln(1 hunks)src/dummy-http-server/DummyHttpServer.cs(7 hunks)src/example-aot/example-aot.csproj(1 hunks)src/net-questdb-client-benchmarks/BenchInserts.cs(1 hunks)src/net-questdb-client-benchmarks/net-questdb-client-benchmarks.csproj(1 hunks)src/net-questdb-client-tests/DecimalTestHelpers.cs(1 hunks)src/net-questdb-client-tests/DummyIlpServer.cs(9 hunks)src/net-questdb-client-tests/HttpTests.cs(49 hunks)src/net-questdb-client-tests/JsonSpecTestRunner.cs(6 hunks)src/net-questdb-client-tests/LineTcpSenderTests.cs(27 hunks)src/net-questdb-client-tests/TcpTests.cs(31 hunks)src/net-questdb-client/Buffers/Buffer.cs(1 hunks)src/net-questdb-client/Buffers/BufferV1.cs(9 hunks)src/net-questdb-client/Buffers/BufferV2.cs(5 hunks)src/net-questdb-client/Buffers/BufferV3.cs(1 hunks)src/net-questdb-client/Buffers/IBuffer.cs(1 hunks)src/net-questdb-client/Enums/BinaryFormatType.cs(1 hunks)src/net-questdb-client/Enums/ProtocolVersion.cs(1 hunks)src/net-questdb-client/Senders/AbstractSender.cs(2 hunks)src/net-questdb-client/Senders/HttpSender.cs(13 hunks)src/net-questdb-client/Senders/ISender.cs(13 hunks)src/net-questdb-client/Senders/ISenderV1.cs(1 hunks)src/net-questdb-client/Senders/ISenderV2.cs(1 hunks)src/tcp-client-test/DummyIlpServer.cs(0 hunks)src/tcp-client-test/JsonSpecTestRunner.cs(0 hunks)src/tcp-client-test/tcp-client-test.csproj(0 hunks)
💤 Files with no reviewable changes (3)
- src/tcp-client-test/DummyIlpServer.cs
- src/tcp-client-test/JsonSpecTestRunner.cs
- src/tcp-client-test/tcp-client-test.csproj
🧰 Additional context used
🪛 Gitleaks (8.28.0)
src/net-questdb-client-tests/TcpTests.cs
[high] 937-937: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🪛 LanguageTool
README.md
[grammar] ~154-~154: Ensure spelling is correct
Context: ...LS certificates should or should not be verifed. Options are on/unsafe_off. ...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~176-~176: Use a hyphen to join words.
Context: ... compatibility | ### net-questdb-client specific parameters | Name | ...
(QB_NEW_EN_HYPHEN)
🔇 Additional comments (32)
src/dummy-http-server/DummyHttpServer.cs (3)
183-186: LGTM!The method correctly uses the renamed
GetReceivedBytes()method, and variable names are clear and appropriate.
255-255: LGTM!The improved error message that includes the actual type value will be very helpful for debugging when new type codes are encountered.
146-149: No issues found. Method rename is complete and correct.The verification confirms that all references to the method have been updated successfully. The old method name
GetReceiveByteshas no remaining references in the codebase, and all call sites correctly useGetReceivedBytes(), including line 185 in the PrintBuffer method.src/example-aot/example-aot.csproj (1)
14-14: LGTM!The project reference path correction properly aligns with the repository reorganization.
README.md (4)
48-48: LGTM!The decimal literal correctly demonstrates the new decimal support feature.
63-63: LGTM!Consistent use of decimal literal in the second example.
163-163: LGTM!Protocol version 3 is properly documented with clear options.
169-174: LGTM!The protocol version behavior table clearly documents the differences and capabilities of each version.
net-questdb-client.sln (1)
22-22: LGTM!The example-aot project path update correctly reflects the repository reorganization.
src/net-questdb-client-benchmarks/BenchInserts.cs (1)
29-29: LGTM!The namespace update correctly reflects the test project consolidation from tcp-client-test to net-questdb-client-tests.
src/net-questdb-client/Enums/ProtocolVersion.cs (2)
43-43: LGTM!The enhanced XML documentation for V2 clarifies its support for ARRAY[DOUBLE] datatype.
47-50: LGTM!The V3 enum member is properly added with clear documentation indicating DECIMAL datatype support.
src/net-questdb-client/Enums/BinaryFormatType.cs (1)
34-34: LGTM!The DECIMAL binary format type is correctly added to support decimal encoding in protocol version 3.
src/net-questdb-client-tests/DummyIlpServer.cs (2)
231-234: LGTM!The
GetReceivedBytes()method is a useful addition for test verification, providing access to the raw received bytes alongside the existing text representation.
308-308: LGTM!The improved exception message provides better debugging information by including the unknown type value.
src/net-questdb-client-tests/LineTcpSenderTests.cs (2)
43-43: LGTM! Namespace rename aligns with test project consolidation.The namespace change from
tcp_client_testtonet_questdb_client_testsis consistent with the broader test project reorganization mentioned in the PR objectives.
59-62: LGTM! Formatting improvements enhance readability.The fluent API call indentation adjustments improve code readability without affecting functionality.
Also applies to: 82-85, 167-172, 199-204, 243-248, 258-263, 294-299
src/net-questdb-client/Senders/ISenderV1.cs (1)
29-34: LGTM! Marker interface for API versioning.The empty interface serves as a versioning boundary for the V1 Sender API. The minimal documentation is appropriate for a marker interface.
src/net-questdb-client/Senders/HttpSender.cs (1)
62-62: LGTM! Minor formatting adjustments.The spacing and formatting changes improve code consistency without affecting behavior.
Also applies to: 78-78, 83-83, 125-125, 131-131, 150-173, 218-218, 332-332, 366-368, 382-382, 407-407, 466-466, 484-486, 491-491, 529-529
src/net-questdb-client/Buffers/BufferV1.cs (2)
793-796: LGTM! Appropriate protocol version guard for DECIMAL support.The method correctly throws a
ProtocolVersionErrorindicating that DECIMAL types are not supported in Protocol Version V1, which aligns with the PR's introduction of decimal support in V3.
139-149: LGTM! Formatting and minor code improvements.The formatting adjustments and variable initialization improvements maintain consistency without changing behavior.
Also applies to: 167-170, 242-243, 397-410, 460-460, 477-479, 543-545
src/net-questdb-client/Buffers/IBuffer.cs (1)
237-240: LGTM! Well-documented interface method for decimal support.The new
Columnoverload fordecimal?values is properly documented and uses nullable types to support NULL database values. The XML documentation clearly describes the ILP binary decimal layout usage.src/net-questdb-client-benchmarks/net-questdb-client-benchmarks.csproj (1)
18-18: LGTM! Project reference update aligns with test consolidation.The change from
tcp-client-testtonet-questdb-client-testsis consistent with the broader test project reorganization mentioned in the PR objectives.src/net-questdb-client/Senders/ISenderV2.cs (1)
33-38: LGTM! Well-documented API versioning marker.The
ISenderV2interface serves as a clear versioning boundary, and the XML documentation helpfully specifies what features V2 adds (ARRAY and binary DOUBLE support).src/net-questdb-client/Senders/AbstractSender.cs (2)
97-102: LGTM! Convenient int overload with implicit conversion.The
Columnoverload forintvalues provides developer convenience and will safely convert tolongwhen forwarding toBuffer.Column(name, long).
323-327: LGTM! Decimal support in sender API.The
Columnoverload fordecimal?values properly forwards to the buffer implementation and maintains the fluent interface pattern, enabling decimal support at the sender level.src/net-questdb-client/Buffers/BufferV2.cs (1)
93-101: Direct write via MemoryMarshal.Cast is fine here.The write is unaligned-safe for primitive structs (double/int/long) and advances correctly. No issues spotted.
If you ever extend T beyond blittable primitives, add a guard or constrain usages to known types.
src/net-questdb-client/Buffers/BufferV3.cs (1)
52-122: Decimal encoding logic looks correct; add multi‑TFM fallback for GetBits(…, Span).The two’s‑complement, big‑endian build, and sign‑byte compression are sound and match tests. Two improvements:
- Portability: decimal.GetBits(value, Span) is available on newer TFMs. Add a fallback to the array‑returning overload to keep older TFMs compiling.
- Clarity: compute negativity from flags only; the extra
value != 0check is redundant for Decimal.Proposed patch:
- Span<int> parts = stackalloc int[4]; - decimal.GetBits(value.Value, parts); - - var flags = parts[3]; +#if NET8_0_OR_GREATER + Span<int> parts = stackalloc int[4]; + decimal.GetBits(value.Value, parts); + var flags = parts[3]; + var low = parts[0]; + var mid = parts[1]; + var high = parts[2]; +#else + var bits = decimal.GetBits(value.Value); + var flags = bits[3]; + var low = bits[0]; + var mid = bits[1]; + var high = bits[2]; +#endif - var low = parts[0]; - var mid = parts[1]; - var high = parts[2]; - var negative = (flags & SignMask) != 0 && value != 0; + var negative = (flags & SignMask) != 0;Run this to confirm TFMs allow the Span overload or need the fallback:
src/net-questdb-client-tests/TcpTests.cs (1)
931-933: Secret scanner finding: hardcoded keys in tests.These look like test fixtures, but they trigger gitleaks. Either:
- replace with obviously fake values (e.g., all-zero) or
- add an allowlist rule scoped to test paths and these exact strings.
Confirm no production secrets are committed.
Also applies to: 954-956, 971-973
src/net-questdb-client-tests/JsonSpecTestRunner.cs (2)
46-88: Nice consolidation with ExecuteTestCase.Centralizing per-case execution reduces duplication and keeps sender setup consistent.
255-257: Use MinimumProtocolVersion to gate tests.You always run with protocol_version=3. Skip tests requiring higher versions to avoid false failures.
Minimal insertion near the start of RunTcp/RunHttp:
@@ public async Task RunTcp(TestCase testCase) - { + { + if (testCase.MinimumProtocolVersion.HasValue && testCase.MinimumProtocolVersion.Value > 3) + Assert.Ignore($"Requires protocol v{testCase.MinimumProtocolVersion}, current v3."); @@ public async Task RunHttp(TestCase testCase) - { + { + if (testCase.MinimumProtocolVersion.HasValue && testCase.MinimumProtocolVersion.Value > 3) + Assert.Ignore($"Requires protocol v{testCase.MinimumProtocolVersion}, current v3.");src/net-questdb-client/Senders/ISender.cs (1)
334-338: Consider also exposing a non-nullable decimal overload.For consistency with other numerics and better overload resolution, add:
- ISender Column(ReadOnlySpan name, decimal value)
and optionally a default-interface NullableColumn(ReadOnlySpan, decimal?) delegating to it.
Note: this would require implementations to add the new method; plan for a follow-up to avoid breaking this PR.
|
@coderabbitai generate docstrings |
✅ Actions performedInitiated docstring generation; will generate only if new commits exist. |
|
Note Docstrings generation - SUCCESS |
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Docstrings generation was requested by @RaphDal. * #52 (comment) The following files were modified: * `src/dummy-http-server/DummyHttpServer.cs` * `src/net-questdb-client-tests/DecimalTestHelpers.cs` * `src/net-questdb-client-tests/DummyIlpServer.cs` * `src/net-questdb-client-tests/JsonSpecTestRunner.cs` * `src/net-questdb-client/Buffers/Buffer.cs` * `src/net-questdb-client/Buffers/BufferV1.cs` * `src/net-questdb-client/Buffers/BufferV2.cs` * `src/net-questdb-client/Buffers/BufferV3.cs` * `src/net-questdb-client/Buffers/IBuffer.cs` * `src/net-questdb-client/Senders/AbstractSender.cs` * `src/net-questdb-client/Senders/HttpSender.cs` * `src/net-questdb-client/Senders/ISender.cs`
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
README.md (1)
176-176: Fix hyphenation in section header.The section header should use consistent hyphenation.
-### net-questdb-client specific parameters +### net-questdb-client-specific parameters
🧹 Nitpick comments (1)
README.md (1)
195-196: Update Column method signatures to document decimal overloads.Given that this PR adds decimal support, the
Columnmethod documentation should include the newdecimalanddecimal?parameter types alongside the existing overloads. This ensures users understand the full API surface for writing decimal columns.-| `Column(ReadOnlySpan<char>, ReadOnlySpan<char> / string / long / double / DateTime / DateTimeOffset)` | `ISender` | Specify column name and value | -| `Column(ReadOnlySpan<char>, string? / long? / double? / DateTime? / DateTimeOffset?)` | `ISender` | Specify column name and value | +| `Column(ReadOnlySpan<char>, ReadOnlySpan<char> / string / long / double / decimal / DateTime / DateTimeOffset)` | `ISender` | Specify column name and value | +| `Column(ReadOnlySpan<char>, string? / long? / double? / decimal? / DateTime? / DateTimeOffset?)` | `ISender` | Specify column name and value |
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
README.md(3 hunks)
🧰 Additional context used
🪛 LanguageTool
README.md
[grammar] ~176-~176: Use a hyphen to join words.
Context: ... compatibility | ### net-questdb-client specific parameters | Name | ...
(QB_NEW_EN_HYPHEN)
🔇 Additional comments (2)
README.md (2)
48-48: Good addition of decimal examples.The C# decimal literal syntax (e.g.,
2615.54m) properly demonstrates the new decimal support in the basic and multi-line examples.Also applies to: 63-63
163-163: Good protocol version documentation.The new
protocol_versionparameter and Protocol Version table correctly document version 3 support for decimal.Also applies to: 169-174
… reliability in test case validation
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/net-questdb-client-tests/JsonSpecTestRunner.cs (1)
56-81: Address the unresolved parsing issues from previous review.The parsing logic still has three issues that were flagged previously:
- Line 71 (LONG): Using
GetDouble()and casting tolongloses precision for integers larger than 2^53. UseGetInt64()instead.- Lines 75-76 (DECIMAL): The null-forgiving operator
!is used without actually checking for null. If the JSON contains a null value, this will throw. Consider checkingJsonValueKindand handling null appropriately.- Line 59 (STRING):
GetString()can return null, which may cause issues depending on how the sender handles it. Consider explicit null handling or document the expected behavior.Apply this diff to address these issues:
switch (column.Type) { case "STRING": - sender.Column(column.Name, ((JsonElement)column.Value).GetString()); + var strValue = ((JsonElement)column.Value).GetString(); + sender.Column(column.Name, strValue); break; case "DOUBLE": sender.Column(column.Name, ((JsonElement)column.Value).GetDouble()); break; case "BOOLEAN": sender.Column(column.Name, ((JsonElement)column.Value).GetBoolean()); break; case "LONG": - sender.Column(column.Name, (long)((JsonElement)column.Value).GetDouble()); + sender.Column(column.Name, ((JsonElement)column.Value).GetInt64()); break; case "DECIMAL": - var d = decimal.Parse(((JsonElement)column.Value).GetString()!); - sender.Column(column.Name, d); + var jsonElement = (JsonElement)column.Value; + if (jsonElement.ValueKind == JsonValueKind.Null) + { + sender.Column(column.Name, (decimal?)null); + } + else + { + var d = decimal.Parse(jsonElement.GetString()!); + sender.Column(column.Name, d); + } break;
🧹 Nitpick comments (1)
src/net-questdb-client-tests/JsonSpecTestRunner.cs (1)
175-180: Consider adding wait logic for HTTP binary assertions to match TCP behavior.The TCP binary assertion (lines 117-121) uses
WaitAssertto ensure the full payload is received before comparing. The HTTP binary assertion here compares immediately without waiting. This inconsistency could lead to flaky tests if the HTTP server hasn't fully buffered the payload.Consider adding a similar wait mechanism for HTTP, or document why HTTP doesn't need it (e.g., if
DummyHttpServer.GetReceivedBytes()blocks until complete).If a wait is needed, you could add a helper similar to
WaitAssertforDummyHttpServer:private static void WaitAssert(DummyHttpServer server, byte[] expected) { var expectedLen = expected.Length; for (var i = 0; i < 500 && server.GetReceivedBytes().Length < expectedLen; i++) { Thread.Sleep(10); } Assert.That(server.GetReceivedBytes(), Is.EqualTo(expected)); }Then update the assertion:
if (testCase.Result.BinaryBase64 != null) { - var received = server.GetReceivedBytes(); var expected = Convert.FromBase64String(testCase.Result.BinaryBase64); - Assert.That(received, Is.EqualTo(expected)); + WaitAssert(server, expected); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/net-questdb-client-tests/JsonSpecTestRunner.cs(7 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/net-questdb-client-tests/JsonSpecTestRunner.cs (2)
src/net-questdb-client-tests/DummyIlpServer.cs (9)
Task(72-103)Task(110-155)Task(176-200)Task(208-224)AcceptAsync(67-70)GetReceivedBytes(231-234)PrintBuffer(236-318)DummyIlpServer(40-326)DummyIlpServer(52-57)src/net-questdb-client/Senders/AbstractSender.cs (18)
Task(58-61)Task(259-259)ISender(46-49)ISender(70-74)ISender(77-81)ISender(84-88)ISender(91-95)ISender(98-102)ISender(105-109)ISender(112-116)ISender(119-123)ISender(126-130)ISender(133-137)ISender(139-143)ISender(145-149)ISender(151-155)ISender(264-268)ISender(323-327)
🔇 Additional comments (1)
src/net-questdb-client-tests/JsonSpecTestRunner.cs (1)
238-247: LGTM!The binary
WaitAsserthelper correctly implements the pattern suggested in previous reviews. It waits for the full payload before asserting, which prevents race conditions in TCP binary assertions.
…vices in BufferV3
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
src/net-questdb-client/Buffers/BufferV3.cs (4)
57-59: Doc nit: scale range clarification.NET decimal scale is 0–28. If the protocol allows up to 76, note both to avoid confusion.
Apply this comment tweak:
- // 3. Scale: 1 byte (0-76 inclusive) - number of decimal places + // 3. Scale: 1 byte - number of decimal places (0–28 for .NET decimal; protocol may allow up to 76)
71-76: TFM compatibility: span-based Decimal.GetBitsdecimal.GetBits(decimal, Span) is only in newer TFMs. Verify your target frameworks; otherwise use the array-returning overload.
Portable fallback:
- Span<int> parts = stackalloc int[4]; - decimal.GetBits(value.Value, parts); - var flags = parts[3]; + int low, mid, high, flags; +#if NET8_0_OR_GREATER + Span<int> parts = stackalloc int[4]; + decimal.GetBits(value.Value, parts); + low = parts[0]; mid = parts[1]; high = parts[2]; flags = parts[3]; +#else + var partsArr = decimal.GetBits(value.Value); + low = partsArr[0]; mid = partsArr[1]; high = partsArr[2]; flags = partsArr[3]; +#endif
80-84: Minor clarity: nullable comparePrefer explicit non-nullable compare to avoid lifted-operator subtlety.
- var negative = (flags & SignMask) != 0 && value != 0; + var negative = (flags & SignMask) != 0 && value.Value != 0m;
85-93: Two’s complement of 96-bit mantissaCarry propagation logic is correct. Please add tests covering: -1m, -0m, Min/Max magnitudes, and random negative scales.
Happy to draft targeted unit tests if useful.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/net-questdb-client-tests/JsonSpecTestRunner.cs(7 hunks)src/net-questdb-client/Buffers/BufferV3.cs(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/net-questdb-client-tests/JsonSpecTestRunner.cs (3)
src/net-questdb-client/Senders/ISender.cs (18)
Task(84-84)Task(100-100)ISender(71-71)ISender(112-112)ISender(120-120)ISender(128-128)ISender(131-131)ISender(134-134)ISender(137-137)ISender(140-140)ISender(143-143)ISender(146-146)ISender(154-154)ISender(219-219)ISender(228-228)ISender(232-232)ISender(240-244)ISender(247-256)src/net-questdb-client-tests/DummyIlpServer.cs (9)
Task(72-103)Task(110-155)Task(176-200)Task(208-224)AcceptAsync(67-70)GetReceivedBytes(231-234)PrintBuffer(236-318)DummyIlpServer(40-326)DummyIlpServer(52-57)src/net-questdb-client-tests/LineTcpSenderTests.cs (2)
WaitAssert(626-635)DummyIlpServer(754-757)
src/net-questdb-client/Buffers/BufferV3.cs (2)
src/net-questdb-client/Buffers/BufferV2.cs (2)
BufferV2(33-265)BufferV2(36-38)src/net-questdb-client/Constants.cs (1)
Constants(29-32)
🔇 Additional comments (6)
src/net-questdb-client-tests/JsonSpecTestRunner.cs (3)
47-97: Good refactoring with proper type handling.The extraction of
ExecuteTestCaseimproves maintainability. The parsing issues flagged in the previous review have been addressed:
- ✅ LONG now uses
GetInt64()(line 72), avoiding precision loss.- ✅ DECIMAL handles null and uses
InvariantCulture(lines 75-86).- ℹ️ STRING (line 60):
GetString()returnsstring?, which is passed toColumn(string?). Per the ISender API, this overload skips null values. Since there's noNullableColumn<string>(it requiresT: struct), the current behavior is correct, though it differs from explicit null handling used for DECIMAL.
126-130: Binary comparison correctly implemented with wait mechanism.The addition of
WaitAssert(DummyIlpServer, byte[])and its use forBinaryBase64validation properly addresses the race condition flagged in the previous review. The wait loop ensures the full payload is received before assertion.Also applies to: 247-256
184-189: Dismiss the review concern—the HTTP binary assertion already waits viaawait ExecuteTestCase().The test at line 170 awaits
ExecuteTestCase(sender, testCase), which ensures the full HTTP request-response cycle completes before the assertion at lines 184-189 runs. The server-side handler appends bytes toIlpEndpoint.ReceiveBytessynchronously during request processing (line 121), and the response is only sent after that append. By the time the assertion executes, the bytes are guaranteed to be available. The assertion is not racy with respect to server-side data arrival.Likely an incorrect or invalid review comment.
src/net-questdb-client/Buffers/BufferV3.cs (3)
31-36: Solid V3 buffer scaffoldConstructor and inheritance wiring look correct and consistent with V2.
64-69: Confirm null encoding contract (scale=0, length=0)Ensure the server/decoder side treats length=0 as null (and not numeric zero). If not already covered, add a round‑trip test.
95-119: Prefix compression and payload write look correctSign-preserving strip is right, and length fits in a byte. Copy with EnsureCapacity/Advance matches existing buffering patterns.
Consider a quick round‑trip test asserting minimal byte length for 0m (1 byte) and -1m (1 byte) and increasing lengths near 2^8/2^16 boundaries.
📝 Add docstrings to `rd_decimal`
|
|
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
src/dummy-http-server/DummyHttpServer.cs (1)
289-297: Guard against index underflow and unify numeric formatting.
- Accessing
bytes[i - 1]wheni == 0can throw.- Double array values use current culture while scalar uses invariant; make both invariant for stable parsing in tests.
Suggested edits within
PrintBuffer():- if (bytes[i] == (byte)'=') - { - if (bytes[i - 1] == (byte)'=') + if (bytes[i] == (byte)'=' && i > 0 && bytes[i - 1] == (byte)'=') { ... - sb.Append(doubleBytes[j]); + sb.Append(doubleBytes[j].ToString(CultureInfo.InvariantCulture)); ... - sb.Append(doubleValue[0].ToString(CultureInfo.InvariantCulture)); + sb.Append(doubleValue[0].ToString(CultureInfo.InvariantCulture)); ... - default: - throw new NotImplementedException($"Type {bytes[i]} not implemented"); + default: + throw new NotImplementedException($"Type {bytes[i]} not implemented");src/net-questdb-client-tests/DummyIlpServer.cs (1)
228-232: Bug: random challenge buffer isn’t filled.
NextBytes(buffer.AsSpan(length))fills from offsetlengthto end (often empty), yielding an all‑zero challenge. This weakens the auth flow and can break interoperability.Apply this fix:
- rnd.NextBytes(buffer.AsSpan(length)); + rnd.NextBytes(buffer.AsSpan(0, length));src/net-questdb-client/Senders/HttpSender.cs (2)
126-131: Don’t add root CA to ClientCertificates.This sends the CA as a client cert. Use CustomTrustStore only; remove this block.
- if (!string.IsNullOrEmpty(Options.tls_roots)) - { - _handler.SslOptions.ClientCertificates ??= new X509Certificate2Collection(); - _handler.SslOptions.ClientCertificates.Add( - X509Certificate2.CreateFromPemFile(Options.tls_roots!, Options.tls_roots_password)); - }
115-121:CreateFromPemFilesecond arg is private key path, not a password; also roots usually don’t have keys.Load roots without a key; if encrypted key support is ever needed, use the proper API and only for client certs.
- chain!.ChainPolicy.TrustMode = X509ChainTrustMode.CustomRootTrust; - chain.ChainPolicy.CustomTrustStore.Add( - X509Certificate2.CreateFromPemFile(Options.tls_roots, Options.tls_roots_password)); + chain!.ChainPolicy.TrustMode = X509ChainTrustMode.CustomRootTrust; + chain.ChainPolicy.CustomTrustStore.Add( + X509Certificate2.CreateFromPemFile(Options.tls_roots!));If client auth via PEM is desired, wire that to Options.client_cert instead, using the key-aware APIs.
Also applies to: 128-131
src/net-questdb-client/Senders/ISender.cs (2)
118-129: Fix duplicated/incorrect XML docs for Column(ReadOnlySpan, ReadOnlySpan).There are two
blocks and the second mentions “string value” while the parameter is a char span. Merge into a single, accurate summary.
- /// <summary> - /// Adds a column (field) to the current row. - /// </summary> - /// <param name="name">The name of the column</param> - /// <param name="value">The value for the column</param> - /// <summary> - /// Adds a column (field) with the specified string value to the current row. - /// </summary> - /// <param name="name">The column name.</param> - /// <param name="value">The column value as a character span.</param> - /// <returns>The sender instance for fluent chaining.</returns> + /// <summary> + /// Adds a column (field) to the current row. + /// </summary> + /// <param name="name">The column name.</param> + /// <param name="value">The column value as a character span.</param> + /// <returns>The sender instance for fluent chaining.</returns> public ISender Column(ReadOnlySpan<char> name, ReadOnlySpan<char> value);
85-96: Correct SendAsync XML: method does not return request/response info.The docs claim it “will return request and response information” for HTTP, but the signature returns Task. Update wording to describe side effects (e.g., exceptions thrown, buffer cleared, LastFlush set).
- /// If the <see cref="SenderOptions.protocol" /> is HTTP, this will return request and response information. - /// <br /> - /// If the <see cref="SenderOptions.protocol" /> is TCP, this will return nulls. + /// For HTTP and TCP, this sends any pending rows. On success, implementations typically clear the buffer and update + /// <see cref="LastFlush"/>; on failure, an <see cref="IngressError"/> is thrown.
♻️ Duplicate comments (3)
src/net-questdb-client/Buffers/Buffer.cs (1)
35-43: Document and reconcileProtocolVersion.Autobehavior.
Create(..., Auto)returns BufferV3. Please document this explicitly in XML docs (param or remarks), since TCP maps Auto→V1 before creation; keep existing behavior but clarify to users. This was raised previously.- /// <param name="version">Protocol version that determines which concrete buffer implementation to create.</param> + /// <param name="version"> + /// Protocol version that determines which concrete buffer implementation to create. + /// When <see cref="ProtocolVersion.Auto"/> is specified, this method returns a <see cref="BufferV3"/> instance. + /// </param>Also applies to: 45-52
src/net-questdb-client-tests/JsonSpecTestRunner.cs (2)
61-92: Parsing fixes LGTM: LONG precision, DECIMAL null/culture, STRING null-safety now correct.
- LONG uses GetInt64 (no precision loss).
- DECIMAL uses InvariantCulture and handles null.
- STRING path is safe because Column(name, string?) is now null-tolerant.
136-141: Binary TCP assertion flakiness addressed.Switch to WaitAssert for binary and add byte[] overload. Good.
Also applies to: 261-270
🧹 Nitpick comments (10)
src/dummy-http-server/DummyHttpServer.cs (1)
175-180: Expose a defensive copy instead of the mutable buffer.Returning the live
List<byte>lets callers mutate server state inadvertently. Return a copy to avoid test-order coupling.- public List<byte> GetReceivedBytes() - { - return IlpEndpoint.ReceiveBytes; - } + public List<byte> GetReceivedBytes() + { + return new List<byte>(IlpEndpoint.ReceiveBytes); // defensive copy + }src/net-questdb-client-tests/DecimalTestHelpers.cs (2)
55-56: Avoid allocations in equality check.Use span comparison instead of ToArray() to skip two allocations.
- CollectionAssert.AreEqual(expectedMantissa.ToArray(), payload.Slice(4, expectedMantissa.Length).ToArray(), - $"Mantissa bytes for `{columnName}` did not match expectation."); + Assert.That(payload.Slice(4, expectedMantissa.Length).SequenceEqual(expectedMantissa), + Is.True, $"Mantissa bytes for `{columnName}` did not match expectation.");
85-91: Tighten column prefix match to reduce false positives.Searching for "columnName=" can hit values. Consider checking preceding byte is start-of-buffer, space, or comma.
- var index = buffer.IndexOf(prefix.AsSpan()); + var index = buffer.IndexOf(prefix.AsSpan()); + while (index >= 0 && index > 0) { + var prev = buffer[index - 1]; + if (prev == (byte)' ' || prev == (byte)',' ) break; + index = buffer.Slice(index + 1).IndexOf(prefix.AsSpan()); + if (index >= 0) index += 1; // compensate for slice + }src/net-questdb-client/Senders/HttpSender.cs (1)
186-189: Block-on-async in Build().
.Resulton ReadFromJsonAsync can deadlock. If Build must stay sync, preferGetAwaiter().GetResult()to avoid AggregateException wrapping and document why sync is required, or move negotiate into async init.- var json = response.Content.ReadFromJsonAsync<SettingsResponse>().Result!; + var json = response.Content.ReadFromJsonAsync<SettingsResponse>() + .GetAwaiter().GetResult()!;src/net-questdb-client-tests/JsonSpecTestRunner.cs (5)
99-103: Stop suppressing obsolete API; use AtAsync(DateTime.UtcNow).Replace AtNowAsync with AtAsync(DateTime.UtcNow) and drop pragma.
-#pragma warning disable CS0618 // Type or member is obsolete - await sender.AtNowAsync(); -#pragma warning restore CS0618 // Type or member is obsolete + await sender.AtAsync(DateTime.UtcNow);
110-118: Gate tests by MinimumProtocolVersion to avoid false failures on newer specs.Skip cases requiring a protocol above the configured v3.
public async Task RunTcp(TestCase testCase) { + if (testCase.MinimumProtocolVersion.HasValue && testCase.MinimumProtocolVersion > 3) + { + Assert.Ignore($"Requires protocol v{testCase.MinimumProtocolVersion}, runner is configured for v3."); + }public async Task RunHttp(TestCase testCase) { using var server = new DummyHttpServer(); await server.StartAsync(HttpPort); @@ - using var sender = Sender.New( - $"http::addr={IPAddress.Loopback}:{HttpPort};protocol_version=3"); + if (testCase.MinimumProtocolVersion.HasValue && testCase.MinimumProtocolVersion > 3) + { + Assert.Ignore($"Requires protocol v{testCase.MinimumProtocolVersion}, runner is configured for v3."); + } + using var sender = Sender.New($"http::addr={IPAddress.Loopback}:{HttpPort};protocol_version=3");Also applies to: 169-178
277-281: Ensure TestCaseSource is never null.Return an empty array when deserialization yields null to prevent discovery-time errors.
- private static TestCase[]? ReadTestCases() + private static TestCase[] ReadTestCases() { using var jsonFile = File.OpenRead("ilp-client-interop-test.json"); - return JsonSerializer.Deserialize<TestCase[]>(jsonFile); + return JsonSerializer.Deserialize<TestCase[]>(jsonFile) ?? Array.Empty<TestCase>(); }
55-63: Optional: be defensive against missing arrays in JSON.Treat null Symbols/Columns as empty to avoid NREs if a spec omits them.
- foreach (var symbol in testCase.Symbols) + foreach (var symbol in testCase.Symbols ?? Array.Empty<TestCaseSymbol>()) { sender.Symbol(symbol.Name, symbol.Value); } @@ - foreach (var column in testCase.Columns) + foreach (var column in testCase.Columns ?? Array.Empty<TestCaseColumn>()) { switch (column.Type)Also applies to: 61-97
124-132: Nit: avoid nullable in catch; the exception is never null.Use
Exception exfor clarity.- catch (Exception? ex) + catch (Exception ex) {Also applies to: 185-194
src/net-questdb-client/Senders/ISender.cs (1)
339-411: Simplify NullableColumn overloads; drop redundant?? throw.Inside
if (value != null)blocks, use.Value. This is clearer and avoids needless null coalescing.public ISender NullableColumn(ReadOnlySpan<char> name, long? value) { if (value != null) { - return Column(name, value ?? throw new InvalidOperationException()); + return Column(name, value.Value); } return this; } @@ public ISender NullableColumn(ReadOnlySpan<char> name, bool? value) { if (value != null) { - return Column(name, value ?? throw new InvalidOperationException()); + return Column(name, value.Value); } return this; } @@ public ISender NullableColumn(ReadOnlySpan<char> name, double? value) { if (value != null) { - return Column(name, value ?? throw new InvalidOperationException()); + return Column(name, value.Value); } return this; } @@ public ISender NullableColumn(ReadOnlySpan<char> name, DateTime? value) { if (value != null) { - return Column(name, value ?? throw new InvalidOperationException()); + return Column(name, value.Value); } return this; } @@ public ISender NullableColumn(ReadOnlySpan<char> name, DateTimeOffset? value) { if (value != null) { - return Column(name, value ?? throw new InvalidOperationException()); + return Column(name, value.Value); } return this; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
src/dummy-http-server/DummyHttpServer.cs(8 hunks)src/net-questdb-client-tests/DecimalTestHelpers.cs(1 hunks)src/net-questdb-client-tests/DummyIlpServer.cs(10 hunks)src/net-questdb-client-tests/JsonSpecTestRunner.cs(7 hunks)src/net-questdb-client/Buffers/Buffer.cs(1 hunks)src/net-questdb-client/Buffers/BufferV1.cs(12 hunks)src/net-questdb-client/Buffers/BufferV2.cs(5 hunks)src/net-questdb-client/Buffers/BufferV3.cs(1 hunks)src/net-questdb-client/Buffers/IBuffer.cs(1 hunks)src/net-questdb-client/Senders/AbstractSender.cs(3 hunks)src/net-questdb-client/Senders/HttpSender.cs(22 hunks)src/net-questdb-client/Senders/ISender.cs(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/net-questdb-client/Senders/AbstractSender.cs
🧰 Additional context used
🧬 Code graph analysis (11)
src/net-questdb-client/Buffers/IBuffer.cs (3)
src/net-questdb-client/Buffers/Buffer.cs (1)
IBuffer(43-53)src/net-questdb-client/Buffers/BufferV1.cs (16)
IBuffer(74-92)IBuffer(247-267)IBuffer(270-290)IBuffer(293-303)IBuffer(306-312)IBuffer(315-324)IBuffer(327-336)IBuffer(339-349)IBuffer(352-361)IBuffer(364-373)IBuffer(376-391)IBuffer(413-444)IBuffer(447-450)IBuffer(453-456)IBuffer(459-463)ReadOnlySpan(186-189)src/net-questdb-client-tests/DecimalTestHelpers.cs (1)
ReadOnlySpan(85-91)
src/net-questdb-client/Buffers/BufferV3.cs (3)
src/net-questdb-client/Buffers/BufferV2.cs (2)
BufferV2(33-286)BufferV2(36-38)src/net-questdb-client/Buffers/IBuffer.cs (18)
IBuffer(62-62)IBuffer(70-70)IBuffer(80-80)IBuffer(93-93)IBuffer(101-101)IBuffer(109-109)IBuffer(117-117)IBuffer(125-125)IBuffer(133-133)IBuffer(141-141)IBuffer(149-149)IBuffer(217-217)IBuffer(220-220)IBuffer(226-226)IBuffer(229-229)IBuffer(237-237)ReadOnlySpan(198-198)Put(223-223)src/net-questdb-client/Constants.cs (1)
Constants(29-32)
src/net-questdb-client-tests/JsonSpecTestRunner.cs (7)
src/net-questdb-client/Senders/HttpSender.cs (4)
Task(294-311)Task(492-506)Task(509-623)ISender(255-271)src/net-questdb-client/Senders/ISender.cs (17)
Task(80-80)Task(96-96)ISender(67-67)ISender(108-108)ISender(116-116)ISender(129-129)ISender(137-137)ISender(140-140)ISender(148-148)ISender(156-156)ISender(164-164)ISender(172-172)ISender(180-180)ISender(253-253)ISender(261-261)ISender(269-269)ISender(277-281)src/dummy-http-server/DummyHttpServer.cs (6)
Task(141-151)Task(156-159)Task(161-164)Task(189-193)Exception(184-187)PrintBuffer(225-307)src/net-questdb-client-tests/DummyIlpServer.cs (9)
Task(83-114)Task(126-171)Task(202-226)Task(234-250)AcceptAsync(72-75)GetReceivedBytes(265-268)PrintBuffer(281-363)DummyIlpServer(40-377)DummyIlpServer(57-62)src/net-questdb-client/Sender.cs (1)
Sender(48-96)src/net-questdb-client-tests/LineTcpSenderTests.cs (2)
WaitAssert(626-635)DummyIlpServer(754-757)src/net-questdb-client-tests/TcpTests.cs (2)
WaitAssert(1029-1038)DummyIlpServer(1040-1043)
src/net-questdb-client-tests/DecimalTestHelpers.cs (2)
src/net-questdb-client/Buffers/IBuffer.cs (1)
ReadOnlySpan(198-198)src/net-questdb-client/Buffers/BufferV1.cs (1)
ReadOnlySpan(186-189)
src/net-questdb-client-tests/DummyIlpServer.cs (4)
src/net-questdb-client-tests/JsonSpecTestRunner.cs (1)
DummyIlpServer(272-275)src/net-questdb-client/Senders/TcpSender.cs (1)
FromBase64String(188-198)src/net-questdb-client/LineTcpSender.cs (1)
FromBase64String(478-488)src/dummy-http-server/DummyHttpServer.cs (2)
PrintBuffer(225-307)StringBuilder(170-173)
src/net-questdb-client/Buffers/BufferV1.cs (3)
src/net-questdb-client/Buffers/IBuffer.cs (19)
Clear(184-184)CancelRow(195-195)IBuffer(62-62)IBuffer(70-70)IBuffer(80-80)IBuffer(93-93)IBuffer(101-101)IBuffer(109-109)IBuffer(117-117)IBuffer(125-125)IBuffer(133-133)IBuffer(141-141)IBuffer(149-149)IBuffer(217-217)IBuffer(220-220)IBuffer(226-226)IBuffer(229-229)IBuffer(237-237)ReadOnlySpan(198-198)src/net-questdb-client/Buffers/Buffer.cs (1)
Buffer(32-54)src/net-questdb-client/Utils/IngressError.cs (3)
IngressError(35-66)IngressError(42-46)IngressError(54-58)
src/net-questdb-client/Buffers/Buffer.cs (3)
src/net-questdb-client/Buffers/BufferV1.cs (2)
BufferV1(34-851)BufferV1(50-56)src/net-questdb-client/Buffers/BufferV2.cs (2)
BufferV2(33-286)BufferV2(36-38)src/net-questdb-client/Buffers/BufferV3.cs (2)
BufferV3(31-132)BufferV3(39-41)
src/net-questdb-client/Senders/ISender.cs (4)
src/net-questdb-client/Senders/HttpSender.cs (1)
ISender(255-271)src/net-questdb-client/Senders/AbstractSender.cs (16)
ISender(46-49)ISender(70-74)ISender(77-81)ISender(84-88)ISender(91-95)ISender(103-107)ISender(110-114)ISender(117-121)ISender(124-128)ISender(131-135)ISender(138-142)ISender(144-148)ISender(150-154)ISender(156-160)ISender(269-273)Clear(255-258)src/net-questdb-client/Buffers/IBuffer.cs (2)
ReadOnlySpan(198-198)Clear(184-184)src/net-questdb-client/Buffers/BufferV1.cs (2)
ReadOnlySpan(186-189)Clear(142-158)
src/dummy-http-server/DummyHttpServer.cs (3)
src/dummy-http-server/IlpEndpoint.cs (2)
IlpEndpoint(72-151)Task(102-129)src/net-questdb-client-tests/DummyIlpServer.cs (5)
Task(83-114)Task(126-171)Task(202-226)Task(234-250)PrintBuffer(281-363)src/dummy-http-server/SettingsEndpoint.cs (1)
SettingsEndpoint(6-27)
src/net-questdb-client/Senders/HttpSender.cs (7)
src/net-questdb-client/Utils/SenderOptions.cs (4)
SenderOptions(90-92)SenderOptions(98-123)SenderOptions(668-674)ToString(594-639)src/net-questdb-client/Senders/TcpSender.cs (2)
Build(59-119)Task(229-252)src/net-questdb-client/Buffers/BufferStreamContent.cs (2)
BufferStreamContent(33-75)BufferStreamContent(35-38)src/net-questdb-client/Buffers/Buffer.cs (1)
Buffer(32-54)src/net-questdb-client/Utils/IngressError.cs (3)
IngressError(35-66)IngressError(42-46)IngressError(54-58)src/net-questdb-client/Buffers/BufferV1.cs (1)
Task(192-212)src/net-questdb-client/Senders/AbstractSender.cs (2)
Task(58-61)Task(264-264)
src/net-questdb-client/Buffers/BufferV2.cs (2)
src/net-questdb-client/Buffers/IBuffer.cs (16)
ReadOnlySpan(198-198)IBuffer(62-62)IBuffer(70-70)IBuffer(80-80)IBuffer(93-93)IBuffer(101-101)IBuffer(109-109)IBuffer(117-117)IBuffer(125-125)IBuffer(133-133)IBuffer(141-141)IBuffer(149-149)IBuffer(217-217)IBuffer(220-220)IBuffer(226-226)IBuffer(229-229)src/net-questdb-client/Buffers/BufferV1.cs (2)
ReadOnlySpan(186-189)NextBuffer(632-648)
🔇 Additional comments (8)
src/dummy-http-server/DummyHttpServer.cs (1)
120-127: Resetting counter in Clear() is good.Resets all relevant state, including
Counter. LGTM.src/net-questdb-client-tests/DummyIlpServer.cs (4)
91-101: Stream layering LGTM.Using
dataStreamto wrapSslStreamand reuse the same path for auth and data is clean.Also applies to: 104-105
184-195: Base64 normalization looks correct.URL‑safe replacement + padding via
Pad()is consistent with client helpers.If duplication with
TcpSender/LineTcpSender.FromBase64Stringis intentional for test isolation, keep; otherwise consider centralizing to avoid drift.
52-57: Constructor docs/readability.Docs are clear and helpful; no behavior change. LGTM.
365-376: Auth config helper LGTM.Simple setter with clear XML docs; aligns with
RunServerAuth.src/net-questdb-client/Buffers/BufferV2.cs (1)
93-104: Binary write tweaks look good.LE write using MemoryMarshal and the early-return path in PutBinaryManyLE are correct and safer around boundaries.
Also applies to: 116-146
src/net-questdb-client/Buffers/BufferV3.cs (1)
81-83: The review comment is based on incorrect API version information and does not require changes.The Span overload for Decimal.GetBits was introduced in .NET 5, not .NET 7. Since the project targets net6.0, net7.0, net8.0, and net9.0—all of which exceed .NET 5—the code at BufferV3.cs:81-83 will compile successfully on all targeted frameworks without requiring conditional compilation. The current implementation is correct.
Likely an incorrect or invalid review comment.
src/net-questdb-client/Senders/ISender.cs (1)
271-281: Null-safe string overload LGTM.Default interface method avoids NRE and keeps fluent chaining.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/net-questdb-client/Buffers/BufferV3.cs (2)
30-31: Consider adding a class-level docstring.The class docstring is currently empty. Consider documenting that this class extends BufferV2 to add Protocol V3 decimal encoding support.
Apply this diff:
-/// <summary /> +/// <summary> +/// BufferV3 extends BufferV2 to support QuestDB Protocol V3, adding binary encoding for decimal columns. +/// </summary> public class BufferV3 : BufferV2
81-131: LGTM! The decimal encoding logic is correct.The implementation correctly:
- Extracts decimal components using
decimal.GetBits- Converts negative mantissas to two's complement (lines 95-103)
- Writes the value in big-endian format (lines 105-111)
- Compresses redundant sign-extension bytes while preserving sign semantics (lines 113-119)
- Handles the null case and ensures zero is always positive (line 93)
The two's complement conversion with carry propagation across the 96-bit mantissa is sound.
Optional: Consider adding inline comments to clarify the two's complement logic.
The two's complement conversion (lines 95-103) is correct but could be clearer for maintainers unfamiliar with multi-word arithmetic. Consider adding brief comments:
if (negative) { - // QuestDB expects negative mantissas in two's complement. + // QuestDB expects negative mantissas in two's complement. + // Negate the 96-bit value by inverting all bits and adding 1, + // propagating carry from low → mid → high. low = ~low + 1; var c = low == 0 ? 1 : 0; mid = ~mid + c; c = mid == 0 && c == 1 ? 1 : 0; high = ~high + c; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/net-questdb-client/Buffers/BufferV3.cs(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/net-questdb-client/Buffers/BufferV3.cs (4)
src/net-questdb-client/Buffers/BufferV2.cs (2)
BufferV2(33-286)BufferV2(36-38)src/net-questdb-client/Buffers/IBuffer.cs (18)
IBuffer(62-62)IBuffer(70-70)IBuffer(80-80)IBuffer(93-93)IBuffer(101-101)IBuffer(109-109)IBuffer(117-117)IBuffer(125-125)IBuffer(133-133)IBuffer(141-141)IBuffer(149-149)IBuffer(217-217)IBuffer(220-220)IBuffer(226-226)IBuffer(229-229)IBuffer(237-237)ReadOnlySpan(198-198)Put(223-223)src/net-questdb-client-tests/DecimalTestHelpers.cs (1)
ReadOnlySpan(85-91)src/net-questdb-client/Constants.cs (1)
Constants(29-32)
🔇 Additional comments (1)
src/net-questdb-client/Buffers/BufferV3.cs (1)
43-54: LGTM!The constants correctly match .NET's internal decimal representation for extracting sign and scale from the flags field returned by
decimal.GetBits.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (7)
src/net-questdb-client/Buffers/BufferStreamContent.cs (1)
66-71: Bug: returned MemoryStream is positioned at end. Rewind before returning.CreateContentReadStreamAsync fills the MemoryStream then returns it without resetting Position, yielding 0 bytes on read.
Apply:
- var stream = new MemoryStream(); - await SerializeToStreamAsync(stream, null, default); - return stream; + var stream = new MemoryStream(); + await SerializeToStreamAsync(stream, null, default); + stream.Position = 0; + return stream;src/net-questdb-client/Senders/AbstractSender.cs (1)
84-88: Remove BufferV1 cast; breaks V2/V3 and decimal path.Casting to BufferV1 will throw when buffer is V2/V3 (protocol v2/v3, Auto). Delegate to IBuffer instead.
- ((BufferV1)Buffer).Column(name, value); + Buffer.Column(name, value);src/net-questdb-client/Senders/TcpSender.cs (1)
76-82: Wrong protocol-version selection: Auto forced to V1.This disables V3 features (including decimal) when Options.protocol_version is Auto, contradicting Buffer.Create (which already maps Auto appropriately). Let Buffer.Create handle Auto.
- Buffer = Buffers.Buffer.Create( + Buffer = Buffers.Buffer.Create( Options.init_buf_size, Options.max_name_len, Options.max_buf_size, - Options.protocol_version == ProtocolVersion.Auto ? ProtocolVersion.V1 : Options.protocol_version + Options.protocol_version );src/net-questdb-client/Senders/HttpSender.cs (1)
130-135: Root CA certificates incorrectly added to ClientCertificates collection.Lines 130-135 add
tls_rootsto theClientCertificatescollection, buttls_rootsshould contain root CA certificates for trust validation, not client authentication certificates. Root CAs are already correctly installed intoCustomTrustStoreon lines 121-123. Adding them toClientCertificatesis semantically incorrect and may cause the client to present root CA certificates during the TLS handshake, which is inappropriate.Remove lines 130-135:
- if (!string.IsNullOrEmpty(Options.tls_roots)) - { - _handler.SslOptions.ClientCertificates ??= new X509Certificate2Collection(); - _handler.SslOptions.ClientCertificates.Add( - X509Certificate2.CreateFromPemFile(Options.tls_roots!, Options.tls_roots_password)); - } -src/net-questdb-client-tests/DummyIlpServer.cs (3)
92-99: Dispose SslStream and use a cert that includes a private key.
- sslStream isn’t disposed; wrap it (or dataStream) in an async using.
- CreateFromCertFile("certificate.pfx") may not load a private key; prefer X509Certificate2. Also consider a test-only self‑signed cert and configurable path.
Apply:
- await using var connection = new NetworkStream(socket, true); - Stream dataStream = connection; - if (_tls) - { - var sslStream = new SslStream(connection); - dataStream = sslStream; - await sslStream.AuthenticateAsServerAsync(GetCertificate()); - } + await using var connection = new NetworkStream(socket, true); + await using Stream dataStream = _tls + ? new SslStream(connection, leaveInnerStreamOpen: false) + : connection; + if (_tls) + { + await ((SslStream)dataStream).AuthenticateAsServerAsync(GetCertificate()); + }And:
- private X509Certificate GetCertificate() + private X509Certificate2 GetCertificate() { - return X509Certificate.CreateFromCertFile("certificate.pfx"); + return new X509Certificate2("certificate.pfx"); // ensure file contains private key }Also applies to: 105-105, 117-120
145-149: Be liberal in base64 decode; avoid URL-safe mismatch.Use the existing normalizer to accept '-'/'_' and missing padding for signatures.
- var signature = Convert.FromBase64String(Pad(signatureRaw)); + var signature = FromBase64String(signatureRaw);
229-233: Challenge RNG is broken (writes zero bytes) and non-crypto.
- buffer.AsSpan(length) with length==buffer.Length produces an empty span; challenge stays all zeros.
- Random(DateTime.Now.Millisecond) is predictable. Use RandomNumberGenerator.Fill.
- private void GenerateRandomBytes(byte[] buffer, int length) + private void GenerateRandomBytes(byte[] buffer, int length) { - var rnd = new Random(DateTime.Now.Millisecond); - rnd.NextBytes(buffer.AsSpan(length)); + RandomNumberGenerator.Fill(buffer.AsSpan(0, length)); }Add import:
+using System.Security.Cryptography;
♻️ Duplicate comments (4)
src/net-questdb-client/Buffers/BufferV1.cs (1)
182-188: CancelRow corrupts state when row spans multiple buffers. Restore Chunk and recompute Length.
This reverts indices/position only; active Chunk and total Length remain wrong across chunks. Same issue was flagged earlier.public void CancelRow() { - _currentBufferIndex = _lineStartBufferIndex; - Length -= Position - _lineStartBufferPosition; - Position = _lineStartBufferPosition; - _hasTable = false; + _currentBufferIndex = _lineStartBufferIndex; + Chunk = _buffers[_currentBufferIndex].Buffer; + Position = _lineStartBufferPosition; + // Recompute total length across all completed buffers + current position + var restoredLength = 0; + for (var i = 0; i < _currentBufferIndex; i++) + { + restoredLength += _buffers[i].Length; + } + restoredLength += Position; + Length = restoredLength; + _hasTable = false; }src/net-questdb-client-tests/DummyIlpServer.cs (2)
203-226: Guard against infinite loop and fix off-by-one in TotalReceived.
- If no EOL and buffer fills, ReadAsync into an empty slice can return 0 and spin forever.
- If stream closes (n==0), loop spins.
- _totalReceived adds one too many when appending trailing bytes after EOL.
private async Task<int> ReceiveUntilEol(Stream connection) { var len = 0; while (true) { - var n = await connection.ReadAsync(_buffer.AsMemory(len)); + if (len >= _buffer.Length) + throw new InvalidOperationException("Line too long while waiting for EOL."); + var n = await connection.ReadAsync(_buffer.AsMemory(len)); + if (n == 0) + throw new IOException("Connection closed before newline was received."); var inBuffer = len + n; for (var i = len; i < inBuffer; i++) { if (_buffer[i] == '\n') { if (i + 1 < inBuffer) { _received.Write(_buffer, i + 1, inBuffer - i - 1); - // ReSharper disable once NonAtomicCompoundOperator - _totalReceived += inBuffer - i; + // ReSharper disable once NonAtomicCompoundOperator + _totalReceived += inBuffer - i - 1; // exclude '\n' } return i; } } len += n; } }
291-353: PrintBuffer robustness fixes look good.
- Proper i>0 guard for "==".
- InvariantCulture for doubles.
- Clearer NotImplementedException.
src/dummy-http-server/DummyHttpServer.cs (1)
93-97: Dynamic port is ignored; fire-and-forget start hides failures.Kestrel is hard-bound to 29473/29474, so passing a URL to RunAsync has no effect. StartAsync launches unawaited, so startup failures can be missed; Healthcheck may point to a closed port. Fix binding and make startup deterministic.
Option A (minimal, deterministic startup; keep fixed ports): await StartAsync and drop the URL parameter.
- o.ListenLocalhost(29474, - options => { options.UseHttps(); }); - o.ListenLocalhost(29473); + // Endpoints are configured via _app.Urls in StartAsync for dynamic binding.- versions ??= new[] { 1, 2, 3, }; + versions ??= new[] { 1, 2, 3 }; SettingsEndpoint.Versions = versions; _port = port; - _ = _app.RunAsync($"http://localhost:{port}"); + // Bind dynamically, then start deterministically. + _app.Urls.Clear(); + _app.Urls.Add($"http://localhost:{port}"); + _app.Urls.Add($"https://localhost:{port + 1}"); + await _app.StartAsync();- public async Task RunAsync() - { - await _app.RunAsync($"http://localhost:{_port}"); - } + public async Task RunAsync() => await StartAsync(_port);Note: Removing explicit ListenLocalhost lets _app.Urls take effect; HTTPS defaults (incl. client cert mode) still apply via ConfigureHttpsDefaults. If you must retain static endpoints for some tests, remove the port parameter and always await
_app.StartAsync()instead.Also applies to: 148-152, 154-160
🧹 Nitpick comments (12)
src/net-questdb-client/Buffers/BufferStreamContent.cs (1)
39-42: Optional: guard against null buffer.
Add an argument null check to fail fast.- public BufferStreamContent(IBuffer buffer) + public BufferStreamContent(IBuffer buffer) { - Buffer = buffer; + if (buffer is null) throw new ArgumentNullException(nameof(buffer)); + Buffer = buffer; }src/net-questdb-client/Senders/TcpSender.cs (3)
88-91: Avoid blocking .Wait() on async connect; honor timeouts/cancellation.
Synchronous wait risks hangs and ignores request_timeout/auth_timeout. Prefer async flow or synchronous Connect with explicit timeout handling.Example direction:
- Make Build async and
await socket.ConnectAsync(Options.Host, Options.Port, ct).- Or use
socket.Connectwith a configurable timeout (connect via Task.WhenAny pattern).
121-125: Dispose CTS and simplify auth wait.
Prevent timer leak and avoid blocking waits.- var authTimeout = new CancellationTokenSource(); - authTimeout.CancelAfter(Options.auth_timeout); + using var authTimeout = new CancellationTokenSource(Options.auth_timeout); _signatureGenerator = Secp256r1SignatureGenerator.Instance.Value; - AuthenticateAsync(authTimeout.Token).AsTask().Wait(authTimeout.Token); + AuthenticateAsync(authTimeout.Token).AsTask().GetAwaiter().GetResult();
223-246: Duplicate Clear/LastFlush in Send; keep one in finally.
No-op but redundant; simplify.try { if (Buffer.Length != 0) { Buffer.WriteToStream(_dataStream, ct); - LastFlush = DateTime.UtcNow; - Buffer.Clear(); } } catch (Exception ex) { ... } finally { LastFlush = DateTime.UtcNow; Buffer.Clear(); }src/net-questdb-client/Senders/HttpSender.cs (2)
144-144: Consider using a dedicated connect_timeout option.
auth_timeoutis being used forConnectTimeout, but semanticallyauth_timeoutsuggests authentication time rather than connection establishment time. While functionally this may work, it could be clearer to use a separateconnect_timeoutsetting or renameauth_timeouttoconnection_timeout.
606-608: Consider aligning LastFlush handling with Send().In
SendAsync(),LastFlushis set in the finally block (line 632) for both success and failure. In contrast,Send()setsLastFlushbefore returning on success (line 362) and only in the finally block on failure. While both approaches work correctly, the inconsistency could be confusing during maintenance.Consider standardizing on one approach—preferably setting
LastFlushbefore early return for consistency withSend().Also applies to: 632-632
src/net-questdb-client-tests/DummyIlpServer.cs (3)
146-146: Drop noisy Console.WriteLine in tests (or guard with DEBUG).It pollutes test logs. Prefer Debug.WriteLine.
- Console.WriteLine(signatureRaw); + Debug.WriteLine(signatureRaw);
174-183: Simplify padding logic for readability.Current formula works but is non-obvious. A simpler equivalent improves clarity.
- private static string Pad(string text) - { - var padding = 3 - (text.Length + 3) % 4; - if (padding == 0) - { - return text; - } - return text + new string('=', padding); - } + private static string Pad(string text) + { + var pad = (4 - (text.Length % 4)) & 3; + return pad == 0 ? text : text.PadRight(text.Length + pad, '='); + }
369-374: Auth config: minor nit.Parameter name in FromBase64String refers to “encodedPrivateKey” but used for public coords too; consider renaming for clarity (optional).
src/dummy-http-server/DummyHttpServer.cs (3)
120-133: Reset endpoint flags in Clear() to avoid test cross-talk.Static flags persist between tests; reset them here.
public void Clear() { IlpEndpoint.ReceiveBuffer.Clear(); IlpEndpoint.ReceiveBytes.Clear(); IlpEndpoint.LastError = null; IlpEndpoint.Counter = 0; + IlpEndpoint.WithTokenAuth = false; + IlpEndpoint.WithBasicAuth = false; + IlpEndpoint.WithRetriableError = false; + IlpEndpoint.WithErrorMessage = false; }
190-194: Don’t create a new HttpClient per call; handle connection failures.Use a static HttpClient and return false on exceptions to match the boolean contract.
Add once near the top of the class:
+ private static readonly HttpClient Http = new();Then update Healthcheck:
- public async Task<bool> Healthcheck() - { - var response = await new HttpClient().GetAsync($"http://localhost:{_port}/ping"); - return response.IsSuccessStatusCode; - } + public async Task<bool> Healthcheck() + { + try + { + using var response = await Http.GetAsync($"http://localhost:{_port}/ping"); + return response.IsSuccessStatusCode; + } + catch + { + return false; + } + }
114-118: Dispose: avoid .Wait() on async; use GetAwaiter().GetResult() and swallow cancellation.Prevents AggregateException and sync-context deadlocks in some runners.
public void Dispose() { Clear(); - _app.StopAsync().Wait(); + try { _app.StopAsync().GetAwaiter().GetResult(); } + catch (OperationCanceledException) { /* ignore */ } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
src/dummy-http-server/DummyHttpServer.cs(6 hunks)src/net-questdb-client-tests/DummyIlpServer.cs(8 hunks)src/net-questdb-client/Buffers/BufferStreamContent.cs(1 hunks)src/net-questdb-client/Buffers/BufferV1.cs(19 hunks)src/net-questdb-client/Buffers/BufferV2.cs(12 hunks)src/net-questdb-client/Buffers/IBuffer.cs(3 hunks)src/net-questdb-client/Senders/AbstractSender.cs(4 hunks)src/net-questdb-client/Senders/HttpSender.cs(24 hunks)src/net-questdb-client/Senders/TcpSender.cs(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/net-questdb-client/Buffers/BufferV2.cs
🧰 Additional context used
🧬 Code graph analysis (7)
src/net-questdb-client/Senders/HttpSender.cs (4)
src/net-questdb-client/Utils/SenderOptions.cs (4)
SenderOptions(90-92)SenderOptions(98-123)SenderOptions(668-674)ToString(594-639)src/net-questdb-client/Senders/TcpSender.cs (2)
Build(74-134)Task(249-272)src/net-questdb-client/Buffers/BufferStreamContent.cs (5)
BufferStreamContent(33-79)BufferStreamContent(39-42)Task(47-50)Task(53-56)Task(66-71)src/net-questdb-client/Buffers/Buffer.cs (1)
Buffer(32-54)
src/net-questdb-client/Senders/AbstractSender.cs (1)
src/net-questdb-client/Senders/ISender.cs (16)
ISender(67-67)ISender(108-108)ISender(116-116)ISender(129-129)ISender(137-137)ISender(140-140)ISender(148-148)ISender(156-156)ISender(164-164)ISender(172-172)ISender(180-180)ISender(253-253)ISender(261-261)ISender(269-269)ISender(277-281)ISender(290-299)
src/net-questdb-client/Buffers/IBuffer.cs (2)
src/net-questdb-client/Buffers/BufferV1.cs (17)
IBuffer(79-97)IBuffer(252-272)IBuffer(275-295)IBuffer(298-308)IBuffer(311-317)IBuffer(320-329)IBuffer(332-341)IBuffer(344-354)IBuffer(357-366)IBuffer(369-378)IBuffer(381-396)IBuffer(418-449)IBuffer(452-455)IBuffer(458-461)IBuffer(464-468)IBuffer(545-562)ReadOnlySpan(191-194)src/net-questdb-client-tests/DecimalTestHelpers.cs (1)
ReadOnlySpan(85-91)
src/net-questdb-client-tests/DummyIlpServer.cs (4)
src/net-questdb-client-tests/JsonSpecTestRunner.cs (2)
DummyIlpServer(272-275)ToString(299-302)src/net-questdb-client-tests/LineTcpSenderTests.cs (2)
DummyIlpServer(754-757)FromBase64String(147-150)src/net-questdb-client-tests/TcpTests.cs (2)
DummyIlpServer(1040-1043)FromBase64String(1062-1065)src/dummy-http-server/DummyHttpServer.cs (2)
PrintBuffer(226-305)StringBuilder(171-174)
src/net-questdb-client/Buffers/BufferV1.cs (3)
src/net-questdb-client/Utils/IngressError.cs (3)
IngressError(35-66)IngressError(42-46)IngressError(54-58)src/net-questdb-client/Buffers/IBuffer.cs (20)
Clear(188-188)CancelRow(199-199)IBuffer(66-66)IBuffer(74-74)IBuffer(84-84)IBuffer(97-97)IBuffer(105-105)IBuffer(113-113)IBuffer(121-121)IBuffer(129-129)IBuffer(137-137)IBuffer(145-145)IBuffer(153-153)IBuffer(228-228)IBuffer(235-235)IBuffer(248-248)IBuffer(260-260)IBuffer(271-271)ReadOnlySpan(205-205)Put(241-241)src/net-questdb-client/Buffers/Buffer.cs (1)
Buffer(32-54)
src/net-questdb-client/Senders/TcpSender.cs (2)
src/net-questdb-client/Utils/SenderOptions.cs (3)
SenderOptions(90-92)SenderOptions(98-123)SenderOptions(668-674)src/net-questdb-client/LineTcpSender.cs (5)
ValueTask(113-133)ValueTask(147-191)ValueTask(215-241)ValueTask(490-513)FromBase64String(478-488)
src/dummy-http-server/DummyHttpServer.cs (3)
src/dummy-http-server/IlpEndpoint.cs (2)
IlpEndpoint(72-151)ToString(50-54)src/net-questdb-client-tests/DummyIlpServer.cs (5)
Task(84-115)Task(127-172)Task(203-227)Task(235-251)PrintBuffer(282-361)src/dummy-http-server/SettingsEndpoint.cs (1)
SettingsEndpoint(6-27)
🔇 Additional comments (7)
src/net-questdb-client/Buffers/BufferStreamContent.cs (1)
35-38: Docstring addition looks good.
Constructor XML docs are accurate and clear.src/net-questdb-client/Senders/AbstractSender.cs (2)
97-107: New int overload: LGTM.
Forwarding to Buffer is correct; docs are clear.
343-353: Nullable decimal overload: LGTM.
Matches IBuffer surface and enables V3 decimal writes.src/net-questdb-client/Buffers/IBuffer.cs (1)
285-294: DECIMAL API docs/readability: LGTM.
Summary/remarks are correct and clearly state V3 requirement.src/net-questdb-client/Senders/HttpSender.cs (1)
97-97: Verify MaxConnectionsPerServer = 1 aligns with concurrency requirements.Setting
MaxConnectionsPerServerto 1 limits the connection pool to a single connection per server. While this may align with the current single-threaded usage pattern, it could become a bottleneck if multiple senders target the same server or if concurrent operations are introduced in the future.Confirm this limitation is intentional and document the reasoning, or consider removing the setting to use the default value.
src/dummy-http-server/DummyHttpServer.cs (2)
45-56: Docs polish: LGTM.Clear, concise XML docs added across ctor, lifecycle, buffers, JWT, and PrintBuffer. Good improvements.
Also applies to: 120-126, 135-141, 154-156, 167-171, 176-181, 197-201, 221-226
59-64: Setup/auth wiring: LGTM.Reasonable logging filters, auth toggles plumbed into FastEndpoints, and AddAuthorization chained correctly when JWT is enabled.
Also applies to: 66-70, 72-76
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/net-questdb-client/Senders/ISender.cs (1)
139-141: Inheritdoc references semantically different overload.The
inheritdocreferences the string-column overload for an integer column. Consider referencing thelongoverload instead or providing explicit documentation, as integer and string columns have different semantics.Apply this diff:
- /// <inheritdoc cref="Column(ReadOnlySpan{char},ReadOnlySpan{char})" /> + /// <inheritdoc cref="Column(ReadOnlySpan{char},long)" /> public ISender Column(ReadOnlySpan<char> name, int value);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/net-questdb-client/Senders/ISender.cs(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/net-questdb-client/Senders/ISender.cs (4)
src/net-questdb-client/Senders/AbstractSender.cs (16)
ISender(46-49)ISender(70-74)ISender(77-81)ISender(84-88)ISender(91-95)ISender(103-107)ISender(110-114)ISender(117-121)ISender(124-128)ISender(131-135)ISender(138-142)ISender(144-148)ISender(150-154)ISender(156-160)ISender(269-273)Clear(255-258)src/net-questdb-client/Senders/HttpSender.cs (1)
ISender(264-280)src/net-questdb-client/Buffers/IBuffer.cs (2)
ReadOnlySpan(205-205)Clear(188-188)src/net-questdb-client/Buffers/BufferV1.cs (2)
ReadOnlySpan(191-194)Clear(147-163)
🔇 Additional comments (3)
src/net-questdb-client/Senders/ISender.cs (3)
275-279: Null-safety fix applied correctly.The null check at line 277 properly addresses the previous NRE concern and maintains fluent chaining semantics.
296-296: LGTM: Consistent fluent return pattern.The updated
return this;statements across all NullableColumn overloads provide cleaner, more idiomatic fluent chaining.Also applies to: 312-312, 328-328, 344-344, 360-360, 376-376, 392-392, 408-408
123-129: Remove the duplicate XML summary.The method already has a
<summary>block at lines 118-122. The additional summary at lines 123-129 creates malformed XML documentation.Apply this diff to remove the duplicate:
- /// <summary> - /// Adds a column (field) with the specified string value to the current row. - /// </summary> - /// <param name="name">The column name.</param> - /// <param name="value">The column value as a character span.</param> - /// <returns>The sender instance for fluent chaining.</returns> public ISender Column(ReadOnlySpan<char> name, ReadOnlySpan<char> value);Likely an incorrect or invalid review comment.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/net-questdb-client/Senders/HttpSender.cs (1)
130-135: Remove incorrect client certificate configuration.The
tls_rootsparameter is intended for configuring trusted root CAs (already correctly used in lines 119-124 for the trust store), not for client certificate authentication. Adding it as a client certificate here is incorrect.Apply this diff to remove the incorrect client certificate configuration:
- if (!string.IsNullOrEmpty(Options.tls_roots)) - { - _handler.SslOptions.ClientCertificates ??= new X509Certificate2Collection(); - _handler.SslOptions.ClientCertificates.Add( - X509Certificate2.CreateFromPemFile(Options.tls_roots!, Options.tls_roots_password)); - } - if (Options.client_cert is not null)src/net-questdb-client/Buffers/BufferV1.cs (1)
848-856: Remove duplicate XML summary tags.The method has two
<summary>tags (lines 848-850 and 852-854), which will cause XML documentation generation errors or warnings. The old summary should be removed.Apply this diff:
- /// <summary> - /// Check that the file name is not too long. - /// </summary> - /// <param name="name"></param> /// <summary> /// Validates that the UTF-8 encoded byte length of the given name is within the configured maximum. /// </summary> /// <param name="name">The name to validate (measured in UTF-8 bytes).</param> /// <exception cref="IngressError">Thrown with <see cref="ErrorCode.InvalidApiCall"/> if the name exceeds the maximum allowed byte length.</exception> private void GuardFsFileNameLimit(ReadOnlySpan<char> name)
♻️ Duplicate comments (1)
src/net-questdb-client/Buffers/BufferV1.cs (1)
184-188: CancelRow fails when row spans multiple buffers (restores index but not Chunk/Length).If a row crossed a chunk boundary, this leaves Chunk pointing at the last buffer and Length only reduced by the current buffer delta. Subsequent writes/readouts are incorrect.
Apply this diff to fix:
- _currentBufferIndex = _lineStartBufferIndex; - Length -= Position - _lineStartBufferPosition; - Position = _lineStartBufferPosition; - _hasTable = false; + // Restore buffer index and active chunk + _currentBufferIndex = _lineStartBufferIndex; + Chunk = _buffers[_currentBufferIndex].Buffer; + // Restore write position to the row start within the active chunk + Position = _lineStartBufferPosition; + // Recompute total length up to the restored position + var restoredLength = 0; + for (var i = 0; i < _currentBufferIndex; i++) + { + restoredLength += _buffers[i].Length; + } + restoredLength += Position; + Length = restoredLength; + _hasTable = false;Add a test where a large column forces NextBuffer() between Table(...) and CancelRow() to catch this.
🧹 Nitpick comments (5)
src/net-questdb-client/Buffers/BufferV1.cs (1)
606-606: Optional: Remove trailing comma in single-element initializer.The trailing comma in
stackalloc char[1] { c, }is legal but unconventional for single-element initializers.- Span<char> chars = stackalloc char[1] { c, }; + Span<char> chars = stackalloc char[1] { c };src/net-questdb-client/Buffers/BufferV3.cs (4)
70-74: Extract aPutDecimalHeaderhelper for parity with V2’s headers.Improves consistency and reduces duplication with
PutDoubleHeader/PutArrayOfDoubleHeader.- Column(name) - .PutAscii(Constants.BINARY_FORMAT_FLAG) - .Put((byte)BinaryFormatType.DECIMAL); + PutDecimalHeader(name);Add this helper in the class:
private void PutDecimalHeader(ReadOnlySpan<char> name) { Column(name) .PutAscii(Constants.BINARY_FORMAT_FLAG) .Put((byte)BinaryFormatType.DECIMAL); }
74-79: Document/confirm null encoding (scale=0, length=0).If this is part of the protocol contract, note it in XML docs and ensure tests cover it (null, non-null zero, various scales).
I can add tests for: null; 0m with scales 0/28; ±1, ±10^28−1; random values.
113-129: Optional: factor compression into a helper and assert non‑zero length.A small helper improves reuse and readability; a debug assert guards invariants for non‑null values.
private static int TrimSignExtendedPrefix(Span<byte> buf, byte sign, out int start) { start = 0; while (start < buf.Length - 1 && buf[start] == sign && ((buf[start + 1] ^ sign) & 0x80) == 0) start++; return buf.Length - start; }Usage:
- var start = 0; - for (; start < span.Length - 1 && span[start] == signByte && ((span[start + 1] ^ signByte) & 0x80) == 0; start++) ; - var size = span.Length - start; + var size = TrimSignExtendedPrefix(span, signByte, out var start); + System.Diagnostics.Debug.Assert(size > 0);
30-41: Fill the class XML summary.Replace the empty
<summary />with a brief description of V3 and decimal support for discoverability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/net-questdb-client/Buffers/BufferV1.cs(19 hunks)src/net-questdb-client/Buffers/BufferV3.cs(1 hunks)src/net-questdb-client/Senders/HttpSender.cs(24 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
src/net-questdb-client/Buffers/BufferV1.cs (3)
src/net-questdb-client/Utils/IngressError.cs (3)
IngressError(35-66)IngressError(42-46)IngressError(54-58)src/net-questdb-client/Buffers/IBuffer.cs (20)
Clear(188-188)CancelRow(199-199)IBuffer(66-66)IBuffer(74-74)IBuffer(84-84)IBuffer(97-97)IBuffer(105-105)IBuffer(113-113)IBuffer(121-121)IBuffer(129-129)IBuffer(137-137)IBuffer(145-145)IBuffer(153-153)IBuffer(228-228)IBuffer(235-235)IBuffer(248-248)IBuffer(260-260)IBuffer(271-271)ReadOnlySpan(205-205)Put(241-241)src/net-questdb-client/Buffers/Buffer.cs (1)
Buffer(32-54)
src/net-questdb-client/Senders/HttpSender.cs (5)
src/net-questdb-client/Utils/SenderOptions.cs (4)
SenderOptions(90-92)SenderOptions(98-123)SenderOptions(668-674)ToString(594-639)src/net-questdb-client/Senders/TcpSender.cs (2)
Build(74-134)Task(249-272)src/net-questdb-client/Buffers/BufferStreamContent.cs (5)
BufferStreamContent(33-79)BufferStreamContent(39-42)Task(47-50)Task(53-56)Task(66-71)src/net-questdb-client/Buffers/Buffer.cs (1)
Buffer(32-54)src/net-questdb-client/Utils/IngressError.cs (3)
IngressError(35-66)IngressError(42-46)IngressError(54-58)
src/net-questdb-client/Buffers/BufferV3.cs (3)
src/net-questdb-client/Buffers/BufferV2.cs (2)
BufferV2(33-343)BufferV2(41-43)src/net-questdb-client/Buffers/IBuffer.cs (18)
IBuffer(66-66)IBuffer(74-74)IBuffer(84-84)IBuffer(97-97)IBuffer(105-105)IBuffer(113-113)IBuffer(121-121)IBuffer(129-129)IBuffer(137-137)IBuffer(145-145)IBuffer(153-153)IBuffer(228-228)IBuffer(235-235)IBuffer(248-248)IBuffer(260-260)IBuffer(271-271)ReadOnlySpan(205-205)Put(241-241)src/net-questdb-client/Constants.cs (1)
Constants(29-32)
🔇 Additional comments (10)
src/net-questdb-client/Senders/HttpSender.cs (6)
60-79: LGTM!The constructor implementations are clean, with clear documentation and appropriate initialization order.
214-246: LGTM!The helper methods are well-implemented. The
GenerateRequestCtscorrectly creates a linked cancellation token source, addressing the concern from the previous review.
333-393: LGTM!The
Sendmethod is well-structured with proper validation, error handling, and cleanup. The success flag correctly controls theLastFlushupdate behavior.
395-479: LGTM!The retry logic correctly creates linked cancellation token sources at both line 408 and line 448, ensuring that retries will respect the caller's cancellation token. The resource disposal and backoff strategy are well-implemented.
481-520: LGTM!Both error handling methods are well-implemented with proper async/await usage, JSON deserialization, and fallback handling for non-JSON responses.
523-637: LGTM!The
SendAsyncmethod correctly mirrors the synchronous version with proper async/await patterns, linked cancellation token sources, and resource disposal.src/net-questdb-client/Buffers/BufferV1.cs (3)
140-163: LGTM!The Clear method correctly resets all buffer state including the Chunk reference. The comprehensive XML documentation clearly describes the reset behavior.
243-271: LGTM!The Table method correctly initializes row state and saves the starting buffer index and position for potential CancelRow operations. The XML documentation thoroughly explains the method's behavior and exception conditions.
866-876: LGTM! BufferV3 properly overrides decimal Column support.Verification confirms BufferV3 (line 62, src/net-questdb-client/Buffers/BufferV3.cs) correctly overrides this method with a full decimal implementation using binary format encoding. The BufferV1 method is appropriately virtual and throws the correct error for an unsupported protocol version.
src/net-questdb-client/Buffers/BufferV3.cs (1)
56-63: Overall encoding looks correct.Header, scale extraction, two’s‑complement, big‑endian layout, and minimal sign‑preserving compression are all sound. Nice work.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
src/net-questdb-client-tests/TcpTests.cs (2)
65-107: Consider additional test cases for decimal encoding.The test covers positive, negative, null, max, and min values, which is good. However, consider adding test cases for:
- Zero value (0.0m)
- Very small decimals near zero (e.g., 0.0001m)
- Precision edge cases (e.g., decimals with maximum precision)
These additional cases would help ensure the decimal encoding handles all boundary conditions correctly.
1045-1060: Remove unreachable code after Assert.Fail.Line 1059 returns
Array.Empty<byte>()afterAssert.Fail()on line 1058, butAssert.Fail()throws an exception, making the return statement unreachable.Apply this diff:
- Assert.Fail("Timed out waiting for decimal ILP payload."); - return Array.Empty<byte>(); + throw new AssertionException("Timed out waiting for decimal ILP payload.");Alternatively, if you prefer to keep
Assert.Fail, remove the unreachable return:Assert.Fail("Timed out waiting for decimal ILP payload."); - return Array.Empty<byte>();src/net-questdb-client/Buffers/BufferV3.cs (2)
30-31: Add class-level documentation.The class summary is empty. Please document that BufferV3 extends BufferV2 to add binary decimal column support for QuestDB Protocol V3.
-/// <summary /> +/// <summary> +/// Extends BufferV2 to support writing decimal columns in QuestDB's binary format (Protocol V3). +/// </summary> public class BufferV3 : BufferV2
118-122: Consider clarifying the empty-body loop.The for loop with an empty body (just a semicolon) is correct but may be unclear to future readers. Consider refactoring to a while loop or adding a comment to explain the intentional empty body.
Option 1: Refactor to while loop.
- var start = 0; - for (; - // We can strip prefix bits that are 0 (if positive) or 1 (if negative) as long as we keep at least - // one of it in front to convey the sign. - start < span.Length - 1 && span[start] == signByte && ((span[start + 1] ^ signByte) & 0x80) == 0; - start++) ; + var start = 0; + // We can strip prefix bits that are 0 (if positive) or 1 (if negative) as long as we keep at least + // one of them in front to convey the sign. + while (start < span.Length - 1 && span[start] == signByte && ((span[start + 1] ^ signByte) & 0x80) == 0) + { + start++; + }Option 2: Add explicit comment about empty body.
var start = 0; for (; // We can strip prefix bits that are 0 (if positive) or 1 (if negative) as long as we keep at least // one of it in front to convey the sign. start < span.Length - 1 && span[start] == signByte && ((span[start + 1] ^ signByte) & 0x80) == 0; - start++) ; + start++) + { + // Intentionally empty: increment handled in for statement + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/net-questdb-client-tests/TcpTests.cs(32 hunks)src/net-questdb-client/Buffers/BufferV1.cs(19 hunks)src/net-questdb-client/Buffers/BufferV3.cs(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/net-questdb-client/Buffers/BufferV1.cs (2)
src/net-questdb-client/Utils/IngressError.cs (3)
IngressError(35-66)IngressError(42-46)IngressError(54-58)src/net-questdb-client/Buffers/IBuffer.cs (20)
Clear(188-188)CancelRow(199-199)IBuffer(66-66)IBuffer(74-74)IBuffer(84-84)IBuffer(97-97)IBuffer(105-105)IBuffer(113-113)IBuffer(121-121)IBuffer(129-129)IBuffer(137-137)IBuffer(145-145)IBuffer(153-153)IBuffer(228-228)IBuffer(235-235)IBuffer(248-248)IBuffer(260-260)IBuffer(271-271)ReadOnlySpan(205-205)Put(241-241)
src/net-questdb-client/Buffers/BufferV3.cs (3)
src/net-questdb-client/Buffers/BufferV2.cs (2)
BufferV2(33-343)BufferV2(41-43)src/net-questdb-client/Buffers/IBuffer.cs (18)
IBuffer(66-66)IBuffer(74-74)IBuffer(84-84)IBuffer(97-97)IBuffer(105-105)IBuffer(113-113)IBuffer(121-121)IBuffer(129-129)IBuffer(137-137)IBuffer(145-145)IBuffer(153-153)IBuffer(228-228)IBuffer(235-235)IBuffer(248-248)IBuffer(260-260)IBuffer(271-271)ReadOnlySpan(205-205)Put(241-241)src/net-questdb-client/Constants.cs (1)
Constants(29-32)
🪛 Gitleaks (8.28.0)
src/net-questdb-client-tests/TcpTests.cs
[high] 937-937: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🔇 Additional comments (10)
src/net-questdb-client/Buffers/BufferV3.cs (3)
56-79: LGTM! Clear format documentation and correct null handling.The binary format comment clearly documents the encoding structure, and the null-value path correctly writes zero scale and length.
81-106: LGTM! Correct two's complement conversion with negative zero handling.The logic correctly converts .NET's sign-magnitude decimal representation to two's complement big-endian format. The check for
value.Value != 0mon line 93 properly handles negative zero. Theuncheckedblock prevents overflow exceptions in checked builds.
108-134: LGTM! Efficient compression preserves sign semantics.The implementation correctly builds a big-endian two's complement representation and compresses redundant leading bytes while preserving sign information. The condition on line 121 ensures that leading sign-extension bytes are stripped only when safe.
src/net-questdb-client/Buffers/BufferV1.cs (7)
141-165: LGTM: Clear properly resets all state.The enhanced documentation is helpful, and the addition of
_lineStartLength = 0correctly initializes the new field introduced to fix the CancelRow issue.
247-277: LGTM: Table method correctly captures line-start state.Line 271 (
_lineStartLength = Length;) is essential for the CancelRow fix, storing the buffer length before any row data is written. The enhanced documentation clearly explains the method's behavior and error conditions.
418-454: LGTM: Enhanced documentation for Put(long).The expanded XML documentation clearly describes the method's behavior and the special case handling for
long.MinValue.
490-655: LGTM: Documentation enhancements across internal methods.The expanded XML doc comments for
Advance,SetTableIfAppropriate,FinishLine,Column,GuardAgainstOversizedChunk,EnsureCapacity,PutUtf8, andPutSpecialmethods improve code clarity without altering behavior.
871-881: LGTM: Decimal Column method correctly rejects DECIMAL for Protocol V1.The implementation properly throws
ProtocolVersionErrorfor DECIMAL types, which are not supported by Protocol Version V1. The method is markedvirtualto allow higher protocol versions (V3) to override with actual decimal support, maintaining consistency with the array-type Column methods above.
857-862: Fix duplicate XML summary tags.Lines 857 and 858 both contain
<summary>opening tags, which is invalid XML documentation syntax.Apply this diff to remove the duplicate tag:
- /// <summary> - /// Check that the file name is not too long. - /// </summary> /// <summary> /// Validates that the UTF-8 encoded byte length of the given name is within the configured maximum. /// </summary>Likely an incorrect or invalid review comment.
177-192: CancelRow fix correctly handles multi-buffer scenarios.The implementation properly saves and restores buffer index (
_lineStartBufferIndex), chunk pointer, length, and position when cancelling a row. The fix inTable()(line 271) stores_lineStartLengthandCancelRow()(line 188) correctly restores it, which addresses the multi-buffer edge case.However, existing tests (TcpTests.cs:575, HttpTests.cs) only verify basic single-buffer row cancellation. No test exercises the critical scenario where a row spans multiple buffers during
NextBuffer()transitions and is then cancelled.Recommendation: Add a test that:
- Writes data beyond current buffer capacity
- Triggers
NextBuffer()- Calls
CancelRow()mid-row- Verifies all buffers are correctly restored
Minor cleanup: Line 191 has trailing whitespace.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/net-questdb-client-tests/TcpTests.cs (1)
1045-1060: Consider parameterizing the timeout and generalizing the error message.The fixed 5-second timeout might be tight on slower CI environments, and the error message mentions "decimal ILP payload" even though this helper could be reused for any ILP line test.
Apply this diff to make the helper more reusable:
-private static byte[] WaitForLineBytes(DummyIlpServer server) +private static byte[] WaitForLineBytes(DummyIlpServer server, int timeoutMs = 5000) { - for (var i = 0; i < 500; i++) + var iterations = timeoutMs / 10; + for (var i = 0; i < iterations; i++) { var bytes = server.GetReceivedBytes(); if (bytes.Length > 0 && bytes[^1] == (byte)'\n') { return bytes; } Thread.Sleep(10); } - Assert.Fail("Timed out waiting for decimal ILP payload."); + Assert.Fail($"Timed out waiting for ILP line payload after {timeoutMs}ms."); return Array.Empty<byte>(); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/net-questdb-client-tests/TcpTests.cs(32 hunks)
🧰 Additional context used
🪛 Gitleaks (8.28.0)
src/net-questdb-client-tests/TcpTests.cs
[high] 937-937: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🔇 Additional comments (1)
src/net-questdb-client-tests/TcpTests.cs (1)
65-107: LGTM! Comprehensive decimal binary encoding test.This test provides excellent coverage of decimal edge cases with protocol V3: positive, negative, null, max, and min values. The binary payload validation using
DecimalTestHelpersensures the mantissa bytes and scales are correctly encoded.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/net-questdb-client-tests/TcpTests.cs (1)
1045-1060: Consider consolidating wait helpers to reduce duplication.The
WaitForLineByteshelper follows the same polling pattern asWaitAssertmethods inJsonSpecTestRunner.cs. While acceptable for test code, you could extract the common polling logic (500 iterations, 10ms sleep) into a shared helper or constants to improve maintainability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/net-questdb-client-tests/TcpTests.cs(32 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/net-questdb-client-tests/TcpTests.cs (5)
src/net-questdb-client-tests/JsonSpecTestRunner.cs (5)
WaitAssert(229-239)WaitAssert(250-259)WaitAssert(261-270)Task(53-103)DummyIlpServer(272-275)src/net-questdb-client/Senders/TcpSender.cs (1)
Task(249-272)src/net-questdb-client/Buffers/BufferV1.cs (1)
Task(201-221)src/net-questdb-client-tests/DummyIlpServer.cs (3)
DummyIlpServer(41-375)DummyIlpServer(58-63)GetReceivedBytes(266-269)src/net-questdb-client-tests/DecimalTestHelpers.cs (3)
DecimalTestHelpers(31-92)AssertDecimalField(40-57)AssertDecimalNullField(67-77)
🪛 Gitleaks (8.28.0)
src/net-questdb-client-tests/TcpTests.cs
[high] 937-937: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🔇 Additional comments (1)
src/net-questdb-client-tests/TcpTests.cs (1)
65-107: LGTM! Comprehensive decimal binary encoding test.The test correctly validates decimal field encoding in binary format with protocol V3, covering positive, negative, null, and boundary values (max/min). The use of
DecimalTestHelpersfor asserting binary payloads ensures precise validation of the wire format.
| Column(name) | ||
| .PutAscii(Constants.BINARY_FORMAT_FLAG) | ||
| .Put((byte)BinaryFormatType.DECIMAL); | ||
| if (value is null) | ||
| { | ||
| Put(0); // Scale | ||
| Put(0); // Length | ||
| return this; | ||
| } |
There was a problem hiding this comment.
We should avoid writing anything, including the column name, when the value is null
There was a problem hiding this comment.
Is this moved somewhere or totally deleted?
There was a problem hiding this comment.
It's already in the net-questdb-client-tests project which tests both http and tcp. I finished to merge them.
| var low = parts[0]; | ||
| var mid = parts[1]; | ||
| var high = parts[2]; | ||
| var negative = (flags & SignMask) != 0 && value.Value != 0m; | ||
|
|
||
| if (negative) | ||
| { | ||
| // QuestDB expects negative mantissas in two's complement. | ||
| unchecked | ||
| { | ||
| low = ~low + 1; | ||
| var c = low == 0 ? 1 : 0; | ||
| mid = ~mid + c; | ||
| c = mid == 0 && c == 1 ? 1 : 0; | ||
| high = ~high + c; | ||
| } | ||
| } | ||
|
|
||
| // We write the byte array on the stack first so that we can compress (remove unnecessary bytes) it later. | ||
| Span<byte> span = stackalloc byte[13]; | ||
| var signByte = (byte)(negative ? 255 : 0); | ||
| span[0] = signByte; | ||
| BinaryPrimitives.WriteInt32BigEndian(span.Slice(1, 4), high); | ||
| BinaryPrimitives.WriteInt32BigEndian(span.Slice(5, 4), mid); | ||
| BinaryPrimitives.WriteInt32BigEndian(span.Slice(9, 4), low); | ||
|
|
||
| // Compress | ||
| var start = 0; | ||
| for (; | ||
| // We can strip prefix bits that are 0 (if positive) or 1 (if negative) as long as we keep at least | ||
| // one of it in front to convey the sign. | ||
| start < span.Length - 1 && span[start] == signByte && ((span[start + 1] ^ signByte) & 0x80) == 0; | ||
| start++) ; | ||
|
|
||
| // 4. Length | ||
| var size = span.Length - start; | ||
| Put((byte)size); |
There was a problem hiding this comment.
A lot of bit twiddling and only 1 test. How can we be sure it's correct?
There was a problem hiding this comment.
For the compress part, there are 5 tests in HttpTests that tests all paths and a few more coming from the ilp-client-interop-test.json file.
For the negation, I've just added some more tests in 331083e
This PR adds decimal support.
This is a tandem pr for:
Usage
Summary by CodeRabbit
New Features
Documentation
Refactor
Tests
Chores