Skip to content

Conversation

@nwoolmer
Copy link
Contributor

@nwoolmer nwoolmer commented Nov 17, 2025

decimal? was added to the Column API, making it the go-to target for null type inference.

This means that if you pass a non-decimal null value (e.g. long?) to Column, it will be inferred as a decimal and break against version of QuestDB prior to 9.2.0.

The fix is to split it appropriately by swapping Column to non-null decimal, and adding a NullableColumn API.

Manual test:

await sender.Table("trades")
            .Symbol("symbol", "ETH-USD")
            .Symbol("side", "sell")
            .Column("price", (long?)1234)
            .Column("amount", 0.00044)
            .AtAsync(DateTime.UtcNow);

error:

Unhandled exception. QuestDB.Utils.IngressError: ProtocolVersionError : Protocol Version does not support DECIMAL types
   at QuestDB.Buffers.BufferV1.Column(ReadOnlySpan`1 name, Nullable`1 value) in <snip>/net-questdb-client/src/net-questdb-client/Buffers/BufferV1.cs:line 880
   at QuestDB.Senders.AbstractSender.Column(ReadOnlySpan`1 name, Nullable`1 value) in <snip>/net-questdb-client/src/net-questdb-client/Senders/AbstractSender.cs:line 351
   at Program.<Main>$(String[] args) in <snip>/net-questdb-client/src/example-basic/Program.cs:line 12
   at Program.<Main>(String[] args)

After fix, it won't compile:

image

Then if you use the new nullable API:

await sender.Table("trades")
            .Symbol("symbol", "ETH-USD")
            .Symbol("side", "sell")
            .NullableColumn("price", (long?)1234)
            .Column("amount", 0.00044)
            .AtAsync(DateTime.UtcNow);

It will work fine, since there is a better type match (long?) in this case.

Summary by CodeRabbit

  • New Features

    • Added NullableColumn to explicitly write nullable decimal values.
    • Connection strings now support protocol_version=3.
  • API Changes

    • Decimal column APIs updated: primary Column now expects non-null decimal; use NullableColumn for nulls.
    • String column behavior: null strings are ignored (no-op) rather than sent.

@coderabbitai
Copy link

coderabbitai bot commented Nov 17, 2025

Walkthrough

Replaces nullable-decimal overloads with non-nullable decimals across buffer and sender interfaces/implementations, adds ISender.NullableColumn(...) for explicit null handling, updates tests to call NullableColumn for null decimals, and updates example connection string to include protocol_version=3.

Changes

Cohort / File(s) Summary
Example / Config
src/example-basic/Program.cs
Connection string updated to include protocol_version=3.
Public API / Interfaces
src/net-questdb-client/Senders/ISender.cs, src/net-questdb-client/Buffers/IBuffer.cs
Column(..., decimal?) changed to Column(..., decimal); ISender gains NullableColumn(..., decimal?) overload(s); minor docs/usings formatting.
Sender Implementation
src/net-questdb-client/Senders/AbstractSender.cs
Removed nullable-decimal Column overload; added non-nullable Column that forwards to buffer; added forwarding logic for nullable via new NullableColumn on interface.
Buffer Implementations
src/net-questdb-client/Buffers/BufferV1.cs, src/net-questdb-client/Buffers/BufferV3.cs
Decimal Column signatures changed to non-nullable decimal; removed nullable handling and adjusted decimal encoding calls to use non-nullable input.
Tests / Runners
src/net-questdb-client-tests/HttpTests.cs, src/net-questdb-client-tests/TcpTests.cs, src/net-questdb-client-tests/JsonSpecTestRunner.cs
Test usages changed from Column("dec_null", (decimal?)null) to NullableColumn("dec_null", (decimal?)null); minor formatting edits.

Sequence Diagram(s)

