feat: support decimal #58
Conversation
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
|
Warning Rate limit exceeded@puzpuzpuz has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 7 minutes and 14 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 (2)
WalkthroughAdds ProtocolVersion3 and DECIMAL support: introduces a Decimal type and ShopspringDecimal interface, buffer decimal encoding and DecimalColumn APIs (binary and textual), sender V3 variants with protocol negotiation updates, nil‑skipping array writer changes, expanded tests, and README documentation. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant C as Client (LineSender)
participant F as Factory
participant S as Sender (V1/V2/V3)
participant B as Buffer
participant D as Decimal helpers
participant Q as QuestDB
C->>F: request sender (server advertises protocols)
F->>F: detectProtocolVersion()
alt server supports 3
F->>S: create SenderV3
else supports 2
F->>S: create SenderV2
else supports 1
F->>S: create SenderV1
else
F-->>C: error / close
end
C->>S: DecimalColumn(name, val)
alt Sender is V3
S->>B: DecimalColumn(name, val)
B->>D: normalize & validate (scale, twos-complement or text)
alt textual input
D->>B: validateText -> write textual decimal payload
else binary input
D->>B: encode binary decimal (scale, offset, payload)
else NULL
B->>B: mark NULL via offset semantics
end
B->>Q: send encoded payload
Q-->>S: response
else Sender is V1/V2
S-->>C: errDecimalNotSupported
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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
puzpuzpuz
left a comment
puzpuzpuz
left a comment
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 There are 3 separate functions to send decimals now:
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
tcp_sender_test.go (1)
463-488: Consider removing Close from the benchmark loop.The Close call inside the loop includes connection teardown overhead in each iteration, which may not accurately reflect typical usage patterns where a sender is reused across multiple batches.
Apply this diff to move Close outside the loop:
func BenchmarkHttpLineSenderDecimal(b *testing.B) { const decimalStr = "123456.789" 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) + defer sender.Close(ctx) b.ResetTimer() for i := 0; i < b.N; i++ { for j := 0; j < 1000; j++ { d := qdb.NewDecimalFromInt64(int64(i), 3) sender. Table(testTable). DecimalColumnScaled("dec_col", d). DecimalColumnString("dec_col2", decimalStr). At(ctx, time.UnixMicro(int64(1000*i))) } sender.Flush(ctx) - sender.Close(ctx) } }decimal.go (1)
140-150: Consider using big.Int.Exp for better performance.The current O(n) loop works but big.Int.Exp uses exponentiation-by-squaring for O(log n) complexity, which is faster for large exponents.
Apply this diff for better performance:
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 (5)
buffer.go(1 hunks)buffer_test.go(2 hunks)decimal.go(1 hunks)sender.go(5 hunks)tcp_sender_test.go(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
sender.go (1)
decimal.go (2)
ScaledDecimal(40-44)ShopspringDecimal(46-49)
buffer.go (1)
decimal.go (2)
ScaledDecimal(40-44)ShopspringDecimal(46-49)
buffer_test.go (1)
decimal.go (3)
NewDecimal(72-87)ScaledDecimal(40-44)NewDecimalFromInt64(90-101)
tcp_sender_test.go (2)
sender.go (6)
NewLineSender(604-633)WithTcp(326-330)WithAddress(443-447)WithProtocolVersion(515-519)ProtocolVersion2(283-283)WithHttp(319-323)decimal.go (1)
NewDecimalFromInt64(90-101)
⏰ 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.20.x
- GitHub Check: Build with Go 1.19.x
🔇 Additional comments (24)
tcp_sender_test.go (1)
369-386: LGTM!The test correctly verifies that DecimalColumn operations fail gracefully under protocol v2 with an appropriate error message.
buffer_test.go (5)
42-53: LGTM!Clean test helper that implements the ShopspringDecimal interface without requiring an external dependency.
497-532: LGTM!Comprehensive test covering positive, negative, and zero decimals with appropriate scale values and binary encoding validation.
534-606: LGTM!Excellent boundary-case coverage for two's complement sign extension and trimming logic, addressing previous review feedback.
608-655: LGTM!Thorough testing of Shopspring decimal conversion, covering both negative exponents (scaling) and positive exponents (multiplication), plus sign extension cases.
657-729: LGTM!Comprehensive validation tests for decimal string formats and error conditions. The tests appropriately cover valid formats (integers, decimals, exponents, special tokens) and invalid formats (empty, malformed exponents, invalid characters), plus error scenarios like scale limits and overflow.
buffer.go (4)
576-585: LGTM!Correct NULL handling that returns early without emitting field separators, addressing past review feedback.
587-604: LGTM!Correct binary encoding implementation for scaled decimals with proper scale validation and error handling.
606-623: LGTM!Clean text-based decimal encoding with proper validation ensuring only valid decimal strings are written.
625-645: LGTM!Proper handling of Shopspring decimals with correct NULL checks before and after conversion, ensuring no field separators are emitted for NULL/nil values.
sender.go (5)
43-43: LGTM!Clear error constant for protocol version gating of decimal support.
110-117: LGTM!Well-documented interface method for string-based decimal columns.
119-135: LGTM!Clear interface methods for binary decimal encoding with consistent documentation.
284-284: LGTM!ProtocolVersion3 constant added with appropriate documentation explaining decimal binary encoding support.
Also applies to: 511-514
755-758: LGTM!Protocol version validation correctly extended to support ProtocolVersion3 with a clear, informative error message.
decimal.go (9)
33-36: LGTM!Constants correctly defined: type code 0x17 for binary decimals and maxDecimalScale=76 aligns with ILP v3 Decimal256 precision limit.
38-49: LGTM!Clean type definitions: ScaledDecimal with 32-byte storage (256-bit) and offset-based NULL representation, plus ShopspringDecimal interface for third-party library compatibility.
51-68: LGTM!Constructor correctly handles both nil and empty slices as NULL (offset=32) and normalizes non-empty input via normalizeTwosComplement.
70-87: LGTM!Clean constructor with proper nil handling and delegation to bigIntToTwosComplement for conversion.
89-101: LGTM!Efficient int64 conversion that avoids big.Int allocation, with correct trimming and offset calculation.
103-116: LGTM!Correct NULL detection and scale validation with appropriate max limit of 76.
118-138: LGTM!Correct conversion logic for Shopspring decimals, handling both positive exponents (multiplication) and negative exponents (scale). Note that large exponents are not bounded here but will be validated downstream during encoding.
152-182: LGTM!Correct two's complement conversion: zero is encoded as a single 0x00 byte (offset=31, not NULL), positive values get sign extension padding when needed, and negative values use proper two's complement formula with sign extension.
184-292: LGTM!All three functions are correctly implemented:
- normalizeTwosComplement properly aligns trimmed bytes in the 32-byte array
- trimTwosComplement removes redundant sign extension bytes
- validateDecimalText correctly validates decimal literals, properly rejecting edge cases like ".e2" (no mantissa digits) while accepting ".5e2" and "5.e2"
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
decimal.go (1)
140-150: Consider optimizing bigPow10 with big.Int.Exp.The current loop-based implementation works but could be more efficient for larger exponents. Consider using exponentiation-by-squaring:
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) }This would reduce O(n) multiplications to O(log n) operations.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
README.md(1 hunks)buffer_test.go(2 hunks)decimal.go(1 hunks)http_sender_test.go(5 hunks)sender.go(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
sender.go (1)
decimal.go (2)
Decimal(39-43)ShopspringDecimal(45-48)
http_sender_test.go (3)
sender.go (4)
NewLineSender(604-633)WithHttp(319-323)WithAddress(443-447)ProtocolVersion3(284-284)export_test.go (1)
ProtocolVersion(131-154)decimal.go (2)
NewDecimalFromInt64(90-101)NewDecimal(72-87)
buffer_test.go (2)
decimal.go (4)
NewDecimal(72-87)Decimal(39-43)NewDecimalFromInt64(90-101)NewDecimalUnsafe(52-68)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.20.x
- GitHub Check: Build with Go 1.19.x
🔇 Additional comments (13)
README.md (1)
191-220: LGTM! Clear and comprehensive decimal documentation.The new "Decimal columns" section provides excellent documentation with practical examples for all three decimal methods. The examples clearly demonstrate scale handling and the different input types supported.
buffer_test.go (2)
42-54: LGTM! Well-designed test helper.The
fakeShopspringDecimaltype properly implements theShopspringDecimalinterface, enabling thorough testing of Shopspring-style decimal integration without introducing an external dependency in tests.
497-766: LGTM! Comprehensive decimal test coverage.The test suite thoroughly exercises decimal functionality including:
- Basic encoding for positive, negative, and zero values
- Boundary conditions and sign extension (addressing previous review feedback)
- Shopspring decimal conversion
- String validation for valid and invalid inputs
- Unsafe construction paths
- Error cases (invalid scale, overflow, missing columns)
The tests validate both correct behavior and proper error handling.
sender.go (3)
43-43: LGTM! Appropriate error for version compatibility.The
errDecimalNotSupportederror provides clear feedback when decimal operations are attempted on protocol versions that don't support them.
110-136: LGTM! Well-documented decimal methods.The three decimal column methods are properly documented with clear descriptions of their serialization behavior and character restrictions. The separation into text (
DecimalColumnFromString) and binary (DecimalColumn,DecimalColumnShopspring) methods gives users flexibility.
284-284: LGTM! Clean protocol version extension.The addition of
ProtocolVersion3is well-integrated:
- Constant properly added to the enum
- Documentation updated to reference ILP v3 and decimal support
- Validation range correctly extended to accept version 3
Also applies to: 504-519, 755-758
decimal.go (4)
38-48: LGTM! Clean type definitions.The
Decimalstruct uses a fixed 32-byte array with offset tracking for efficient storage, and theShopspringDecimalinterface provides a minimal, clean integration point for third-party decimal types.
50-101: LGTM! Well-designed constructors with proper NULL handling.The three constructors provide flexibility:
NewDecimalUnsafe: for raw byte inputNewDecimal: forbig.IntvaluesNewDecimalFromInt64: optimized fast pathAll correctly handle NULL cases (nil/empty input → offset=32) and the int64 path avoids allocations.
152-183: LGTM! Correct two's complement conversion.The zero-value handling at line 154 correctly returns
offset=31(not 32), representing a non-NULL single-byte zero. The positive and negative paths properly handle sign extension and normalization. This addresses previous review feedback.
223-293: LGTM! Thorough decimal text validation.The validation correctly handles:
- Special tokens (NaN, Infinity)
- Sign prefixes
- Decimal points
- Exponent notation (e/E with optional sign)
- Invalid characters and malformed input
The logic properly rejects edge cases like empty strings, lone signs, and incomplete exponents.
http_sender_test.go (3)
828-858: LGTM! Comprehensive protocol negotiation testing.The updated tests properly cover protocol version negotiation scenarios:
- Server advertising future versions (v4) correctly results in an error
- Server advertising v3 (lines 849-858) correctly selects
ProtocolVersion3This ensures robust version negotiation behavior.
928-945: LGTM! Proper version compatibility check.This test correctly verifies that attempting to use decimal columns on protocol v2 results in the "current protocol version does not support decimal" error, ensuring users get clear feedback when using an incompatible version.
1021-1049: LGTM! Useful performance benchmark.The benchmark exercises all three decimal methods (both
DecimalColumnoverloads andDecimalColumnFromString) across 1000 messages per iteration, providing valuable performance data. According to the PR comments, this achieves zero allocations per operation.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
decimal.go (1)
118-150: Conversion logic is correct; consider optional bounds check on exponent.The Shopspring decimal conversion correctly handles positive/negative exponents and NULL coefficients. While large positive exponents will eventually be caught by the 256-bit check in
bigIntToTwosComplement, an early bounds check (e.g., rejecting exp > 76) could prevent intermediate over-allocation, though this is low priority given final validation will catch it.
📜 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.20.x
- GitHub Check: Build with Go 1.19.x
🔇 Additional comments (5)
decimal.go (5)
33-48: LGTM! Clean type definitions with efficient fixed-size storage.The fixed [32]byte array for unscaled values avoids allocations, and the maxDecimalScale constant correctly matches Arrow Decimal256's 76-digit precision limit.
50-87: LGTM! NULL handling and constructor delegation are correct.Both constructors properly handle NULL cases and delegate to appropriate conversion functions. The distinction between strict nil checking in
NewDecimaland lenient empty-slice checking inNewDecimalUnsafeis appropriate for their respective input types.
89-101: LGTM! Efficient int64 conversion with no heap allocations.The implementation correctly handles positive, negative, and zero values using stack-allocated arrays and proper two's complement trimming.
152-216: LGTM! Two's complement conversion and normalization are correctly implemented.The zero case now correctly returns offset=31 (addressed from past review), and the positive/negative conversion logic properly handles sign extension, trimming, and 256-bit bounds checking. The trimming algorithm correctly preserves minimal two's complement representation for both positive and negative values.
218-288: LGTM! Decimal text validation is comprehensive and correct.The validation correctly handles all edge cases including bare exponents (rejected), dotted decimals with exponents (accepted), and special IEEE 754 values. The fix from past reviews properly prevents invalid forms like ".e2" while allowing valid forms like ".5e2" and "5.e2".
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
buffer.go (2)
89-89: Clarify the relationship to decimal support.The
arrayElemNullconstant appears to be for null array handling rather than decimal support. Is this part of the broader ProtocolVersion3 changes mentioned in the AI summary, or should it be in a separate commit?
625-645: Optional refactor suggestion is valid: consider removing the redundant early error check for consistency.The explicit
b.lastErrcheck at lines 629-631 is indeed inconsistent with the codebase pattern. Most other column methods—includingInt64Column,Float64Column,TimestampColumn, andDecimalColumn—don't have this early check; they rely onprepareForField()(which checksb.lastErrat line 415) to guard against errors. Removing this check inDecimalColumnShopspringwould align with the established pattern across the codebase.The early check may have been added as a micro-optimization to avoid the
convertShopspringDecimal()call when an error already exists, but this optimization is not consistently applied elsewhere.Apply the suggested diff if consistency is preferred:
func (b *buffer) DecimalColumnShopspring(name string, val ShopspringDecimal) *buffer { if val == nil { return b } - if b.lastErr != nil { - return b - } dec, err := convertShopspringDecimal(val) if err != nil { b.lastErr = err return b } if dec.isNull() { // Don't write null decimals return b } if !b.prepareForField() { return b } return b.decimalColumn(name, dec) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
buffer.go(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
buffer.go (1)
decimal.go (2)
Decimal(39-43)ShopspringDecimal(45-48)
⏰ 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.20.x
- GitHub Check: Build with Go 1.19.x
🔇 Additional comments (3)
buffer.go (3)
576-585: LGTM! NULL handling is correct.The NULL check before
prepareForField()ensures no stray separators are written for null decimals, properly addressing the issue from previous reviews.
606-623: No issues found — validation is comprehensive.The
validateDecimalTextfunction handles all mentioned edge cases: empty strings (line 221-223), invalid formats with multiple dots (line 248-250), missing digits (line 284-286), invalid characters (line 280), and complete exponent validation including missing or incomplete exponents (lines 253-278). TheDecimalColumnFromStringmethod correctly validates input before writing and returns early on validation failure.
587-604: Verify the protocol specification for DECIMAL binary encoding.The
decimalBinaryTypeCodeconstant is correctly defined as0x17in decimal.go (line 34), and the encoding logic appears syntactically sound.However, the original review comment referenced an "ILP protocol version 3 DECIMAL specification," which does not appear to exist. QuestDB's ILP protocol supports only versions 1 (text ILP) and 2 (binary extensions for f64 and arrays), with no published DECIMAL binary format.
Before approving this code, confirm:
- What protocol version/specification is this implementation targeting?
- Is this a custom QuestDB extension beyond the official ILP spec?
- Has the encoding format been validated against the intended protocol?
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
integration_test.go (1)
501-501: Use idiomatic Go variable naming.The variable name
neg_duses underscore style, which is not idiomatic Go. Consider renaming tonegDfor consistency with Go naming conventions.- neg_d, err := qdb.NewDecimal(big.NewInt(-12345), 2) + negD, err := qdb.NewDecimal(big.NewInt(-12345), 2) if err != nil { return err } err = s. Table(testTable). DecimalColumnFromString("text_col", "123.45"). DecimalColumn("binary_col", d). - DecimalColumn("binary_neg_col", neg_d). + DecimalColumn("binary_neg_col", negD).Apply similar change at line 520:
- neg_d = qdb.NewDecimalFromInt64(-12346, 2) + negD = qdb.NewDecimalFromInt64(-12346, 2) return s. Table(testTable). DecimalColumnFromString("text_col", "123.46"). DecimalColumn("binary_col", d). - DecimalColumn("binary_neg_col", neg_d). + DecimalColumn("binary_neg_col", negD).Also applies to: 520-520
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
integration_test.go(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
integration_test.go (3)
sender.go (9)
LineSender(55-244)NewLineSender(604-633)WithTcp(326-330)WithAddress(443-447)WithProtocolVersion(515-519)ProtocolVersion1(282-282)ProtocolVersion2(283-283)ProtocolVersion3(284-284)WithHttp(319-323)decimal.go (2)
NewDecimalFromInt64(90-101)NewDecimal(72-87)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.20.x
- GitHub Check: Build with Go 1.19.x
🔇 Additional comments (6)
integration_test.go (6)
34-34: LGTM: Import addition supports skip logic.The
stringsimport is correctly added to support thestrings.Containscheck in the runtime skip logic for decimal tests.
547-547: LGTM: Clean extension to protocol version 3.The iteration and switch statements consistently handle protocol version 3 for both TCP and HTTP transports, following the established pattern for versions 1 and 2.
Also applies to: 559-580
585-591: LGTM: Runtime version-based skip logic addresses past feedback.The code now queries the actual protocol version from the sender and uses it to conditionally skip tests, resolving the previous concern about hardcoded version assumptions. The skip conditions correctly gate:
- Double array tests for protocol version < 2
- Decimal tests for protocol version < 3
500-501: Verify scale conversion behavior.The test creates decimals with scale 2 (e.g.,
NewDecimalFromInt64(12345, 2)), but the expected column type isDECIMAL(18,3)with scale 3, and expected values show 3 decimal places ("123.450"). This suggests the server normalizes to a different scale than what's sent.Please confirm this scale conversion (2 → 3) is the intended behavior, as it could cause confusion if the client and server scales don't match as expected.
Also applies to: 516-520
496-542: The original review comment is based on incorrect assumptions.The
DecimalColumnShopspringimplementation inbuffer.go:626-627explicitly returns early whenval == nil, meaning thebinary_null_colcolumn is intentionally never added to the message. This is the designed behavior. The test correctly omits this column from the expected results because it was never sent to the server.Likely an incorrect or invalid review comment.
143-143: QuestDB 9.2.0 image exists but lacks published release notes—verify decimal support claim.The container image exists on Docker Hub, but GitHub has no published release notes for version 9.2.0. The comment states the update is for decimal support, but decimal type support remains an open feature request as of the most recent documentation. The latest documented QuestDB release (9.1.0) introduced nanosecond timestamps and performance improvements, not decimal support. Verify that 9.2.0 actually includes the decimal functionality required by your tests before merging.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tcp_sender_test.go (1)
369-386: Consider testing all three decimal methods for completeness.The test correctly verifies that
DecimalColumnFromStringfails with ProtocolVersion2. For comprehensive coverage, you could also testDecimalColumn(for ScaledDecimal) andDecimalColumnShopspringmethods, as all three are part of the new decimal API.Example extension:
err = sender. Table(testTable). DecimalColumnFromString("price", "12.99"). At(ctx, time.UnixMicro(1)) assert.Error(t, err) assert.Contains(t, err.Error(), "current protocol version does not support decimal") + + dec, _ := qdb.NewDecimalFromInt64(1299, 2) + err = sender. + Table(testTable). + DecimalColumn("price", dec). + At(ctx, time.UnixMicro(2)) + assert.Error(t, err) + assert.Contains(t, err.Error(), "current protocol version does not support decimal")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tcp_sender_test.go(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tcp_sender_test.go (1)
sender.go (5)
NewLineSender(604-633)WithTcp(326-330)WithAddress(443-447)WithProtocolVersion(515-519)ProtocolVersion2(283-283)
⏰ 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.20.x
- GitHub Check: Build with Go 1.19.x
🔇 Additional comments (2)
tcp_sender_test.go (2)
146-147: LGTM! Error message accurately reflects V3 support.The updated error message correctly documents that protocol version 3 (with decimal support) is now available alongside versions 1 and 2.
395-395: Good practice: explicit protocol version pins benchmark baseline.Specifying
ProtocolVersion2ensures these benchmarks remain stable and comparable over time, even if the default protocol version changes in future releases.Also applies to: 434-434
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
buffer.go (1)
669-699: Fix: NULL arrays leave stray separators, breaking ILP.All four array methods (
Float64Array1DColumn,Float64Array2DColumn,Float64Array3DColumn,Float64ArrayNDColumn) callprepareForField()before checking for nil arrays. When a nil array is encountered, the methods return early (lines 675, 707, 755, 816) without writing any value, leaving a dangling separator (' ' or ',') in the buffer. This produces malformed ILP lines.This is the same issue that was fixed for decimal columns in commit a7f4c07.
Apply this diff to check for nil before writing separators:
func (b *buffer) Float64Array1DColumn(name string, values []float64) *buffer { + if values == nil { + // Don't write null arrays + return b + } if !b.prepareForField() { return b } - if values == nil { - // Don't write null arrays - return b - } b.lastErr = b.writeColumnName(name) if b.lastErr != nil { return b } dim1 := len(values) if dim1 > MaxArrayElements { b.lastErr = fmt.Errorf("array size %d exceeds maximum limit %d", dim1, MaxArrayElements) return b } b.writeFloat64ArrayHeader(1) // Write shape b.writeInt32(int32(dim1)) // Write values if len(values) > 0 { b.writeFloat64Data(values) } b.hasFields = true return b } func (b *buffer) Float64Array2DColumn(name string, values [][]float64) *buffer { + if values == nil { + // Don't write null arrays + return b + } if !b.prepareForField() { return b } - if values == nil { - // Don't write null arrays - return b - } b.lastErr = b.writeColumnName(name) if b.lastErr != nil { return b } // Validate array shape dim1 := len(values) var dim2 int if dim1 > 0 { dim2 = len(values[0]) totalElements := product([]uint{uint(dim1), uint(dim2)}) if totalElements > MaxArrayElements { b.lastErr = fmt.Errorf("array size %d exceeds maximum limit %d", totalElements, MaxArrayElements) return b } for i, row := range values { if len(row) != dim2 { b.lastErr = fmt.Errorf("irregular 2D array shape: row %d has length %d, expected %d", i, len(row), dim2) return b } } } b.writeFloat64ArrayHeader(2) // Write shape b.writeInt32(int32(dim1)) b.writeInt32(int32(dim2)) // Write values for _, row := range values { if len(row) > 0 { b.writeFloat64Data(row) } } b.hasFields = true return b } func (b *buffer) Float64Array3DColumn(name string, values [][][]float64) *buffer { + if values == nil { + // Don't write null arrays + return b + } if !b.prepareForField() { return b } - if values == nil { - // Don't write null arrays - return b - } b.lastErr = b.writeColumnName(name) if b.lastErr != nil { return b } // Validate array shape dim1 := len(values) var dim2, dim3 int if dim1 > 0 { dim2 = len(values[0]) if dim2 > 0 { dim3 = len(values[0][0]) } totalElements := product([]uint{uint(dim1), uint(dim2), uint(dim3)}) if totalElements > MaxArrayElements { b.lastErr = fmt.Errorf("array size %d exceeds maximum limit %d", totalElements, MaxArrayElements) return b } for i, level1 := range values { if len(level1) != dim2 { b.lastErr = fmt.Errorf("irregular 3D array shape: level1[%d] has length %d, expected %d", i, len(level1), dim2) return b } for j, level2 := range level1 { if len(level2) != dim3 { b.lastErr = fmt.Errorf("irregular 3D array shape: level2[%d][%d] has length %d, expected %d", i, j, len(level2), dim3) return b } } } } b.writeFloat64ArrayHeader(3) // Write shape b.writeInt32(int32(dim1)) b.writeInt32(int32(dim2)) b.writeInt32(int32(dim3)) // Write values for _, level1 := range values { for _, level2 := range level1 { if len(level2) > 0 { b.writeFloat64Data(level2) } } } b.hasFields = true return b } func (b *buffer) Float64ArrayNDColumn(name string, value *NdArray[float64]) *buffer { + if value == nil { + // Don't write null arrays + return b + } if !b.prepareForField() { return b } - if value == nil { - // Don't write null arrays - return b - } b.lastErr = b.writeColumnName(name) if b.lastErr != nil { return b } shape := value.Shape() numDims := value.NDims() // Write nDims b.writeFloat64ArrayHeader(byte(numDims)) // Write shape for _, dim := range shape { b.writeInt32(int32(dim)) } // Write data data := value.Data() if len(data) > 0 { b.writeFloat64Data(data) } b.hasFields = true return b }Also applies to: 701-747, 749-808, 810-841
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
buffer.go(6 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
buffer.go (1)
decimal.go (2)
Decimal(39-43)ShopspringDecimal(45-48)
⏰ 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 (4)
buffer.go (4)
575-603: LGTM! Decimal encoding correctly handles null values.The null check at line 576 before
prepareForFieldensures that null decimals don't leave stray separators in the buffer. The internaldecimalColumnhelper cleanly encodes the binary format with scale, offset, and unscaled bytes.
605-622: LGTM! String decimal validation is appropriately positioned.The text validation at line 609 occurs before writing the column name, ensuring malformed input is caught early. The text format encoding with the 'd' suffix is clean and consistent.
624-644: LGTM! Shopspring decimal correctly avoids stray separators.The nil checks at lines 625 and 636 both occur before
prepareForField, ensuring that null values don't leave dangling separators. This correctly addresses the issue flagged in previous reviews.
597-597: ConstantdecimalBinaryTypeCodeis properly defined and correctly used.The constant is defined in
decimal.go:34asdecimalBinaryTypeCode byte = 0x17and is correctly referenced inbuffer.go:597. No issues found.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
integration_test.go (3)
503-548: Decimal integration test is good; consider making Shopspringnilbehavior explicitThis test nicely exercises:
- text-based decimals (
DecimalColumnFromString),- binary decimals via
NewDecimalFromInt64andNewDecimal(positive and negative),- and type normalization to
DECIMAL(18,3)(hence the123.450/123.460strings).One thing that’s implicit is
DecimalColumnShopspring("binary_null_col", nil): currently the expectations don’t includebinary_null_col, so the test only asserts “calling this doesn’t error,” not whether the column is created (with nulls) or omitted entirely.If you care about that contract long‑term, consider either:
- adding
binary_null_coltoColumnsand assertingnilvalues inDataset, or- adding a short comment that we intentionally don’t assert anything about this column here (pure smoke test), with more focused coverage elsewhere.
If you tighten the expectations, please double‑check how
DecimalColumnShopspringcurrently treatsnil(no column vs. nulls) and align the test with that behavior.
552-587: Protocol version loop and sender construction cleanly extended to v3Extending the
pVersionloop to{0, 1, 2, 3}and wiring both TCP/HTTP switches to callWithProtocolVersionfor 1–3 while leaving0as “default/negotiated” keeps the matrix explicit and matches the new v3 support. This should work well with the laterProtocolVersion(sender)‑based gating.If you ever need to debug version‑specific failures, consider including
pVersionin the subtest name (e.g.fmt.Sprintf("%s: %s/v%d", tc.name, protocol, pVersion)), so it’s immediately visible which protocol version failed.
34-34: Runtime protocol-version gating for arrays/decimals looks correctImporting
stringsand switching tosenderVersion := qdb.ProtocolVersion(sender)to gate tests is a solid improvement:
senderVersionreflects the actual negotiated sender (especially for HTTP v0), avoiding hardcoded guesses.senderVersion < 2 && tc.name == "double array"correctly skips array tests on pre‑v2 senders.senderVersion < 3 && strings.Contains(tc.name, "decimal")ensures decimal tests only run when ILP v3 is in play.- Both early‑return paths cleanly close the sender and stop the container, avoiding leaks.
The name/substring checks are a bit ad‑hoc but fine for the current set of tests.
If this grows, you might consider adding a
minProtocolVersionfield to the test case struct instead of keying offtc.name/strings.Contains, to make the requirements more explicit and less dependent on naming.Also applies to: 592-601
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
integration_test.go(8 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
integration_test.go (3)
sender.go (9)
LineSender(55-244)NewLineSender(604-633)WithTcp(326-330)WithAddress(443-447)WithProtocolVersion(515-519)ProtocolVersion1(282-282)ProtocolVersion2(283-283)ProtocolVersion3(284-284)WithHttp(319-323)decimal.go (2)
NewDecimalFromInt64(90-101)NewDecimal(72-87)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 (2)
integration_test.go (2)
137-155: QuestDB image bump to 9.2.0 for integration testsUpdating the test container image to
questdb/questdb:9.2.0is consistent with needing ILP v3/DECIMAL support and fits the rest of the PR; no issues from the test-harness perspective.If you haven’t already, please confirm locally that this tag is available in your CI environment and still exposes
/settingswith the expected ILP configuration.
424-501: Adding LONGicolumn to array test keeps expectations alignedThe extra
Int64Column("i", …)and corresponding{"i", "LONG"}column plus the updated dataset (usingfloat64(1/2/3)for the scanned LONGs) are internally consistent and make the three array rows easy to distinguish. The NaN →niland empty/nil array expectations still match the existing behavior.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
integration_test.go (1)
436-436: Consider the necessity of the addedicolumn.The addition of an
Int64Column("i", ...)to the "double array" test case appears unrelated to decimal support. While not harmful and potentially useful for row identification, it might be worth clarifying if this change is necessary or if it should be in a separate PR for test improvements.Also applies to: 449-449, 461-461, 470-470, 479-479, 486-486, 493-493
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
integration_test.go(16 hunks)tcp_integration_test.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
integration_test.go (3)
sender.go (9)
LineSender(55-244)NewLineSender(604-633)WithTcp(326-330)WithAddress(443-447)WithProtocolVersion(515-519)ProtocolVersion1(282-282)ProtocolVersion2(283-283)ProtocolVersion3(284-284)WithHttp(319-323)decimal.go (2)
NewDecimalFromInt64(90-101)NewDecimal(72-87)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 (9)
tcp_integration_test.go (1)
511-526: LGTM! Clean helper for DDL execution.The
ddlfunction follows the same pattern as the existingdropTableandqueryTableDatahelpers, appropriately executing DDL statements via the QuestDB HTTP/execendpoint.integration_test.go (8)
34-34: LGTM! Import is used for decimal test skip logic.The
stringsimport is correctly used on line 606 to check test names for decimal-related tests.
59-59: LGTM! Signature change enables DDL setup for decimal tests.The updated signature allows writer functions to execute DDL statements (via the HTTP address) and use test assertions, which is necessary for the new decimal test case that requires pre-created schema.
143-143: LGTM! Container version supports protocol v3 and decimals.The QuestDB 9.2.0 image provides the necessary support for protocol version 3 and decimal column types being tested in this PR.
274-274: LGTM! All writer functions updated consistently.All writer functions have been updated to match the new
writerFnsignature, maintaining consistency across test cases.Also applies to: 323-323, 345-345, 365-365, 385-385, 406-406
562-562: LGTM! Protocol version 3 testing added.The loop now includes protocol version 3, enabling tests for decimal support which requires this version.
574-595: LGTM! Protocol version 3 support added for both TCP and HTTP.Both protocol switch statements correctly handle version 3, enabling comprehensive testing across all supported versions.
600-610: LGTM! Skip logic correctly uses negotiated protocol version.The implementation correctly:
- Queries the actual negotiated protocol version from the sender rather than making assumptions
- Skips feature tests based on version requirements (arrays need v2+, decimals need v3+)
- Addresses previous review feedback about not hardcoding version assumptions
612-612: LGTM! Writer function call updated with new parameters.The call correctly passes the test context and HTTP address to match the updated
writerFnsignature.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
integration_test.go (1)
503-557: Fix decimal test expected Columns to includebinary_null_col(mismatch with DDL and Dataset)The DDL creates 5 columns:
text_col decimal(18,3)binary_col decimal(18,3)binary_neg_col decimal(18,3)binary_null_col decimal(18,3)ts timestamp(designated)The writer sends values for all four decimal columns (with
DecimalColumnShopspring("binary_null_col", nil)for NULLs), and eachDatasetrow has 5 entries (the fourth beingnil).However,
expected.Columnsonly lists the first three decimal columns plusts, so you have 4 columns but 5 values per dataset row, and the column list does not containbinary_null_col. This will causereflect.DeepEqualto fail even when the server behavior is correct and also matches a past review comment that flagged the missing column.Update the expected table metadata to include
binary_null_colso it aligns with the DDL and Dataset:- tableData{ - Columns: []column{ - {"text_col", "DECIMAL(18,3)"}, - {"binary_col", "DECIMAL(18,3)"}, - {"binary_neg_col", "DECIMAL(18,3)"}, - {"ts", "TIMESTAMP"}, - }, - Dataset: [][]any{ - {"123.450", "123.450", "-123.450", nil, "1970-01-01T00:00:00.000001Z"}, - {"123.460", "123.460", "-123.460", nil, "1970-01-01T00:00:00.000002Z"}, - }, - Count: 2, - }, + tableData{ + Columns: []column{ + {"text_col", "DECIMAL(18,3)"}, + {"binary_col", "DECIMAL(18,3)"}, + {"binary_neg_col", "DECIMAL(18,3)"}, + {"binary_null_col", "DECIMAL(18,3)"}, + {"ts", "TIMESTAMP"}, + }, + Dataset: [][]any{ + {"123.450", "123.450", "-123.450", nil, "1970-01-01T00:00:00.000001Z"}, + {"123.460", "123.460", "-123.460", nil, "1970-01-01T00:00:00.000002Z"}, + }, + Count: 2, + },This will make the expected schema/data consistent with how the table is created and populated.
🧹 Nitpick comments (2)
integration_test.go (2)
424-501: Double array test + protocol gating look correct; name-based gating is acceptable but could be structuredThe “double array” test now:
- Adds an
i LONGcolumn and includes it in bothColumnsandDataset, which keeps row ordering/data shape consistent.- Exercises 1D/2D/3D/ND arrays, including NaN →
nilmapping, with expected nested structures matching the writes.- Skips cleanly when
senderVersion < 2, so v1 senders don’t attempt unsupported array types, and resources are closed before returning.All of that looks solid.
If you want to reduce reliance on
tc.name == "double array"in the future, you could encode aminProtocolVersionfield in the test case struct and gate features off that rather than the test name, but this is optional and the current approach is workable given the small number of gated tests.Also applies to: 600-605
560-627: Protocol version matrix + negotiated version gating looks good; consider including version in subtest nameThe new triple loop over
protocol×pVersion×testCasetogether with:
- Explicit
WithProtocolVersion(...)wiring for pVersion 1–3,- The default pVersion 0 path deferring to the sender’s own negotiation, and
senderVersion := qdb.ProtocolVersion(sender)used to gate v2+ (“double array”) and v3+ (“decimal”) tests,correctly bases feature skips on the actual negotiated/constructed sender type instead of guessed values, addressing earlier concerns about misclassification.
One small usability tweak: the subtest name is currently
fmt.Sprintf("%s: %s", tc.name, protocol), so runs for differentpVersionvalues are indistinguishable in output. If you want clearer diagnostics when a specific protocol version fails, you could optionally includepVersionin the name, e.g.:suite.T().Run(fmt.Sprintf("%s: %s/v%d", tc.name, protocol, pVersion), func(t *testing.T) { // ... })Not required for correctness, but it makes tracking failures across the version matrix easier.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
integration_test.go(16 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
integration_test.go (3)
sender.go (9)
LineSender(55-244)NewLineSender(604-633)WithTcp(326-330)WithAddress(443-447)WithProtocolVersion(515-519)ProtocolVersion1(282-282)ProtocolVersion2(283-283)ProtocolVersion3(284-284)WithHttp(319-323)decimal.go (2)
NewDecimalFromInt64(90-101)NewDecimal(72-87)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.20.x
- GitHub Check: Build with Go 1.19.x
🔇 Additional comments (2)
integration_test.go (2)
27-60: Top-level wiring for decimal tests looks consistentThe added
stringsimport and the newwriterFnsignature carrying*testing.TandhttpAddressare coherent with how the test cases are wired later (especially the decimal DDL). Call sites consistently passtandquestdbC.httpAddress, and tests that don’t needhttpAddresssimply ignore it, which is fine for this suite.
137-255: Verify QuestDB image tag aligns with decimal + ILP v3 supportThe container image is now hard‑coded to
questdb/questdb:9.2.0. Since this test suite is the contract for decimal and ILP v3 behavior, please double‑check that this tag:
- Exists in the registry used by CI, and
- Is the intended minimal version that includes the decimal feature set exercised here.
If you expect to bump this again when server-side decimal changes, consider pulling the tag from a central constant or env to avoid scattering the version string.
This PR adds decimal support.
This is a tandem pr for:
Usage
Progress
Summary by CodeRabbit
New Features
Bug Fixes / Behavior
Documentation
Tests