-
Couldn't load subscription status.
- Fork 10
feat: support decimal #58
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
@CodeRabbit review
✅ Actions performed
Review triggered.
Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.
|
Warning Rate limit exceeded@RaphDal has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 8 minutes and 5 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (4)
WalkthroughAdds ProtocolVersion3 and comprehensive QuestDB DECIMAL support: new ScaledDecimal type and helpers, buffer and sender plumbing for textual and binary decimals, protocol negotiation updated to accept v3, many tests and README docs, plus a test submodule pointer update. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client as Caller (LineSender)
participant Sender as Sender (V3 / V2 / Base)
participant Buffer as Buffer
participant Decimal as ScaledDecimal
participant Server as QuestDB
Client->>+Sender: DecimalColumn(name, val)
alt Sender supports decimals (ProtocolVersion3)
Sender->>+Buffer: DecimalColumn(name, val)
Buffer->>Decimal: normalize & validate
alt text input
Decimal->>Buffer: validateDecimalText → write text + 'd'
else binary input
Decimal->>Buffer: write binary '=' + type + scale + len + payload
else NULL
Buffer->>Buffer: write NULL marker
end
Buffer-->>-Sender: chainable sender returned
Sender->>+Server: send encoded payload
Server-->>-Sender: response/ack
else does not support decimals
Sender-->>-Client: errDecimalNotSupported
end
sequenceDiagram
autonumber
participant Factory as sender factory
participant Server as QuestDB advert
participant Client as newLineSender
Server->>Factory: advertise capabilities (e.g., {2,3})
Factory->>Factory: detectProtocolVersion()
alt supports 3
Factory->>Client: create SenderV3 (embed V2)
else supports 2 only
Factory->>Client: create SenderV2
else unsupported
Factory-->>Client: error (close connection)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Potential attention points:
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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 |
@coderabbitai
coderabbitai
bot
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
tcp_sender_test.go (1)
145-148: Critical: Test expectation conflicts with updated protocol version support.This test expects
protocol_version=3to be rejected, butconf_parse.go(lines 172-173) now accepts ProtocolVersion3 for decimal support. This is causing the pipeline failure.Apply this diff to test an unsupported version:
{ name: "protocol_version", - config: "tcp::protocol_version=3;", - expectedErr: "current client only supports protocol version 1 (text format for all datatypes), 2 (binary format for part datatypes) or explicitly unset", + config: "tcp::protocol_version=4;", + expectedErr: "current client only supports protocol version 1 (text format for all datatypes), 2 (binary format for part datatypes), 3 (decimals) or explicitly unset", },integration_test.go (1)
142-155: Container image likely lacks ILP v3 decimals; bump image or gate tests.questdb/questdb:9.0.2 probably doesn’t support ILP v3 decimals. This explains the failing decimal tests. Either:
- Upgrade the container to the first release that includes decimal/ILP v3, or
- Gate decimal tests on server capabilities (see next comment).
I can propose a concrete tag once you confirm the target QuestDB version for decimals.
🧹 Nitpick comments (7)
sender.go (3)
109-118: Public API: DecimalColumn contract looks good; clarify mixed-type behavior across protocol versions.Docs say non-string values are supported (ScaledDecimal, DecimalMarshaler, shopspring) and strings encode text. It would help to note explicitly:
- Binary decimals require ProtocolVersion3; strings are accepted in text mode on earlier versions.
This avoids ambiguity for users who might pass ScaledDecimal on v1/v2.
494-498: Warn about explicitly forcing protocol on HTTP.The comment recommends setting a fixed version to avoid negotiation cost. Given decimal requires server v3 support, add a short warning that forcing v3 against an older server will fail (or lead to client-side errors when using DecimalColumn).
738-741: Validation message: keep concise and consistent.The error mentions "1 (text), 2 (floats/arrays), 3 (binary decimals) or unset". Consider simplifying and aligning with docs, e.g.:
"supported protocol versions: 1, 2, 3, or unset"interop_test.go (1)
157-185: parseDecimal64 may overflow on large values; prefer big.Int path.Current parsing uses int64 and will overflow for larger decimals. Switch to big.Int and construct with qdb.NewDecimal.
Apply this diff:
func parseDecimal64(s string) (qdb.ScaledDecimal, error) { // Remove whitespace s = strings.TrimSpace(s) // Check for empty string if s == "" { return qdb.ScaledDecimal{}, fmt.Errorf("empty string") } // Find the decimal point and remove it pointIndex := strings.Index(s, ".") if pointIndex != -1 { - s = strings.ReplaceAll(s, ".", "") + s = strings.ReplaceAll(s, ".", "") } - // Parse the integer part - unscaled, err := strconv.ParseInt(s, 10, 64) + // Parse the integer part using big.Int to avoid overflow + unscaledBI := new(big.Int) + _, ok := unscaledBI.SetString(s, 10) + if !ok { + return qdb.ScaledDecimal{}, fmt.Errorf("invalid integer: %s", s) + } - if err != nil { - return qdb.ScaledDecimal{}, err - } scale := 0 if pointIndex != -1 { scale = len(s) - pointIndex } - return qdb.NewDecimalFromInt64(unscaled, uint32(scale)), nil + return qdb.NewDecimal(unscaledBI, uint32(scale)), nil }http_sender.go (2)
122-125: V3 type embedding is fine; avoid re-declaring identical passthroughs.Since httpLineSenderV3 embeds httpLineSenderV2, methods from V2 are promoted automatically. You can drop the duplicate V3 passthroughs and keep only V3-specific overrides (DecimalColumn). This reduces maintenance churn.
650-713: Remove duplicated V3 passthrough methods; keep only DecimalColumn.All these V3 methods mirror V2 implementations. Rely on method promotion via embedding to avoid duplication. Keep just DecimalColumn on V3.
- func (s *httpLineSenderV3) Table(name string) LineSender { ... } - func (s *httpLineSenderV3) Symbol(name, val string) LineSender { ... } - func (s *httpLineSenderV3) Int64Column(name string, val int64) LineSender { ... } - func (s *httpLineSenderV3) Long256Column(name string, val *big.Int) LineSender { ... } - func (s *httpLineSenderV3) TimestampColumn(name string, ts time.Time) LineSender { ... } - func (s *httpLineSenderV3) StringColumn(name, val string) LineSender { ... } - func (s *httpLineSenderV3) BoolColumn(name string, val bool) LineSender { ... } - func (s *httpLineSenderV3) Float64Column(name string, val float64) LineSender { ... } - func (s *httpLineSenderV3) Float64Array1DColumn(name string, values []float64) LineSender { ... } - func (s *httpLineSenderV3) Float64Array2DColumn(name string, values [][]float64) LineSender { ... } - func (s *httpLineSenderV3) Float64Array3DColumn(name string, values [][][]float64) LineSender { ... } - func (s *httpLineSenderV3) Float64ArrayNDColumn(name string, values *NdArray[float64]) LineSender { ... } + // V3 inherits all V2 methods; only DecimalColumn differs. func (s *httpLineSenderV3) DecimalColumn(name string, val any) LineSender { s.buf.DecimalColumn(name, val) return s }decimal.go (1)
192-216: Avoid O(exp) multiplication; use big.Int.Exp for 10^exp.bigPow10 loops exp times; this is slow for large exponents. Replace with exponentiation by squaring.
-func bigPow10(exponent int) *big.Int { - if exponent <= 0 { - return big.NewInt(1) - } - result := big.NewInt(1) - ten := big.NewInt(10) - for i := 0; i < exponent; i++ { - result.Mul(result, ten) - } - return result -} +func bigPow10(exponent int) *big.Int { + if exponent <= 0 { + return big.NewInt(1) + } + return new(big.Int).Exp(big.NewInt(10), big.NewInt(int64(exponent)), nil) +}Optional: add a cheap overflow pre-check using bit-length estimate to avoid allocating huge numbers that we’ll reject later (based on 256‐bit cap).
Also applies to: 230-240
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
README.md(1 hunks)buffer.go(1 hunks)buffer_test.go(2 hunks)conf_parse.go(1 hunks)decimal.go(1 hunks)export_test.go(4 hunks)http_sender.go(5 hunks)http_sender_test.go(3 hunks)integration_test.go(2 hunks)interop_test.go(4 hunks)sender.go(4 hunks)sender_pool.go(1 hunks)tcp_sender.go(4 hunks)tcp_sender_test.go(1 hunks)test/interop/questdb-client-test(1 hunks)utils_test.go(3 hunks)
🧰 Additional context used
🪛 GitHub Actions: build
integration_test.go
[error] 593-593: decimal_type (TCP) test failed: Condition never satisfied during TestE2EValidWrites/decimal_type:_tcp#03
[error] 588-588: decimal_type (HTTP) test failed: Received error "current protocol version does not support decimal" during TestE2EValidWrites/decimal_type:_http#03
tcp_sender_test.go
[error] 155-155: TestTcpPathologicalCasesFromEnv/protocol_version failed: "dial tcp 127.0.0.1:9009: connect: connection refused" did not contain expected protocol-version guidance
🔇 Additional comments (29)
test/interop/questdb-client-test (2)
1-1: Implementation files not provided in this review.The PR objective describes substantial feature additions (ScaledDecimal type, binary encoding, DecimalColumn methods for HTTP and TCP senders, and protocol negotiation updates), but only the test submodule pointer is visible here. The main implementation in the go-questdb-client repository should be reviewed for:
- Correctness of ScaledDecimal type definition and validation
- Proper binary encoding/decoding of decimal values
- DecimalColumn method implementation for both HTTP and TCP senders
- Protocol version negotiation and error handling for unsupported protocol versions
- Edge cases (scale boundaries, large numbers, negative values)
- Consistency with the QuestDB server implementation (per tandem PR #6068)
Please ensure these files are included in the complete code review.
1-1: Submodule commit is valid and publicly accessible on feature branch.The commit
1aaa3f96ab06c6bef7d1b08400c418ef87562e56is valid and found on therd_decimalbranch ofhttps://github.com/questdb/questdb-client-test.git. However, verify:
- Feature branch reference is intentional for this PR (submodule points to
rd_decimal, notmain)- Comprehensive decimal test scenarios are implemented (binary/text formats, protocol versions, error cases)
- CI/CD pipeline passes with the updated submodule
conf_parse.go (1)
172-173: LGTM! Protocol version 3 support added correctly.The validation logic properly accepts ProtocolVersion3 and the error message accurately describes the supported versions including the new decimal support.
export_test.go (1)
66-77: LGTM! Test helpers properly extended for V3 senders.The changes consistently add support for the new
httpLineSenderV3andtcpLineSenderV3types across all helper functions, following the established pattern for V2 senders.Also applies to: 91-102, 116-127, 141-152
tcp_sender_test.go (1)
369-386: LGTM! Properly tests unsupported decimal in ProtocolVersion2.The test correctly verifies that attempting to use DecimalColumn with TCP ProtocolVersion2 returns an appropriate error message.
README.md (1)
191-221: LGTM! Clear and comprehensive decimal column documentation.The documentation effectively explains decimal support with practical examples covering ScaledDecimal types, shopspring compatibility, and string literals. The examples are correct and easy to follow.
http_sender_test.go (4)
830-836: LGTM! Test refactored to verify client picks highest supported version.Changing the server capabilities from
{2,3}to{2,4}properly tests that the client selects ProtocolVersion2 when the server offers a higher unsupported version (4).
841-846: LGTM! Test updated to verify error on unsupported protocol version.Changing from version
{3}to{4}correctly tests that the client returns an error when the server only supports a protocol version higher than what the client supports.
848-857: LGTM! Test properly verifies ProtocolVersion3 negotiation.This new test correctly verifies that when the server advertises support for versions
{2,3}, the client successfully negotiates and selects ProtocolVersion3.
927-944: LGTM! Properly tests unsupported decimal in ProtocolVersion2.The test correctly verifies that attempting to use DecimalColumn with HTTP ProtocolVersion2 returns an appropriate error message, mirroring the TCP test coverage.
utils_test.go (2)
29-29: LGTM! Necessary imports for new test helpers.The
encoding/base64andgolang.org/x/exp/slicesimports are required for the new test helper functions.Also applies to: 43-43
347-372: LGTM! Useful test helpers for decimal validation.The new helper functions properly support testing:
expectAnyLines: validates that received lines match any expected lineexpectBinaryBase64: validates binary data streams by decoding base64 and comparing bytesBoth implementations correctly handle the channel polling and assertions.
buffer_test.go (2)
42-53: LGTM! Clever test helper for shopspring compatibility.The
fakeShopspringDecimaltype provides a minimal implementation to test shopspring decimal compatibility without requiring the external dependency. TheCoefficient()andExponent()methods correctly satisfy the expected interface.
497-621: LGTM! Comprehensive decimal column test coverage.The test suite thoroughly validates decimal functionality:
- TestDecimalColumnText: verifies binary encoding for various decimal values including positive, negative, zero, NULL, and shopspring-compatible types
- TestDecimalColumnStringValidation: validates text decimal parsing for valid formats (integers, decimals, exponents, special tokens) and rejects invalid strings
- TestDecimalColumnErrors: properly tests error conditions for invalid scale, overflow, and unsupported types
All test cases are well-structured with clear expectations.
buffer.go (1)
576-615: LGTM! DecimalColumn implementation follows established patterns.The method correctly:
- Validates buffer state and writes the column name
- Handles string decimals by validating the format and appending the 'd' suffix
- Handles typed decimals by normalizing, encoding to binary, and writing the binary format with the
==prefix (matching the pattern inFloat64ColumnBinary)- Properly propagates errors via
lastErrand updateshasFieldsThe implementation is consistent with other column methods in the codebase.
sender.go (1)
264-268: ProtocolVersion3 addition is consistent.Enum extension is fine and forwards compatibility is preserved.
sender_pool.go (1)
317-320: Forwarder method is correct.pooledSender.DecimalColumn forwards and preserves chaining semantics, consistent with other columns.
interop_test.go (1)
82-85: Interop: explicitly pinning ProtocolVersion3 is fine for the fake servers.Given the test servers emulate protocol v3, this change is appropriate.
Also applies to: 101-104
tcp_sender.go (5)
53-56: V3 type embedding approach is clean.tcpLineSenderV3 reuses V2 behavior and adds DecimalColumn support. Good reuse.
143-164: Fail-fast on unsupported protocol: good; ensure conn is closed.You close the connection before returning error—good resource hygiene.
206-210: Correct: V1 TCP rejects DecimalColumn.Setting lastErr is consistent with other unsupported types.
374-377: V2 TCP rejects DecimalColumn; OK per spec.Consistent with ILP v2 not supporting binary decimals.
439-442: V3 TCP implements DecimalColumn via buffer.Matches the intended binary decimal path.
http_sender.go (4)
182-197: Protocol switch LGTM, clear error on unsupported versions.Construction for v1/v2/v3 looks correct; default returns a helpful error. No issues.
Please ensure a test covers the case where the server reports only unsupported versions (e.g., [4]) and that we surface the "unsupported protocol version" error.
308-312: Explicit decimal rejection for v1 path is correct.Setting last error prevents partial writes and matches existing array-unsupported behavior. Good.
530-551: Version negotiation logic is sound.Prefers v3, then v2, then v1; ignores unknown entries. Also updates filename limit. Looks good.
645-649: v2 HTTP correctly rejects DecimalColumn.Error aligns with behavior that DECIMAL needs protocol v3. Good.
decimal.go (2)
130-146: Scale/width checks are good; message will align after width fix.Encoding path sets 0/ nil for NULL; max‐scale guard is in ensureValidScale. LGTM after the 32‐byte fix above.
328-401: Are NaN/Infinity meant to be valid "decimal" literals?validateDecimalText accepts NaN/Infinity tokens; these usually apply to IEEE754 floats, not DECIMAL. If this is for float parsing, consider renaming to avoid confusion or gate tokens by type.
Would you confirm whether DECIMAL text inputs allow NaN/Infinity in ILP v3? If not, we should disallow them here or move this validator to the float path.
@coderabbitai
coderabbitai
bot
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
buffer_test.go(2 hunks)decimal.go(1 hunks)integration_test.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- buffer_test.go
🧰 Additional context used
🧬 Code graph analysis (1)
integration_test.go (3)
sender.go (9)
LineSender(54-227)NewLineSender(587-616)WithTcp(309-313)WithAddress(426-430)WithProtocolVersion(498-502)ProtocolVersion1(265-265)ProtocolVersion2(266-266)ProtocolVersion3(267-267)WithHttp(302-306)decimal.go (2)
NewDecimalFromInt64(85-94)NullDecimal(97-99)export_test.go (1)
ProtocolVersion(131-154)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build with Go 1.19.x
- GitHub Check: Build with Go 1.20.x
🔇 Additional comments (7)
integration_test.go (3)
496-533: LGTM!The decimal test case provides good coverage with text representation, binary encoding (positive and negative), and NULL values. The expected data correctly shows scale-3 formatting ("123.450") matching the DECIMAL(18,3) column type, even though the input uses scale-2 values.
538-572: Good extension to include ProtocolVersion3.The protocol version handling is consistent across TCP and HTTP paths. The switch structure cleanly supports auto-negotiation (case 0) and explicit version selection (cases 1, 2, 3).
576-582: LGTM!The skip logic correctly uses the actual negotiated protocol version from the sender and appropriately skips decimal tests for versions < 3. This addresses the previous review concern about hardcoded versions.
decimal.go (4)
34-46: LGTM!The constants and type definition are well-designed. The maxDecimalScale=76 aligns with Apache Arrow Decimal256 specification, and maxDecimalBytes=127 provides a client-side buffer limit (with server-side validation for the 256-bit limit).
58-99: LGTM!The constructor implementations are consistent and handle NULL values appropriately. NewDecimalFromInt64 efficiently converts int64 to two's complement representation, and all constructors properly normalize their inputs.
101-145: LGTM!The public methods are well-implemented with proper NULL handling and validation. The toBinary method correctly prepares the decimal for wire format, and the error messages accurately reflect the constraints.
147-240: LGTM!The conversion functions provide excellent interoperability with external decimal types. The duck-typing approach for shopspring/decimal is elegant, and the nil handling is comprehensive with proper reflection checks.
@coderabbitai
coderabbitai
bot
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Nitpick comments (1)
decimal.go (1)
230-240: Consider optimizing bigPow10 for large exponents.The current implementation uses a simple loop to compute 10^exponent. While correct for typical decimal scales (0-76), it could be optimized using exponentiation by squaring for better performance with large exponents.
Example optimization:
func bigPow10(exponent int) *big.Int { if exponent <= 0 { return big.NewInt(1) } - result := big.NewInt(1) - ten := big.NewInt(10) - for i := 0; i < exponent; i++ { - result.Mul(result, ten) - } - return result + // Use exponentiation by squaring for better performance + result := big.NewInt(10) + base := big.NewInt(10) + exp := exponent - 1 + for exp > 0 { + if exp&1 == 1 { + result.Mul(result, base) + } + base.Mul(base, base) + exp >>= 1 + } + return result }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
decimal.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build with Go 1.19.x
- GitHub Check: Build with Go 1.20.x
🔇 Additional comments (10)
decimal.go (10)
40-56: LGTM! Clean type design.The ScaledDecimal struct and interfaces are well-designed. The lowercase
shopspringDecimalinterface is appropriate for duck-type detection of external decimal libraries without requiring a direct dependency.
58-99: LGTM! Constructors handle NULL cases correctly.All constructor functions are well-implemented:
NewScaledDecimalcorrectly treats both nil and empty slices as NULL (now documented)NewDecimalcorrectly checks only for nil*big.IntNewDecimalFromInt64always creates valid decimals (appropriate for primitive types)NullDecimalprovides explicit NULL construction
101-118: LGTM! Accessor methods are correct.All accessor methods are well-implemented and maintain proper encapsulation. The
UnscaledValuemethod correctly returns a copy to prevent external mutation.
120-145: LGTM! Validation and binary conversion are correct.The scale validation correctly enforces the 76-digit maximum precision for Decimal256. The binary conversion includes appropriate defensive handling for edge cases (empty payload →
{0}).
147-215: LGTM! Type conversion and normalization logic are correct.The type conversion handles multiple input types correctly:
- Native
ScaledDecimaltypes (value and pointer)DecimalMarshalerinterface for custom types- Duck-typed shopspring/decimal via
shopspringDecimalinterfaceThe shopspring decimal conversion correctly interprets the coefficient/exponent representation and maps it to the scaled decimal format.
242-273: LGTM! Two's complement conversion is correct.The
bigIntToTwosComplementfunction correctly handles all cases:
- Zero values
- Positive values with proper sign-bit padding
- Negative values using standard two's complement representation
The trimming logic ensures minimal byte representation while preserving the sign.
275-326: LGTM! Normalization and conversion helpers are correct.All helper functions correctly implement two's complement operations:
normalizeTwosComplementensures consistent representationtrimTwosComplementremoves redundant padding bytestwosComplementToBigIntcorrectly decodes signed values
364-364: LGTM! Exponent validation correctly rejects malformed cases.The check
if digits == 0at line 364 correctly rejects exponents without preceding mantissa digits, including edge cases like ".e2" while still allowing valid forms like ".5e2" and "5.e2".
328-401: Overall validation logic is robust.The
validateDecimalTextfunction comprehensively validates decimal string formats, handling:
- Special tokens (NaN, Infinity)
- Sign prefixes
- Decimal points
- Scientific notation (exponent)
- Edge cases and malformed inputs
The logic correctly rejects invalid formats while accepting all valid decimal representations.
34-38: Based on the search results and code verification, I can now finalize the review comment analysis:
The 127-byte buffer limit is correctly implemented and tested. The error message at line 142 accurately states the protocol constraint, and the design intentionally allows larger values to be rejected by the server, consistent with how QuestDB handles validation for extensible data types.
@coderabbitai
coderabbitai
bot
left a comment
There was a problem hiding this 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 (2)
buffer_test.go (2)
546-595: LGTM: Good validation coverage for string-based decimals.The test validates various numeric string formats including integers, decimals, scientific notation, and special tokens (NaN, Infinity), plus error handling for malformed inputs.
Optional: Line 590 could use more specific error assertions (e.g., checking for "invalid decimal format" or specific format requirements) rather than just checking for "decimal" substring.
597-628: Good error coverage with opportunity for boundary testing.The test suite validates scale limits, overflow, type checking, and null-only column rejection.
Consider adding boundary tests:
- Scale boundary: Test scale=99 to confirm it's the maximum valid value, since scale=100 fails
- Size boundary: Test a value near the 127-byte limit to validate the exact threshold, rather than only testing an extremely large value (2^2100)
These tests would reduce the risk of off-by-one errors in the validation logic.
Minor: The "no column" test name (line 622) could be more descriptive, e.g., "null only decimal counts as no column" to clarify what behavior is being validated.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
buffer.go(1 hunks)buffer_test.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- buffer.go
🧰 Additional context used
🧬 Code graph analysis (1)
buffer_test.go (2)
decimal.go (3)
NewDecimalFromInt64(85-94)NewDecimal(73-82)NullDecimal(97-99)export_test.go (1)
Messages(56-79)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build with Go 1.19.x
- GitHub Check: Build with Go 1.20.x
🔇 Additional comments (2)
buffer_test.go (2)
42-53: LGTM: Clean test adapter for decimal interface compatibility.This helper enables testing compatibility with shopspring-style decimal libraries without introducing an external dependency.
497-544: LGTM: Comprehensive coverage of decimal encoding scenarios.The test suite validates binary encoding for positive, negative, zero, null, shopspring-compatible, and nil-pointer cases. The hardcoded byte expectations ensure precise encoding validation.
@coderabbitai
coderabbitai
bot
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
♻️ Duplicate comments (1)
decimal.go (1)
58-64: Doc and behavior now match (nil/empty => NULL). LGTM.
🧹 Nitpick comments (3)
decimal.go (3)
34-38: Clarify maxDecimalBytes (encoding cap) and document it.127 looks like the int8-encoded length limit, not a numeric range. Add a comment to prevent future confusion.
const ( decimalBinaryTypeCode byte = 0x17 maxDecimalScale uint32 = 76 - maxDecimalBytes int = 127 + // Max encoded length of the unscaled two's‐complement payload in ILP v3. + // This is an encoding cap (int8 length field), not a numeric range check. + maxDecimalBytes int = 127 )
141-144: Use the constant in the error to avoid magic numbers.Message currently hardcodes 127; tie it to maxDecimalBytes and clarify it’s an encoding limit.
- return 0, nil, fmt.Errorf("decimal value exceeds 127-bytes limit (got %d bytes)", len(payload)) + return 0, nil, fmt.Errorf("decimal value exceeds encoded length limit (%d bytes; got %d)", maxDecimalBytes, len(payload))
335-338: DECIMAL should not accept NaN/Infinity; confirm intent.If this validator is for DECIMAL, allow-listing NaN/Infinity is surprising and likely rejected server-side. Either disallow here or split float/decimal validators.
- switch text { - case "NaN", "Infinity", "+Infinity", "-Infinity": - return nil - } + // DECIMAL does not support NaN/Infinity; let parsing reject them explicitly. + switch text { + case "NaN", "Infinity", "+Infinity", "-Infinity": + return fmt.Errorf("decimal literal does not support %q", text) + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
decimal.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build with Go 1.19.x
- GitHub Check: Build with Go 1.20.x
🔇 Additional comments (1)
decimal.go (1)
120-128: Confirm server-side max scale (76) stays in sync.If QuestDB adjusts DECIMAL precision/scale, mirror here to avoid client/server mismatch; consider linking to the source of truth in a comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We seem to do multiple allocations
func BenchmarkHttpLineSenderBatch1000_2(b *testing.B) { ctx := context.Background() srv, err := newTestHttpServer(readAndDiscard) assert.NoError(b, err) defer srv.Close() sender, err := qdb.NewLineSender(ctx, qdb.WithHttp(), qdb.WithAddress(srv.Addr())) assert.NoError(b, err) b.ResetTimer() for i := 0; i < b.N; i++ { for j := 0; j < 1000; j++ { d := qdb.NewDecimalFromInt64(int64(i), 3) sender. Table(testTable). DecimalColumn("d_col", d). At(ctx, time.UnixMicro(int64(1000*i))) } sender.Flush(ctx) sender.Close(ctx) } }
With the above benchmark I see this:
BenchmarkHttpLineSenderBatch1000_2-24 20016 54712 ns/op 72001 B/op 3000 allocs/op
So, 3 allocations and 54us per row. If I change the benchmark to write a single double column, I get this:
BenchmarkHttpLineSenderBatch1000_2-24 189351 6316 ns/op 0 B/op 0 allocs/op
README.md
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we want to support this 3rd-party library? I've checked it and the last release was 1.5 years ago while there are 100+ open GH issues, some of which look like straightforward bugs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems to be the only popular decimal integration in the golang ecosystem.
Having initial support for it makes it easier for customers to connect to QDB.
README.md
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: why don't we accept just one option: nil?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed this part, the senders no longer sends NULL through the wire. Instead, a nil value is ignored, same as other types.
buffer.go
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be great to use strong typing in the client API: we could split this method into multiple ones accepting string, ScaledDecimal, etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, there are now 3 separate functions:
DecimalColumnStringDecimalColumnShopspringDecimalColumnScaled
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At this point, we already wrote column name to the buffer. ILP requires the value to be included. Can't we interpret zero length as null on the server side?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, we don't even need to send the value.
In the fix, we no longer send the field if the value is null.
decimal.go
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we allow the payload to be up to 127 bytes while Decimal256 is 32 bytes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was to future proof it, if we ever want to increase the size of decimals. I reverted this to have a strict limit of 32-bytes in order to remove unnecessary allocations.
buffer_test.go
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: the test cases don't look like text.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good, catch. Fixed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This trimming and padding code isn't very simple, so we need more test cases around it:
- Very small negative numbers (-1, -2, -127, -128, -129)
- Boundary values at byte transitions
- Values requiring sign extension
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it, added a few cases.
buffer_test.go
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some test cases here are failing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry about that, fixed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Both e10 and .e10 seem to pass this validation while they shouldn't.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this is only a light validation to ensure that there aren't any forbidden chars.
@coderabbitai
coderabbitai
bot
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 3
🧹 Nitpick comments (4)
utils_test.go (2)
347-356: Semantics of expectAnyLines are "first match wins."This consumes one line and returns true on the first match, dropping prior non-matching lines. That’s fine for your use, just flagging the behavior. If you need to assert that "at least one of N lines was produced overall," consider draining until timeout or match.
358-372: Avoid potential 10s hangs when extra data arrives.If the server sends more bytes than expected, equality will never be reached and the Eventually will time out. Consider bailing out early when len(actual) > len(data), or assert prefix until lengths match.
func expectBinaryBase64(t *testing.T, linesCh chan string, expected string) { data, err := base64.StdEncoding.DecodeString(expected) assert.NoError(t, err) actual := make([]byte, 0) assert.Eventually(t, func() bool { select { case l := <-linesCh: actual = append(actual, []byte(l+"\n")...) + if len(actual) > len(data) { + return false + } default: return false } return slices.Equal(data, actual) }, 10*time.Second, 100*time.Millisecond) }interop_test.go (1)
157-186: parseDecimal64 scope is intentionally narrow; document limitations.This only supports simple fixed-point (no exponent) and int64 range. If interop cases expand, note the limitation in a comment to avoid confusion.
-// parseDecimal64 quick and dirty parser for a decimal64 value from its string representation +// parseDecimal64: quick and dirty parser for decimal64 from plain fixed-point strings. +// Limitations: no exponent support; unscaled must fit into int64; intended for tests only.decimal.go (1)
154-164: Use big.Int.Exp for pow10; current loop is O(exp).Improves performance for larger exponents without changing behavior.
func bigPow10(exponent int) *big.Int { if exponent <= 0 { return big.NewInt(1) } - result := big.NewInt(1) - ten := big.NewInt(10) - for i := 0; i < exponent; i++ { - result.Mul(result, ten) - } - return result + return new(big.Int).Exp(big.NewInt(10), big.NewInt(int64(exponent)), nil) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
README.md(1 hunks)buffer.go(1 hunks)buffer_test.go(2 hunks)decimal.go(1 hunks)http_sender.go(5 hunks)http_sender_test.go(3 hunks)integration_test.go(2 hunks)interop_test.go(4 hunks)sender.go(5 hunks)sender_pool.go(1 hunks)tcp_sender.go(4 hunks)tcp_sender_test.go(1 hunks)utils_test.go(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- README.md
- integration_test.go
- http_sender_test.go
- sender_pool.go
- tcp_sender_test.go
🧰 Additional context used
🧬 Code graph analysis (6)
tcp_sender.go (3)
sender.go (4)
ProtocolVersion1(282-282)ProtocolVersion2(283-283)ProtocolVersion3(284-284)LineSender(55-244)decimal.go (2)
ShopspringDecimal(46-49)ScaledDecimal(40-44)ndarray.go (1)
NdArray(27-31)
buffer_test.go (1)
decimal.go (3)
NewDecimal(72-87)ScaledDecimal(40-44)NewDecimalFromInt64(90-101)
sender.go (1)
decimal.go (2)
ScaledDecimal(40-44)ShopspringDecimal(46-49)
interop_test.go (2)
sender.go (7)
NewLineSender(604-633)WithTcp(326-330)WithAddress(443-447)WithProtocolVersion(515-519)ProtocolVersion3(284-284)WithHttp(319-323)LineSender(55-244)decimal.go (2)
ScaledDecimal(40-44)NewDecimalFromInt64(90-101)
buffer.go (1)
decimal.go (2)
ScaledDecimal(40-44)ShopspringDecimal(46-49)
http_sender.go (3)
sender.go (4)
ProtocolVersion1(282-282)ProtocolVersion2(283-283)ProtocolVersion3(284-284)LineSender(55-244)decimal.go (2)
ShopspringDecimal(46-49)ScaledDecimal(40-44)ndarray.go (1)
NdArray(27-31)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build with Go 1.19.x
- GitHub Check: Build with Go 1.20.x
🔇 Additional comments (22)
utils_test.go (1)
29-29: No action needed for these small additions.Imports and widening protocol versions to include 3 look good.
Also applies to: 44-44, 77-77
interop_test.go (2)
82-85: Good: exercising ProtocolVersion3 in both TCP and HTTP paths.This aligns interop with decimal support and v3 framing.
Also applies to: 101-104
109-155: execute flow is clean and extensible.Symbols, columns, and result assertions are well-structured. No issues found.
sender.go (1)
755-758: Validation message reads well and includes v3 — LGTM.buffer_test.go (6)
42-54: LGTM!The test helper cleanly implements the
ShopspringDecimalinterface for testing purposes.
497-532: LGTM!The test cases correctly validate binary decimal encoding for positive, negative, and zero values with appropriate scales.
534-606: LGTM!Comprehensive boundary testing for two's complement encoding. The sign extension cases correctly validate that positive values with high bit set and negative values are properly extended.
608-650: LGTM!The tests correctly validate Shopspring decimal conversion semantics where negative exponents map to scale and positive exponents multiply the coefficient.
652-701: LGTM!Thorough validation of both valid and invalid decimal string formats, with proper error checking for malformed inputs.
703-724: LGTM!The error test cases appropriately validate scale limits, overflow conditions, and nil handling.
tcp_sender.go (5)
53-55: LGTM!Clean struct declaration following the established embedding pattern for protocol version hierarchy.
143-164: LGTM!Protocol version routing is correct with proper backward compatibility (defaulting to V1) and appropriate error handling that closes the connection for unsupported versions.
206-219: LGTM!Decimal methods on the base sender correctly signal not-supported for protocol V1 while maintaining the fluent interface.
384-397: LGTM!Protocol V2 correctly signals that decimal columns are not supported, consistent with the feature being added only in V3.
399-472: LGTM!The V3 sender implementation correctly delegates to buffer methods for all column types, including the new decimal support. The fluent interface is properly maintained.
http_sender.go (7)
122-124: LGTM!Clean struct declaration consistent with the tcp_sender pattern.
182-197: LGTM!Protocol version routing correctly returns the appropriate sender type with proper error handling for unsupported versions.
308-321: LGTM!Decimal methods correctly signal not-supported for protocol V1.
540-557: LGTM!The server settings parser correctly prioritizes protocol version 3 and properly falls back to V2 or V1 based on server capabilities.
655-668: LGTM!Protocol V2 correctly signals that decimal columns are not supported.
670-728: LGTM!The V3 sender correctly implements all standard column methods by delegating to the buffer.
730-743: LGTM!Decimal column methods correctly delegate to buffer implementations, enabling full decimal support in protocol V3.
Thank you for the review and the benchmark @puzpuzpuz
I addressed most of your reviews in d2b6670.
We no longer have unnecessary allocations when sending decimal through the wire with ScaledDecimal and string:
BenchmarkHttpLineSenderBatch1000_2-8 27495 42933 ns/op 1 B/op 0 allocs/op
There are 3 separate functions to send decimals now:
DecimalColumnScaled(name string, val ScaledDecimal) LineSenderDecimalColumnString(name string, val string) LineSenderDecimalColumnShopspring(name string, val ShopspringDecimal) LineSender
Uh oh!
There was an error while loading. Please reload this page.
This PR adds decimal support.
This is a tandem pr for:
Usage
Progress
Summary by CodeRabbit
New Features
Documentation
Tests