sequenceDiagram
    participant Caller as Caller Code
    participant Sender as ISender
    participant Buffer as IBuffer

    rect rgb(240,240,255)
    Note over Caller,Buffer: Old flow (pre-change)
    Caller->>Sender: Column(name, decimal?)
    alt value non-null
        Sender->>Buffer: Column(name, decimal)
        Buffer->>Buffer: Encode decimal (nullable-aware)
    else value null
        Sender->>Buffer: Column(name, null)  // buffer handled null
    end
    end

    rect rgb(240,255,240)
    Note over Caller,Buffer: New flow (post-change)
    Caller->>Sender: NullableColumn(name, decimal?)
    alt value non-null
        Sender->>Sender: Column(name, (decimal)value)
        Sender->>Buffer: Column(name, decimal)
        Buffer->>Buffer: Encode decimal (non-nullable path)
    else value null
        Sender-->>Caller: return this (skip writing)
    end
    end
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

  • Areas requiring extra attention:
    • BufferV3 decimal encoding math (mantissa/sign/scale) after switching to non-nullable inputs.
    • Interface/implementation consistency for NullableColumn behavior and documentation.
    • All call sites and tests updated to use NullableColumn where null decimals are expected.

Possibly related PRs

  • feat: support decimal #52 — modifies decimal handling surfaces (IBuffer/BufferV3, ISender/AbstractSender, tests), likely overlapping the same API changes.

Poem

A rabbit taps the keys at night,
Turning nullable shadows bright,
Decimal doors now gently close,
NullableColumn guards who knows,
Protocol v3 — hop, data's right. 🐇

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately describes the main change: addressing null type inference issues from a previous decimal merge by introducing NullableColumn API.
Docstring Coverage ✅ Passed Docstring coverage is 93.44% which is sufficient. The required threshold is 80.00%.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch fix-post-decimal-null-inference

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 4

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
src/net-questdb-client/Buffers/BufferV3.cs (1)

56-132: Decimal binary encoding is correct; adjust param doc to non‑nullable

The new Column(ReadOnlySpan<char> name, decimal value) implementation correctly:

  • Extracts scale from the decimal flags.
  • Forms a 96‑bit mantissa, applies two’s‑complement for negatives, and prefixes with a sign byte.
  • Compresses redundant leading sign bytes while preserving sign information.
  • Emits the expected payloads for positive, negative, max, and min decimals, as verified by the tests in TcpTests/HttpTests.

However, the XML <param name="value"> still refers to a “nullable decimal value…when null writes zero scale and zero length,” which no longer reflects the non‑nullable signature or the new null‑handling strategy.

Suggested doc tweak:

-    /// <param name="value">Nullable decimal value to encode; when null writes zero scale and zero length.</param>
+    /// <param name="value">Decimal value to encode in binary ILP format.</param>

This keeps the documentation aligned with the actual method contract; nullable decimals are now handled at a higher layer before calling this overload.

🧹 Nitpick comments (1)
src/example-basic/Program.cs (1)

4-19: Example now requires protocol v3 explicitly

Hard‑wiring protocol_version=3 in the basic example is fine for users on recent QuestDB versions, but it will fail against older servers that only negotiate v1/v2. If you still want this sample to be “drop‑in” for mixed environments, consider either leaving the protocol version unspecified (letting negotiation pick the best available) or documenting that this example assumes a QuestDB release with ILP v3 support.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 939350d and 977ffc1.

📒 Files selected for processing (9)
  • src/example-basic/Program.cs (1 hunks)
  • src/net-questdb-client-tests/HttpTests.cs (1 hunks)
  • src/net-questdb-client-tests/JsonSpecTestRunner.cs (5 hunks)
  • 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 (4 hunks)
  • src/net-questdb-client/Buffers/IBuffer.cs (1 hunks)
  • src/net-questdb-client/Senders/AbstractSender.cs (4 hunks)
  • src/net-questdb-client/Senders/ISender.cs (12 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: RaphDal
Repo: questdb/net-questdb-client PR: 52
File: src/net-questdb-client/Senders/ISender.cs:410-418
Timestamp: 2025-10-17T16:38:52.474Z
Learning: In src/net-questdb-client/Senders/ISender.cs, the decimal type intentionally does not have a NullableColumn overload. This is a deliberate design decision because NullableColumn is deprecated in favor of using the Column method directly with nullable types.
Learnt from: RaphDal
Repo: questdb/net-questdb-client PR: 52
File: src/net-questdb-client/Senders/ISender.cs:410-418
Timestamp: 2025-10-17T16:38:52.474Z
Learning: In the net-questdb-client project (C#), the NullableColumn pattern (e.g., NullableColumn(ReadOnlySpan<char> name, long? value)) is considered a bad practice and is only kept for backward compatibility reasons. Do not suggest adding NullableColumn overloads for new types.
📚 Learning: 2025-10-17T16:38:52.474Z
Learnt from: RaphDal
Repo: questdb/net-questdb-client PR: 52
File: src/net-questdb-client/Senders/ISender.cs:410-418
Timestamp: 2025-10-17T16:38:52.474Z
Learning: In src/net-questdb-client/Senders/ISender.cs, the decimal type intentionally does not have a NullableColumn overload. This is a deliberate design decision because NullableColumn is deprecated in favor of using the Column method directly with nullable types.

Applied to files:

  • src/example-basic/Program.cs
  • src/net-questdb-client-tests/JsonSpecTestRunner.cs
  • src/net-questdb-client/Buffers/IBuffer.cs
  • src/net-questdb-client/Senders/AbstractSender.cs
  • src/net-questdb-client-tests/TcpTests.cs
  • src/net-questdb-client/Buffers/BufferV3.cs
  • src/net-questdb-client-tests/HttpTests.cs
  • src/net-questdb-client/Buffers/BufferV1.cs
  • src/net-questdb-client/Senders/ISender.cs
📚 Learning: 2025-10-17T16:38:52.474Z
Learnt from: RaphDal
Repo: questdb/net-questdb-client PR: 52
File: src/net-questdb-client/Senders/ISender.cs:410-418
Timestamp: 2025-10-17T16:38:52.474Z
Learning: In the net-questdb-client project (C#), the NullableColumn pattern (e.g., NullableColumn(ReadOnlySpan<char> name, long? value)) is considered a bad practice and is only kept for backward compatibility reasons. Do not suggest adding NullableColumn overloads for new types.

Applied to files:

  • src/net-questdb-client-tests/JsonSpecTestRunner.cs
  • src/net-questdb-client/Buffers/IBuffer.cs
  • src/net-questdb-client/Senders/AbstractSender.cs
  • src/net-questdb-client-tests/TcpTests.cs
  • src/net-questdb-client/Buffers/BufferV3.cs
  • src/net-questdb-client-tests/HttpTests.cs
  • src/net-questdb-client/Buffers/BufferV1.cs
  • src/net-questdb-client/Senders/ISender.cs
📚 Learning: 2025-10-17T16:37:03.583Z
Learnt from: RaphDal
Repo: questdb/net-questdb-client PR: 52
File: src/dummy-http-server/DummyHttpServer.cs:228-299
Timestamp: 2025-10-17T16:37:03.583Z
Learning: In src/dummy-http-server/DummyHttpServer.cs, the PrintBuffer() method is a test helper that produces human-readable output from received bytes. Decimal tests use binary representation directly via DecimalTestHelpers and do not call PrintBuffer, so DECIMAL type decoding in PrintBuffer is not required.

Applied to files:

  • src/net-questdb-client-tests/TcpTests.cs
  • src/net-questdb-client/Buffers/BufferV3.cs
  • src/net-questdb-client/Buffers/BufferV1.cs
🧬 Code graph analysis (6)
src/example-basic/Program.cs (1)
src/net-questdb-client/Sender.cs (1)
  • Sender (48-96)
src/net-questdb-client/Buffers/IBuffer.cs (2)
src/net-questdb-client/Buffers/BufferV1.cs (17)
  • IBuffer (80-98)
  • IBuffer (257-278)
  • IBuffer (281-301)
  • IBuffer (304-314)
  • IBuffer (317-323)
  • IBuffer (326-335)
  • IBuffer (338-347)
  • IBuffer (350-360)
  • IBuffer (363-372)
  • IBuffer (375-384)
  • IBuffer (387-402)
  • IBuffer (427-458)
  • IBuffer (461-464)
  • IBuffer (467-470)
  • IBuffer (473-477)
  • IBuffer (503-506)
  • ReadOnlySpan (195-198)
src/net-questdb-client-tests/DecimalTestHelpers.cs (1)
  • ReadOnlySpan (65-71)
src/net-questdb-client/Senders/AbstractSender.cs (1)
src/net-questdb-client/Senders/ISender.cs (16)
  • ISender (68-68)
  • ISender (109-109)
  • ISender (117-117)
  • ISender (130-130)
  • ISender (138-138)
  • ISender (141-141)
  • ISender (149-149)
  • ISender (157-157)
  • ISender (165-165)
  • ISender (173-173)
  • ISender (181-181)
  • ISender (255-255)
  • ISender (266-266)
  • ISender (274-274)
  • ISender (282-290)
  • ISender (300-309)
src/net-questdb-client/Buffers/BufferV3.cs (1)
src/net-questdb-client/Buffers/BufferV1.cs (17)
  • IBuffer (80-98)
  • IBuffer (257-278)
  • IBuffer (281-301)
  • IBuffer (304-314)
  • IBuffer (317-323)
  • IBuffer (326-335)
  • IBuffer (338-347)
  • IBuffer (350-360)
  • IBuffer (363-372)
  • IBuffer (375-384)
  • IBuffer (387-402)
  • IBuffer (427-458)
  • IBuffer (461-464)
  • IBuffer (467-470)
  • IBuffer (473-477)
  • IBuffer (503-506)
  • ReadOnlySpan (195-198)
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 (261-261)
  • IBuffer (273-273)
  • ReadOnlySpan (205-205)
  • Put (241-241)
src/net-questdb-client/Senders/ISender.cs (1)
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 (275-279)
  • ISender (281-285)
🔇 Additional comments (10)
src/net-questdb-client-tests/JsonSpecTestRunner.cs (2)

81-91: Correctly routes DECIMAL nulls via NullableColumn

Switching the DECIMAL null case to sender.NullableColumn(column.Name, (decimal?)null) while keeping non‑null values on Column(column.Name, d) matches the new non‑nullable decimal API and avoids the previous null‑inference issue. Behaviour for non‑null values is unchanged.


47-53: XML doc reflow improves clarity

The updated XML summaries and parameter docs here are clearer and better aligned with the actual behaviour of the test runner without changing semantics.

Also applies to: 106-113, 169-176, 305-306

src/net-questdb-client-tests/TcpTests.cs (1)

71-108: Decimal binary encoding test correctly exercises nullable path

Updating SendLineWithDecimalBinaryEncoding to call .NullableColumn("dec_null", (decimal?)null) while leaving the existing byte‑level assertions for dec_pos, dec_neg, dec_max, and dec_min intact is consistent with the new API. The additional assertion that "dec_null=" does not appear in the buffer confirms that null decimals are omitted, matching the intended wire format.

src/net-questdb-client-tests/HttpTests.cs (1)

88-138: HTTP decimal test correctly uses NullableColumn for null value

Switching dec_null to .NullableColumn("dec_null", (decimal?)null) while continuing to assert the absence of "dec_null=" and the exact binary encoding for other decimal fields accurately exercises the new nullable decimal API over HTTP and guards against regressions in the v3 binary format.

src/net-questdb-client/Buffers/BufferV3.cs (1)

46-55: Ctor doc clarifies BufferV3 configuration parameters

The new summary and parameter docs for BufferV3(int bufferSize, int maxNameLen, int maxBufSize) clearly describe buffer and name‑length limits without changing behaviour.

src/net-questdb-client/Senders/AbstractSender.cs (1)

97-107: LGTM!

The documentation formatting improvements are clear and consistent.

Also applies to: 318-331

src/net-questdb-client/Buffers/BufferV1.cs (1)

45-46: LGTM!

The formatting improvements, documentation enhancements, and code alignment changes are consistent and do not alter behavior.

Also applies to: 141-166, 178-192, 247-278, 419-458, 510-549, 565-610, 629-686, 757-823, 829-882, 888-902

src/net-questdb-client/Senders/ISender.cs (3)

25-25: LGTM!

The using QuestDB.Enums; directive is appropriately added to support error code references in documentation.


282-290: LGTM!

The explicit null check makes the string column handling consistent with the nullable pattern used elsewhere in the API.


125-173: LGTM!

The documentation formatting improvements enhance clarity and consistency across the interface without altering behavior.

Also applies to: 244-274, 293-328, 331-430

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (1)
src/net-questdb-client/Buffers/BufferV1.cs (1)

493-506: LGTM! Documentation appears corrected.

The implementation correctly rejects DECIMAL types for Protocol Version V1 by throwing a ProtocolVersionError. The documentation has been properly updated to remove references to nullable values, addressing the previous review concern.

Minor suggestion: Consider making the error message more specific, e.g., "Protocol Version V1 does not support DECIMAL types" for clarity.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 977ffc1 and 5165008.

📒 Files selected for processing (3)
  • src/net-questdb-client-tests/HttpTests.cs (1 hunks)
  • src/net-questdb-client/Buffers/BufferV1.cs (19 hunks)
  • src/net-questdb-client/Senders/ISender.cs (12 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • src/net-questdb-client-tests/HttpTests.cs
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: RaphDal
Repo: questdb/net-questdb-client PR: 52
File: src/net-questdb-client/Senders/ISender.cs:410-418
Timestamp: 2025-10-17T16:38:52.474Z
Learning: In the net-questdb-client project (C#), the NullableColumn pattern (e.g., NullableColumn(ReadOnlySpan<char> name, long? value)) is considered a bad practice and is only kept for backward compatibility reasons. Do not suggest adding NullableColumn overloads for new types.
Learnt from: RaphDal
Repo: questdb/net-questdb-client PR: 52
File: src/net-questdb-client/Senders/ISender.cs:410-418
Timestamp: 2025-10-17T16:38:52.474Z
Learning: In src/net-questdb-client/Senders/ISender.cs, the decimal type intentionally does not have a NullableColumn overload. This is a deliberate design decision because NullableColumn is deprecated in favor of using the Column method directly with nullable types.
📚 Learning: 2025-10-17T16:38:52.474Z
Learnt from: RaphDal
Repo: questdb/net-questdb-client PR: 52
File: src/net-questdb-client/Senders/ISender.cs:410-418
Timestamp: 2025-10-17T16:38:52.474Z
Learning: In src/net-questdb-client/Senders/ISender.cs, the decimal type intentionally does not have a NullableColumn overload. This is a deliberate design decision because NullableColumn is deprecated in favor of using the Column method directly with nullable types.

Applied to files:

  • src/net-questdb-client/Buffers/BufferV1.cs
  • src/net-questdb-client/Senders/ISender.cs
📚 Learning: 2025-10-17T16:38:52.474Z
Learnt from: RaphDal
Repo: questdb/net-questdb-client PR: 52
File: src/net-questdb-client/Senders/ISender.cs:410-418
Timestamp: 2025-10-17T16:38:52.474Z
Learning: In the net-questdb-client project (C#), the NullableColumn pattern (e.g., NullableColumn(ReadOnlySpan<char> name, long? value)) is considered a bad practice and is only kept for backward compatibility reasons. Do not suggest adding NullableColumn overloads for new types.

Applied to files:

  • src/net-questdb-client/Buffers/BufferV1.cs
  • src/net-questdb-client/Senders/ISender.cs
📚 Learning: 2025-10-17T16:37:03.583Z
Learnt from: RaphDal
Repo: questdb/net-questdb-client PR: 52
File: src/dummy-http-server/DummyHttpServer.cs:228-299
Timestamp: 2025-10-17T16:37:03.583Z
Learning: In src/dummy-http-server/DummyHttpServer.cs, the PrintBuffer() method is a test helper that produces human-readable output from received bytes. Decimal tests use binary representation directly via DecimalTestHelpers and do not call PrintBuffer, so DECIMAL type decoding in PrintBuffer is not required.

Applied to files:

  • src/net-questdb-client/Buffers/BufferV1.cs
🧬 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 (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 (261-261)
  • IBuffer (273-273)
  • ReadOnlySpan (205-205)
  • Put (241-241)
src/net-questdb-client/Senders/ISender.cs (2)
src/net-questdb-client/Senders/AbstractSender.cs (17)
  • 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 (275-279)
  • ISender (281-285)
  • Clear (255-258)
src/net-questdb-client/Buffers/IBuffer.cs (2)
  • ReadOnlySpan (205-205)
  • Clear (188-188)
🔇 Additional comments (5)
src/net-questdb-client/Buffers/BufferV1.cs (1)

45-902: Documentation and formatting improvements look good.

The remaining changes throughout the file consist of documentation enhancements, consistent formatting, and clearer error messages. These improve code maintainability without affecting functionality.

src/net-questdb-client/Senders/ISender.cs (4)

25-25: LGTM!

The new using directive is necessary for the ErrorCode enum referenced in the decimal method documentation (line 449).


282-290: Good defensive improvement.

Adding explicit null handling prevents a potential NullReferenceException when a null string is passed. The method now gracefully returns early, consistent with the nullable column pattern used throughout the interface.


432-459: API design aligns with PR objectives and resolves null inference issue.

The split between non-nullable Column(decimal) and NullableColumn(decimal?) correctly addresses the type inference problem described in the PR objectives. By removing decimal? from the Column overload set, non-decimal nullable values (e.g., long?) will now match their correct overloads instead of being incorrectly inferred as decimal?.

The documentation has been properly updated to clarify that Column(decimal) accepts non-nullable values and directs users to NullableColumn(decimal?) for nullable scenarios. The implementation correctly delegates to Column when the value is non-null and returns early when null.

Note: This intentionally diverges from the retrieved learning that "NullableColumn is deprecated" because the PR objectives justify this exception to prevent type inference issues with older QuestDB versions.

Based on learnings


125-430: Documentation improvements enhance clarity.

The documentation updates throughout the file improve consistency, clarify parameter behaviors, and better explain return values for fluent chaining. These changes enhance the developer experience without affecting functionality.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (1)
src/net-questdb-client/Buffers/BufferV3.cs (1)

85-87: Optional: Variable alignment improves readability.

The alignment of variable declarations and assignments is a nice stylistic touch that improves code readability.

Also applies to: 97-98, 104-105

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 5165008 and 85fe5c2.

📒 Files selected for processing (1)
  • src/net-questdb-client/Buffers/BufferV3.cs (4 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: RaphDal
Repo: questdb/net-questdb-client PR: 52
File: src/net-questdb-client/Senders/ISender.cs:410-418
Timestamp: 2025-10-17T16:38:52.474Z
Learning: In src/net-questdb-client/Senders/ISender.cs, the decimal type intentionally does not have a NullableColumn overload. This is a deliberate design decision because NullableColumn is deprecated in favor of using the Column method directly with nullable types.
Learnt from: RaphDal
Repo: questdb/net-questdb-client PR: 52
File: src/net-questdb-client/Senders/ISender.cs:410-418
Timestamp: 2025-10-17T16:38:52.474Z
Learning: In the net-questdb-client project (C#), the NullableColumn pattern (e.g., NullableColumn(ReadOnlySpan<char> name, long? value)) is considered a bad practice and is only kept for backward compatibility reasons. Do not suggest adding NullableColumn overloads for new types.
Learnt from: RaphDal
Repo: questdb/net-questdb-client PR: 52
File: src/dummy-http-server/DummyHttpServer.cs:228-299
Timestamp: 2025-10-17T16:37:03.583Z
Learning: In src/dummy-http-server/DummyHttpServer.cs, the PrintBuffer() method is a test helper that produces human-readable output from received bytes. Decimal tests use binary representation directly via DecimalTestHelpers and do not call PrintBuffer, so DECIMAL type decoding in PrintBuffer is not required.
📚 Learning: 2025-10-17T16:38:52.474Z
Learnt from: RaphDal
Repo: questdb/net-questdb-client PR: 52
File: src/net-questdb-client/Senders/ISender.cs:410-418
Timestamp: 2025-10-17T16:38:52.474Z
Learning: In the net-questdb-client project (C#), the NullableColumn pattern (e.g., NullableColumn(ReadOnlySpan<char> name, long? value)) is considered a bad practice and is only kept for backward compatibility reasons. Do not suggest adding NullableColumn overloads for new types.

Applied to files:

  • src/net-questdb-client/Buffers/BufferV3.cs
📚 Learning: 2025-10-17T16:37:03.583Z
Learnt from: RaphDal
Repo: questdb/net-questdb-client PR: 52
File: src/dummy-http-server/DummyHttpServer.cs:228-299
Timestamp: 2025-10-17T16:37:03.583Z
Learning: In src/dummy-http-server/DummyHttpServer.cs, the PrintBuffer() method is a test helper that produces human-readable output from received bytes. Decimal tests use binary representation directly via DecimalTestHelpers and do not call PrintBuffer, so DECIMAL type decoding in PrintBuffer is not required.

Applied to files:

  • src/net-questdb-client/Buffers/BufferV3.cs
📚 Learning: 2025-10-17T16:38:52.474Z
Learnt from: RaphDal
Repo: questdb/net-questdb-client PR: 52
File: src/net-questdb-client/Senders/ISender.cs:410-418
Timestamp: 2025-10-17T16:38:52.474Z
Learning: In src/net-questdb-client/Senders/ISender.cs, the decimal type intentionally does not have a NullableColumn overload. This is a deliberate design decision because NullableColumn is deprecated in favor of using the Column method directly with nullable types.

Applied to files:

  • src/net-questdb-client/Buffers/BufferV3.cs
🧬 Code graph analysis (1)
src/net-questdb-client/Buffers/BufferV3.cs (2)
src/net-questdb-client/Buffers/BufferV1.cs (17)
  • IBuffer (80-98)
  • IBuffer (257-278)
  • IBuffer (281-301)
  • IBuffer (304-314)
  • IBuffer (317-323)
  • IBuffer (326-335)
  • IBuffer (338-347)
  • IBuffer (350-360)
  • IBuffer (363-372)
  • IBuffer (375-384)
  • IBuffer (387-402)
  • IBuffer (427-458)
  • IBuffer (461-464)
  • IBuffer (467-470)
  • IBuffer (473-477)
  • IBuffer (503-506)
  • ReadOnlySpan (195-198)
src/net-questdb-client-tests/DecimalTestHelpers.cs (1)
  • ReadOnlySpan (65-71)
🔇 Additional comments (4)
src/net-questdb-client/Buffers/BufferV3.cs (4)

46-54: LGTM!

The public constructor is a clean addition that properly delegates to the base class.


76-88: LGTM!

The implementation correctly removes nullable unwrapping (.Value) since the parameter is now non-nullable. The decimal encoding logic remains sound.


113-120: Good practice: Explicit braces improve clarity.

Adding explicit braces around the empty loop body (even with just a semicolon) is a good defensive coding practice that improves readability and prevents potential future errors.


56-63: All callers have been properly updated—the API change is complete and correct.

The verification confirms:

  • NullableColumn(ReadOnlySpan<char> name, decimal? value) overload exists in ISender.cs
  • All test files (TcpTests.cs, HttpTests.cs, JsonSpecTestRunner.cs) correctly use NullableColumn for nullable decimals
  • Non-nullable Column method signature is in place
  • Documentation has been updated to guide users to NullableColumn for null values

The signature change from decimal? to decimal correctly prevents the null inference problem (where passing long? would incorrectly infer as decimal?), and the parallel NullableColumn API provides an explicit mechanism for nullable decimals.

@ideoma ideoma merged commit 3a51706 into main Nov 17, 2025
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants