feat: support decimal #123
Conversation
WalkthroughAdds ILP DECIMAL datatype support and Protocol Version V3 across Rust, FFI/C, C++, Python, tests, and examples. Introduces DecimalView (text/binary), serialization/validation, new decimal APIs (Rust/C/C++), feature-gated conversions for rust_decimal/bigdecimal, protocol negotiation updates, and extensive tests/examples. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant App
participant Buffer
participant DecimalView
participant Serializer
participant Network
App->>Buffer: column_dec(name, value)
Buffer->>Buffer: ensure protocol >= V3
alt value is text
Buffer->>DecimalView: try_new_string(value)
else value is scaled binary
Buffer->>DecimalView: try_new_scaled(scale, bytes)
end
DecimalView-->>Buffer: DecimalView(String|Scaled)
Buffer->>Serializer: serialize(decimal)
alt String path
Serializer-->>Buffer: append text + 'd'
else Scaled path
Serializer-->>Buffer: append '=' + type + scale + len + mantissa
end
Buffer->>Network: send ILP message
Network-->>App: success or error (invalid_decimal on validation failure)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧬 Code graph analysis (1)questdb-rs/src/ingress/decimal.rs (2)
⏰ 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). (9)
🔇 Additional comments (5)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
questdb-rs/src/ingress/buffer.rs (1)
978-1063: column_decimal: enable binary for future versions and update docs
- Gate binary like other binary-capable types: treat any non‑V1 as binary (future-proof, consistent with column_f64/column_arr).
- Add column_decimal to the method table in the docs.
Apply this minimal change for gating:
- value.serialize( - &mut self.output, - self.protocol_version == ProtocolVersion::V2, - )?; + value.serialize( + &mut self.output, + self.protocol_version != ProtocolVersion::V1, + )?;Doc table addition (outside this hunk): add a row
- | column_decimal | DECIMAL |
to the “Buffer method calls, Serialized ILP types and QuestDB types” table.questdb-rs/src/ingress/decimal.rs (1)
180-234: bigdecimal binary path looks correct; consider aligning length strategyImplementation uses minimal two’s complement via to_signed_bytes_be with scale checks. For consistency, consider emitting minimal-length mantissa for rust_decimal too (not a blocker).
questdb-rs/src/tests/decimal.rs (1)
602-611: Avoid hard-coded offset when slicing the binary decimal payloadUsing a fixed 17-byte offset assumes specific table/symbol names. Compute the start dynamically to make tests robust.
Example change:
- // Skip the prefix and \n suffix - let dec_binary = &bytes[17..bytes.len() - 1]; + // Find " dec=" prefix and slice from there (exclude trailing '\n') + let prefix = b" dec="; + let start = bytes + .windows(prefix.len()) + .position(|w| w == prefix) + .expect("missing ' dec=' prefix") + + prefix.len(); + let dec_binary = &bytes[start..bytes.len() - 1];Apply similarly in the bigdecimal v2 test.
Also applies to: 685-691
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
questdb-rs-ffi/src/lib.rs(2 hunks)questdb-rs/Cargo.toml(5 hunks)questdb-rs/examples/basic.rs(1 hunks)questdb-rs/examples/http.rs(1 hunks)questdb-rs/examples/protocol_version.rs(1 hunks)questdb-rs/src/error.rs(1 hunks)questdb-rs/src/ingress/buffer.rs(3 hunks)questdb-rs/src/ingress/decimal.rs(1 hunks)questdb-rs/src/ingress/mod.rs(2 hunks)questdb-rs/src/tests/decimal.rs(1 hunks)questdb-rs/src/tests/mod.rs(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (6)
questdb-rs/examples/http.rs (3)
questdb-rs/examples/basic.rs (1)
main(8-34)questdb-rs/examples/protocol_version.rs (1)
main(10-41)questdb-rs/src/ingress/mod.rs (1)
from_conf(390-544)
questdb-rs/examples/protocol_version.rs (3)
questdb-rs/examples/basic.rs (1)
main(8-34)questdb-rs/examples/http.rs (1)
main(10-27)questdb-rs/src/ingress/mod.rs (1)
from_conf(390-544)
questdb-rs/src/tests/decimal.rs (2)
questdb-rs/src/ingress/buffer.rs (7)
new(97-102)new(181-188)new(204-263)new(303-350)new(486-488)len(523-525)with_max_name_len(500-508)questdb-rs/src/error.rs (3)
new(95-100)code(128-130)msg(133-135)
questdb-rs/src/ingress/buffer.rs (1)
system_test/questdb_line_sender.py (1)
ProtocolVersion(101-120)
questdb-rs/examples/basic.rs (2)
questdb-rs/src/ingress/mod.rs (1)
from_conf(390-544)questdb-rs/src/ingress/timestamp.rs (2)
from_datetime(130-132)from_datetime(211-220)
questdb-rs/src/ingress/decimal.rs (1)
questdb-rs/src/ingress/buffer.rs (1)
must_escape_unquoted(75-77)
⏰ 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). (9)
- GitHub Check: questdb.c-questdb-client (Building and testing on windows-msvc-2019)
- GitHub Check: questdb.c-questdb-client (Building and testing on windows-msvc-2022)
- GitHub Check: questdb.c-questdb-client (Building and testing on mac)
- GitHub Check: questdb.c-questdb-client (Building and testing on linux-nightly)
- GitHub Check: questdb.c-questdb-client (Building and testing on linux-beta)
- GitHub Check: questdb.c-questdb-client (Building and testing on linux-stable)
- GitHub Check: questdb.c-questdb-client (Building and testing on linux)
- GitHub Check: questdb.c-questdb-client (Building and testing cargo fmt and clippy)
- GitHub Check: questdb.c-questdb-client (Building and testing Vs QuestDB 'master')
🔇 Additional comments (14)
questdb-rs/Cargo.toml (2)
49-50: LGTM: Decimal dependency versions are current and properly configured.The versions align with the latest stable releases documented in the learnings. Both crates are correctly marked as optional dependencies to support feature-gated decimal serialization.
Based on learnings.
108-112: LGTM: Feature declarations follow best practices.The feature flags are properly structured to expose the optional decimal dependencies, allowing users to opt-in based on their precision requirements (rust_decimal for fixed ~28-digit precision, bigdecimal for arbitrary precision).
Based on learnings.
questdb-rs/src/tests/mod.rs (1)
33-33: LGTM: Test module integration is correct.The decimal test module declaration follows the existing pattern and properly extends the test suite to cover the new decimal serialization functionality.
questdb-rs-ffi/src/lib.rs (1)
228-229: LGTM: FFI error code integration follows established patterns.The new
line_sender_error_invalid_decimalvariant and its mapping at lines 258-260 are consistent with the existing error handling architecture, ensuring proper error propagation across the FFI boundary.questdb-rs/examples/basic.rs (1)
11-11: LGTM: Example correctly demonstrates decimal serialization.The protocol version 2 configuration enables the decimal serialization pathway, and the string literal approach (
"2615.54") demonstrates the simplest usage pattern requiring no additional dependencies. This complements the other examples that showcase the rust_decimal and bigdecimal integrations.Also applies to: 19-19
questdb-rs/examples/protocol_version.rs (2)
1-3: LGTM: BigDecimal integration demonstrates arbitrary-precision decimal support.The example properly showcases
bigdecimalusage for high-precision decimal values, parsing from string and using the same value across both protocol versions. This complements therust_decimalexample in http.rs by demonstrating the alternative arbitrary-precision approach.Based on learnings.
Also applies to: 11-11, 21-21, 35-35
30-30: LGTM: Corrected buffer creation.The change from
sender.new_buffer()tosender2.new_buffer()ensures the buffer uses the correct sender's protocol settings, preventing potential configuration mismatches between the two senders.questdb-rs/src/error.rs (1)
82-83: LGTM: Error code addition is well-integrated.The
InvalidDecimalvariant follows the established pattern and is properly positioned in the error code enumeration. The clear documentation ensures users understand when this error occurs during decimal serialization.questdb-rs/examples/http.rs (1)
1-2: LGTM: Example properly demonstrates rust_decimal integration.The example correctly showcases
rust_decimal::Decimalusage with protocol version 2, demonstrating the fixed-precision decimal approach (~28 significant digits). This is appropriate for financial use cases as noted in the learnings.Based on learnings.
Also applies to: 8-8, 15-15, 20-20
questdb-rs/src/ingress/mod.rs (1)
65-66: LGTM: Decimal module integration is clean and minimal.The decimal module is properly integrated into the ingress API surface, exposing
DecimalSerializeras a public trait. TheDECIMAL_BINARY_FORMAT_TYPEconstant (value 23) follows the pattern of existing format type constants and coordinates with the server-side implementation for protocol version 2 binary serialization.Also applies to: 77-77
questdb-rs/src/ingress/buffer.rs (1)
75-77: Visibility change looks goodMaking must_escape_unquoted pub(crate) enables reuse from decimal.rs without widening the public API.
questdb-rs/src/ingress/decimal.rs (2)
103-126: &str serializer is correct and leanValidation against ILP-reserved bytes and appending the 'd' suffix is appropriate; text path ignores the binary flag as intended.
25-26: Import path is correct must_escape_unquoted is re-exported viapub use buffer::*;insrc/ingress/mod.rs, so the existing import indecimal.rscompiles as-is.Likely an incorrect or invalid review comment.
questdb-rs/src/tests/decimal.rs (1)
43-101: Test coverage is thoroughCovers text/binary paths, error cases, and buffer integration across features. Nice.
Also applies to: 112-137, 139-181, 187-326, 332-534, 540-567, 615-630, 632-647, 648-665, 667-692
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
questdb-rs/src/ingress/decimal.rs (1)
167-170: Fix the misleading comment (duplicate of past review).The comment on line 167 says "We skip the upper 3 bytes" but line 170 says "Skip upper 4 bytes, write lower 12". As discussed in the previous review, the code correctly writes 13 bytes (via
[3..]), which includes one sign-extension byte needed for sign detection plus the 12-byte mantissa.Apply this diff to correct the comment:
- // We skip the upper 3 bytes (which are sign-extended) and write the lower 13 bytes + // Skip the upper 3 sign-extended bytes and write the lower 13 bytes (1 sign byte + 12 mantissa) let mantissa = self.mantissa(); out.push(13); - out.extend_from_slice(&mantissa.to_be_bytes()[3..]); // Skip upper 4 bytes, write lower 12 + out.extend_from_slice(&mantissa.to_be_bytes()[3..]);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
questdb-rs/src/ingress/decimal.rs(1 hunks)questdb-rs/src/ingress/mod.rs(3 hunks)questdb-rs/src/tests/decimal.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- questdb-rs/src/ingress/mod.rs
🧰 Additional context used
🧬 Code graph analysis (2)
questdb-rs/src/ingress/decimal.rs (1)
questdb-rs/src/ingress/buffer.rs (1)
must_escape_unquoted(75-77)
questdb-rs/src/tests/decimal.rs (2)
questdb-rs/src/error.rs (3)
new(95-100)code(128-130)msg(133-135)questdb-rs/src/ingress/buffer.rs (7)
new(97-102)new(181-188)new(204-263)new(303-350)new(486-488)len(523-525)with_max_name_len(500-508)
⏰ 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). (9)
- GitHub Check: questdb.c-questdb-client (Building and testing on windows-msvc-2019)
- GitHub Check: questdb.c-questdb-client (Building and testing on windows-msvc-2022)
- GitHub Check: questdb.c-questdb-client (Building and testing on mac)
- GitHub Check: questdb.c-questdb-client (Building and testing on linux-nightly)
- GitHub Check: questdb.c-questdb-client (Building and testing on linux-beta)
- GitHub Check: questdb.c-questdb-client (Building and testing on linux-stable)
- GitHub Check: questdb.c-questdb-client (Building and testing on linux)
- GitHub Check: questdb.c-questdb-client (Building and testing Vs QuestDB 'master')
- GitHub Check: questdb.c-questdb-client (Building and testing cargo fmt and clippy)
🔇 Additional comments (7)
questdb-rs/src/ingress/decimal.rs (3)
27-77: Excellent trait design and documentation.The
DecimalSerializertrait is well-defined with comprehensive documentation covering both text and binary formats, including concrete examples. The documented constraints (scale ≤ 76, two's complement big-endian encoding) align with the implementations below.
103-126: Clean &str implementation with appropriate validation scope.The implementation correctly validates ILP reserved characters while explicitly avoiding full decimal syntax validation, which is a reasonable trade-off documented in lines 88-93. The server-side validation will catch malformed decimals.
176-230: Robust BigDecimal handling with proper constraint enforcement.The implementation correctly:
- Validates scale ≤ 76 (QuestDB limit)
- Handles negative scales by upscaling to scale 0 (lines 203-211)
- Enforces mantissa size limit (i8::MAX bytes, line 213)
The negative scale handling is particularly well-done: converting
1.23e12(negative scale) to1230000000000with scale 0 ensures protocol compliance.questdb-rs/src/tests/decimal.rs (4)
29-37: Clean helper function for test readability.The
serialize_decimalwrapper simplifies test code by encapsulating the common pattern of creating a buffer and calling the trait method.
139-183: Well-designed binary format validator with correct sign extension.The
parse_binary_decimalhelper properly validates the binary format structure (marker, type, scale, length) and correctly handles sign extension for negative values (lines 174-178). This is essential for verifying two's complement big-endian encoding.
478-522: Excellent edge case coverage for BigDecimal limits.These tests thoroughly validate:
- Maximum scale of 76 (QuestDB limit)
- Rejection of scale > 76 with clear error message
- Negative scale normalization to scale 0
- Large value rejection beyond i8::MAX bytes
This ensures the implementation respects all documented constraints.
542-694: Comprehensive integration tests across protocol versions.The buffer integration tests validate the
column_decimalmethod across:
- Both V1 (text) and V2 (binary) protocols
- All decimal types (&str, rust_decimal, bigdecimal)
- Multiple decimals per buffer
- Name length enforcement
The V2 tests correctly verify binary encoding by extracting and parsing the decimal bytes (e.g., lines 609-614).
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
include/questdb/ingress/line_sender.h (1)
46-86: Add missingline_sender_error_invalid_decimalto the C API enum
Include aline_sender_error_invalid_decimalentry inline_sender_error_code(in include/questdb/ingress/line_sender.h) to match the FFI mapping for decimal validation errors.system_test/questdb_line_sender.py (2)
687-689: Update type hint to include Decimal.The type hint for the
valueparameter should includeDecimalto match the implementation below that handlesDecimalvalues.Apply this diff to fix the type hint:
def column( self, name: str, - value: Union[bool, int, float, str, TimestampMicros, datetime]): + value: Union[bool, int, float, str, Decimal, TimestampMicros, datetime]):
733-737: Update error message to include Decimal.The error message should list
Decimalas one of the expected types since it's now supported.Apply this diff:
fqn = _fully_qual_name(value) raise ValueError( f'Bad field value of type {fqn}: Expected one of ' - '`bool`, `int`, `float` or `str`.') + '`bool`, `int`, `float`, `str`, `Decimal`, `TimestampMicros`, or `datetime`.')
🧹 Nitpick comments (2)
system_test/test.py (1)
546-547: Document the default decimal scale.The comment mentions "By default, the decimal created as a scale of 3", but it's unclear where this default is configured or if it's server-side behavior. Consider adding a comment referencing the configuration or documenting this behavior.
doc/C.md (1)
93-96: Consider adding guidance on decimal vs float usage.The documentation now shows the decimal API but doesn't explain when developers should use
column_decimal_strversuscolumn_f64. Consider adding a brief note about use cases (e.g., financial data, precise decimal arithmetic) and the differences in server-side storage/precision.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
doc/C.md(1 hunks)doc/CPP.md(1 hunks)examples/line_sender_c_example.c(1 hunks)examples/line_sender_c_example_auth.c(1 hunks)examples/line_sender_c_example_auth_tls.c(1 hunks)examples/line_sender_c_example_from_conf.c(1 hunks)examples/line_sender_c_example_from_env.c(1 hunks)examples/line_sender_c_example_http.c(1 hunks)examples/line_sender_c_example_tls_ca.c(1 hunks)examples/line_sender_cpp_example.cpp(1 hunks)examples/line_sender_cpp_example_auth.cpp(1 hunks)examples/line_sender_cpp_example_auth_tls.cpp(1 hunks)examples/line_sender_cpp_example_from_conf.cpp(1 hunks)examples/line_sender_cpp_example_from_env.cpp(1 hunks)examples/line_sender_cpp_example_http.cpp(1 hunks)examples/line_sender_cpp_example_tls_ca.cpp(1 hunks)include/questdb/ingress/line_sender.h(1 hunks)include/questdb/ingress/line_sender.hpp(1 hunks)questdb-rs-ffi/src/lib.rs(3 hunks)system_test/questdb_line_sender.py(3 hunks)system_test/test.py(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (12)
include/questdb/ingress/line_sender.h (2)
questdb-rs-ffi/src/lib.rs (1)
line_sender_buffer_column_decimal_str(948-959)include/questdb/ingress/line_sender.hpp (22)
buffer(1685-1691)buffer(1685-1685)buffer(1702-1717)buffer(1702-1702)buffer(1740-1762)buffer(1740-1741)name(810-816)name(810-810)name(824-830)name(824-824)name(858-864)name(858-858)name(871-877)name(871-871)name(884-890)name(884-884)name(1005-1011)name(1005-1005)name(1019-1022)name(1019-1019)name(1024-1027)name(1024-1024)
examples/line_sender_c_example_from_conf.c (1)
questdb-rs-ffi/src/lib.rs (1)
line_sender_buffer_column_decimal_str(948-959)
examples/line_sender_c_example.c (1)
questdb-rs-ffi/src/lib.rs (1)
line_sender_buffer_column_decimal_str(948-959)
include/questdb/ingress/line_sender.hpp (2)
questdb-rs-ffi/src/lib.rs (1)
line_sender_buffer_column_decimal_str(948-959)questdb-rs/src/ingress/buffer.rs (1)
column_decimal(1050-1062)
examples/line_sender_c_example_from_env.c (1)
questdb-rs-ffi/src/lib.rs (1)
line_sender_buffer_column_decimal_str(948-959)
questdb-rs-ffi/src/lib.rs (2)
include/questdb/ingress/line_sender.hpp (30)
buffer(1685-1691)buffer(1685-1685)buffer(1702-1717)buffer(1702-1702)buffer(1740-1762)buffer(1740-1741)line_sender_buffer(634-643)line_sender_buffer(634-637)line_sender_buffer(645-652)line_sender_buffer(654-662)line_sender_buffer(1180-1184)name(810-816)name(810-810)name(824-830)name(824-824)name(858-864)name(858-858)name(871-877)name(871-871)name(884-890)name(884-884)name(1005-1011)name(1005-1005)name(1019-1022)name(1019-1019)name(1024-1027)name(1024-1024)line_sender_error(142-146)column_decimal(1046-1050)column_decimal(1046-1047)questdb-rs/src/ingress/buffer.rs (1)
column_decimal(1050-1062)
examples/line_sender_c_example_http.c (1)
questdb-rs-ffi/src/lib.rs (1)
line_sender_buffer_column_decimal_str(948-959)
examples/line_sender_c_example_tls_ca.c (1)
questdb-rs-ffi/src/lib.rs (1)
line_sender_buffer_column_decimal_str(948-959)
examples/line_sender_c_example_auth.c (1)
questdb-rs-ffi/src/lib.rs (1)
line_sender_buffer_column_decimal_str(948-959)
examples/line_sender_c_example_auth_tls.c (1)
questdb-rs-ffi/src/lib.rs (1)
line_sender_buffer_column_decimal_str(948-959)
system_test/questdb_line_sender.py (2)
questdb-rs-ffi/src/lib.rs (1)
line_sender_buffer_column_decimal_str(948-959)include/questdb/ingress/line_sender.hpp (34)
_impl(222-225)_impl(227-230)_impl(232-235)_impl(708-714)_impl(717-723)_impl(726-732)_impl(739-745)_impl(755-763)_impl(790-794)_impl(800-804)_impl(1167-1178)_impl(1187-1197)_impl(1546-1553)_impl(1772-1775)_impl(1780-1787)_impl(1795-1800)name(810-816)name(810-810)name(824-830)name(824-824)name(858-864)name(858-858)name(871-877)name(871-871)name(884-890)name(884-884)name(1005-1011)name(1005-1005)name(1019-1022)name(1019-1019)name(1024-1027)name(1024-1024)_utf8(261-264)_utf8(261-261)
system_test/test.py (2)
system_test/questdb_line_sender.py (8)
table(671-677)table(909-911)column(687-738)column(917-921)at_now(793-796)at_now(934-935)buffer(862-863)peek(648-659)system_test/fixture.py (1)
retry_check_table(277-315)
⏰ 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). (9)
- GitHub Check: questdb.c-questdb-client (Building and testing on windows-msvc-2019)
- GitHub Check: questdb.c-questdb-client (Building and testing on windows-msvc-2022)
- GitHub Check: questdb.c-questdb-client (Building and testing on mac)
- GitHub Check: questdb.c-questdb-client (Building and testing on linux-nightly)
- GitHub Check: questdb.c-questdb-client (Building and testing on linux-beta)
- GitHub Check: questdb.c-questdb-client (Building and testing on linux-stable)
- GitHub Check: questdb.c-questdb-client (Building and testing cargo fmt and clippy)
- GitHub Check: questdb.c-questdb-client (Building and testing on linux)
- GitHub Check: questdb.c-questdb-client (Building and testing Vs QuestDB 'master')
🔇 Additional comments (24)
examples/line_sender_cpp_example_from_env.cpp (1)
26-26: LGTM! Consistent with the new decimal API.The change correctly demonstrates the new
column_decimalAPI for inserting decimal values as strings.examples/line_sender_cpp_example.cpp (1)
28-28: LGTM! Correct decimal API usage.The change properly uses
column_decimalwith a UTF-8 string literal for decimal values.examples/line_sender_cpp_example_auth_tls.cpp (1)
32-32: LGTM! Proper decimal API usage.Correctly demonstrates the decimal string API with authentication and TLS.
examples/line_sender_c_example_from_env.c (1)
40-42: LGTM! Correct C API usage for decimals.The change properly uses the C API
line_sender_buffer_column_decimal_strwith a UTF-8 literal and maintains the existing error handling pattern.examples/line_sender_c_example.c (1)
54-56: LGTM! Consistent C API usage.Correctly demonstrates the decimal string API in C with proper error handling.
doc/CPP.md (1)
79-79: LGTM! Documentation correctly updated.The code example in the documentation properly demonstrates the new
column_decimalAPI.examples/line_sender_cpp_example_auth.cpp (1)
32-32: LGTM! Proper usage with authentication.Correctly uses the decimal API in an authenticated context.
examples/line_sender_c_example_auth_tls.c (1)
64-66: LGTM! C API usage with authentication and TLS is correct.The decimal string API is properly used with authentication and TLS configuration.
Note on PR status: The PR description indicates "c: not done, c++: not done" in the progress checklist, yet these C/C++ examples are updated. Based on the FFI bindings and these examples, the C/C++ API surface appears functional. Consider updating the checklist or clarifying what remains incomplete for C/C++ support (e.g., comprehensive testing, additional API surface, documentation).
examples/line_sender_c_example_from_conf.c (1)
41-44: LGTM! Decimal string API usage is correct.The transition from
line_sender_buffer_column_f64toline_sender_buffer_column_decimal_stris implemented correctly, usingQDB_UTF8_LITERALfor the decimal value and maintaining consistent error handling.examples/line_sender_c_example_auth.c (1)
63-66: LGTM! Consistent decimal API usage.The change correctly replaces the floating-point column with the decimal string API, maintaining proper error handling and UTF-8 encoding.
examples/line_sender_c_example_tls_ca.c (1)
67-70: LGTM! Decimal API correctly applied.The implementation follows the same correct pattern as the other C examples, properly using the decimal string API.
examples/line_sender_cpp_example_from_conf.cpp (1)
27-27: LGTM! C++ decimal API usage is correct.The change from
.column()to.column_decimal()with the_utf8literal is properly implemented, maintaining the fluent interface pattern.examples/line_sender_c_example_http.c (1)
52-55: LGTM! HTTP example correctly updated.The decimal string API is properly applied, consistent with the other C examples.
examples/line_sender_cpp_example_http.cpp (1)
27-27: LGTM! C++ decimal API correctly applied.The change is consistent with the other C++ examples, properly using
.column_decimal()with the UTF-8 literal.examples/line_sender_cpp_example_tls_ca.cpp (1)
35-35: LGTM! TLS example correctly updated.The decimal API is properly applied, maintaining consistency with the other C++ examples.
include/questdb/ingress/line_sender.h (1)
499-513: LGTM! API function declaration is correct and well-documented.The new
line_sender_buffer_column_decimal_strfunction is properly declared with clear documentation, matching the signature used in the C examples and the FFI implementation.system_test/questdb_line_sender.py (2)
294-300: LGTM!The DLL binding registration for the new decimal string API is correct and follows the established pattern.
714-719: LGTM!The Decimal handling correctly converts the value to string and calls the appropriate C function.
system_test/test.py (2)
787-794: LGTM!The test expectations correctly reflect the change from DOUBLE to DECIMAL(18,3) type with string-formatted values.
520-549: Verify NaN/Infinity mapping to None
The test asserts Decimal("NaN") and Decimal("Infinity") are represented as null; confirm this is the intended, consistently implemented server behavior and documented in the API/specs.questdb-rs-ffi/src/lib.rs (3)
228-229: LGTM!The new error code
line_sender_error_invalid_decimalis properly added to the enum.
258-258: LGTM!The error code mapping from
ErrorCode::InvalidDecimaltoline_sender_error_invalid_decimalis correctly implemented.
941-959: LGTM!The C API function implementation correctly:
- Unwraps the buffer, column name, and UTF-8 value
- Delegates to the Rust
buffer.column_decimalmethod- Handles errors using the established
bubble_err_to_c!macro- Returns bool for success/failure
include/questdb/ingress/line_sender.hpp (1)
1029-1063: LGTM!The four
column_decimaloverloads provide convenient C++ API access:
- Base implementation with
utf8_view- Template for character array literals
std::string_viewoverloadconst std::string&overloadAll properly delegate to the underlying C API via
line_sender_buffer_column_decimal_strand follow the established pattern used by other column methods.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
system_test/questdb_line_sender.py (1)
687-689: Update the type hint to includeDecimal.The type hint for the
valueparameter doesn't includeDecimal, but the implementation (lines 714-719) handles this type. This inconsistency can cause type checkers to incorrectly flag valid code.Apply this diff to fix the type hint:
def column( self, name: str, - value: Union[bool, int, float, str, TimestampMicros, datetime]): + value: Union[bool, int, float, str, Decimal, TimestampMicros, datetime]):
🧹 Nitpick comments (1)
system_test/questdb_line_sender.py (1)
733-737: Consider usingTypeErrorfor invalid types.The error message correctly lists all supported types including
Decimal. However, for invalid type errors,TypeErroris more semantically appropriate thanValueError.Based on learnings
Apply this diff for a more appropriate exception type:else: fqn = _fully_qual_name(value) - raise ValueError( + raise TypeError( f'Bad field value of type {fqn}: Expected one of ' '`bool`, `int`, `float`, `str`, `Decimal`, `TimestampMicros`, or `datetime`.')
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
include/questdb/ingress/line_sender.h(2 hunks)include/questdb/ingress/line_sender.hpp(2 hunks)system_test/questdb_line_sender.py(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- include/questdb/ingress/line_sender.h
🧰 Additional context used
🧬 Code graph analysis (2)
system_test/questdb_line_sender.py (2)
questdb-rs-ffi/src/lib.rs (1)
line_sender_buffer_column_decimal_str(948-959)include/questdb/ingress/line_sender.hpp (34)
_impl(231-234)_impl(236-239)_impl(241-244)_impl(717-723)_impl(726-732)_impl(735-741)_impl(748-754)_impl(764-772)_impl(799-803)_impl(809-813)_impl(1176-1187)_impl(1196-1206)_impl(1555-1562)_impl(1781-1784)_impl(1789-1796)_impl(1804-1809)name(819-825)name(819-819)name(833-839)name(833-833)name(867-873)name(867-867)name(880-886)name(880-880)name(893-899)name(893-893)name(1014-1020)name(1014-1014)name(1028-1031)name(1028-1028)name(1033-1036)name(1033-1033)_utf8(270-273)_utf8(270-270)
include/questdb/ingress/line_sender.hpp (2)
questdb-rs-ffi/src/lib.rs (1)
line_sender_buffer_column_decimal_str(948-959)questdb-rs/src/ingress/buffer.rs (1)
column_decimal(1050-1062)
🪛 Ruff (0.13.3)
system_test/questdb_line_sender.py
735-737: Prefer TypeError exception for invalid type
(TRY004)
735-737: Avoid specifying long messages outside the exception class
(TRY003)
⏰ 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). (9)
- GitHub Check: questdb.c-questdb-client (Building and testing on windows-msvc-2019)
- GitHub Check: questdb.c-questdb-client (Building and testing on windows-msvc-2022)
- GitHub Check: questdb.c-questdb-client (Building and testing on mac)
- GitHub Check: questdb.c-questdb-client (Building and testing on linux-nightly)
- GitHub Check: questdb.c-questdb-client (Building and testing on linux-beta)
- GitHub Check: questdb.c-questdb-client (Building and testing on linux-stable)
- GitHub Check: questdb.c-questdb-client (Building and testing on linux)
- GitHub Check: questdb.c-questdb-client (Building and testing Vs QuestDB 'master')
- GitHub Check: questdb.c-questdb-client (Building and testing cargo fmt and clippy)
🔇 Additional comments (5)
system_test/questdb_line_sender.py (3)
294-300: LGTM!The C binding setup for
line_sender_buffer_column_decimal_stris correct and matches the signature from the Rust FFI layer.
714-719: LGTM!The
Decimalhandling implementation is correct. Converting the decimal to a string usingstr(value)is appropriate for serialization.
917-919: LGTM!The type hint for
Sender.columncorrectly includesDecimaland matches the implementation.include/questdb/ingress/line_sender.hpp (2)
85-93: LGTM!The new error codes are well-documented and follow the existing pattern. The
invalid_decimalerror code appropriately supports the new decimal functionality.
1038-1071: LGTM!The
column_decimalmethods are well-implemented and follow the same pattern as existing string column methods. The multiple overloads provide good ergonomics for different string types, and all correctly delegate to the underlying C function.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
questdb-rs/src/tests/interop/ilp-client-interop-test.json (1)
1638-1680: LGTM! Test case covers essential decimal scenarios.The test case validates decimal serialization with a good range of values including zero, negative zero, and various magnitudes. The expected line and binary formats are properly specified for both V1 and V2 protocol validation.
Consider adding edge case tests in future iterations:
- Maximum precision values (if there's a documented limit)
- Very large exponents (e.g., scientific notation)
- Boundary values for the decimal type
- Potentially invalid or out-of-range decimals (in separate error test cases)
However, these may already be covered in dedicated unit tests elsewhere in the codebase.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
questdb-rs/Cargo.toml(5 hunks)questdb-rs/build.rs(3 hunks)questdb-rs/src/tests/interop/ilp-client-interop-test.json(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
questdb-rs/build.rs (2)
include/questdb/ingress/line_sender.hpp (29)
column(860-860)column(913-946)column(913-914)column(961-977)column(961-962)column(1001-1007)column(1001-1001)column(1023-1026)column(1023-1023)column(1075-1081)column(1075-1077)column(1086-1091)column(1086-1087)name(819-825)name(819-819)name(833-839)name(833-833)name(867-873)name(867-867)name(880-886)name(880-880)name(893-899)name(893-893)name(1014-1020)name(1014-1014)name(1028-1031)name(1028-1028)name(1033-1036)name(1033-1033)system_test/questdb_line_sender.py (2)
column(687-738)column(917-921)
🔇 Additional comments (3)
questdb-rs/build.rs (3)
65-69: LGTM!The DecimalColumn struct follows the same pattern as other column types in the file, with appropriate derives for serialization.
204-208: LGTM!The code generation pattern is consistent with other column types. The
unwrap()onBigDecimal::from_stris acceptable here since this is test generation code that runs at compile time—if the JSON contains invalid decimal strings, the build will fail with a clear error message.
78-78:bigdecimalis enabled byjson_tests, soBigDecimalimport is safe.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
examples/line_sender_cpp_example_http.cpp (1)
28-28: Good demonstration of the decimal feature.The change from a double literal to a decimal literal correctly demonstrates the new decimal column support. Using decimal for the price field is semantically appropriate for financial data.
Optional: Consider adding an explanatory comment.
Since this is an example file demonstrating new functionality, you might add a brief comment explaining why decimal is preferred for financial precision:
+ // Use decimal for price to maintain exact precision for financial data .column(price_name, "2615.54"_decimal)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
CMakeLists.txt(1 hunks)doc/CPP.md(1 hunks)examples/line_sender_cpp_example.cpp(2 hunks)examples/line_sender_cpp_example_auth.cpp(2 hunks)examples/line_sender_cpp_example_auth_tls.cpp(2 hunks)examples/line_sender_cpp_example_decimal_binary.cpp(1 hunks)examples/line_sender_cpp_example_decimal_custom.cpp(1 hunks)examples/line_sender_cpp_example_from_conf.cpp(2 hunks)examples/line_sender_cpp_example_from_env.cpp(2 hunks)examples/line_sender_cpp_example_http.cpp(2 hunks)examples/line_sender_cpp_example_tls_ca.cpp(2 hunks)include/questdb/ingress/line_sender.h(2 hunks)include/questdb/ingress/line_sender.hpp(4 hunks)questdb-rs-ffi/src/decimal.rs(1 hunks)questdb-rs-ffi/src/lib.rs(5 hunks)questdb-rs/src/ingress/mod.rs(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- examples/line_sender_cpp_example.cpp
- examples/line_sender_cpp_example_tls_ca.cpp
- questdb-rs/src/ingress/mod.rs
- examples/line_sender_cpp_example_from_env.cpp
- examples/line_sender_cpp_example_auth.cpp
🧰 Additional context used
🧬 Code graph analysis (5)
include/questdb/ingress/line_sender.hpp (2)
examples/line_sender_cpp_example_decimal_custom.cpp (2)
data(29-39)_scale(24-27)questdb-rs-ffi/src/lib.rs (2)
line_sender_buffer_column_decimal_str(950-961)line_sender_buffer_column_decimal(973-986)
examples/line_sender_cpp_example_decimal_binary.cpp (1)
include/questdb/ingress/line_sender.hpp (11)
binary_view(751-756)binary_view(765-770)binary_view(779-784)binary_view(792-797)binary_view(806-811)buffer(1921-1927)buffer(1921-1921)buffer(1938-1953)buffer(1938-1938)buffer(1976-1998)buffer(1976-1977)
questdb-rs-ffi/src/lib.rs (2)
questdb-rs/src/ingress/buffer.rs (6)
column_decimal(1050-1062)new(97-102)new(181-188)new(204-263)new(303-350)new(486-488)questdb-rs-ffi/src/decimal.rs (1)
new(78-80)
examples/line_sender_cpp_example_decimal_custom.cpp (2)
include/questdb/ingress/line_sender.hpp (7)
_scale(815-818)buffer(1921-1927)buffer(1921-1921)buffer(1938-1953)buffer(1938-1938)buffer(1976-1998)buffer(1976-1977)examples/line_sender_cpp_example_decimal_binary.cpp (2)
example(8-51)example(8-8)
include/questdb/ingress/line_sender.h (3)
questdb-rs-ffi/src/lib.rs (2)
line_sender_buffer_column_decimal_str(950-961)line_sender_buffer_column_decimal(973-986)include/questdb/ingress/line_sender.hpp (22)
buffer(1921-1927)buffer(1921-1921)buffer(1938-1953)buffer(1938-1938)buffer(1976-1998)buffer(1976-1977)name(1025-1031)name(1025-1025)name(1039-1045)name(1039-1039)name(1073-1079)name(1073-1073)name(1086-1092)name(1086-1086)name(1099-1105)name(1099-1099)name(1220-1226)name(1220-1220)name(1234-1237)name(1234-1234)name(1239-1242)name(1239-1239)examples/line_sender_cpp_example_decimal_custom.cpp (1)
data(29-39)
⏰ 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). (9)
- GitHub Check: questdb.c-questdb-client (Building and testing on windows-msvc-2019)
- GitHub Check: questdb.c-questdb-client (Building and testing on windows-msvc-2022)
- GitHub Check: questdb.c-questdb-client (Building and testing on mac)
- GitHub Check: questdb.c-questdb-client (Building and testing on linux-nightly)
- GitHub Check: questdb.c-questdb-client (Building and testing on linux-beta)
- GitHub Check: questdb.c-questdb-client (Building and testing on linux-stable)
- GitHub Check: questdb.c-questdb-client (Building and testing on linux)
- GitHub Check: questdb.c-questdb-client (Building and testing Vs QuestDB 'master')
- GitHub Check: questdb.c-questdb-client (Building and testing cargo fmt and clippy)
🔇 Additional comments (20)
examples/line_sender_cpp_example_http.cpp (1)
6-6: LGTM!The using directive correctly imports the decimal namespace, enabling the
_decimalliteral suffix used on line 28. This is appropriate for an example file.doc/CPP.md (1)
79-79: LGTM! Documentation correctly demonstrates decimal literal usage.The change from a numeric literal to a decimal literal using the
_decimalsuffix accurately reflects the new decimal support introduced in this PR.include/questdb/ingress/line_sender.hpp (4)
85-93: LGTM! Error codes properly extended for decimal support.The new error codes (
array_error,protocol_version_error,invalid_decimal) are appropriately added to support the expanded functionality. Theinvalid_decimalerror code specifically addresses decimal parsing and validation errors.
640-712: Well-designed text-based decimal API.The
decimal::text_viewwrapper aroundutf8_viewprovides type safety while maintaining ergonomic usage through:
- Multiple constructor overloads for different string types
- The
_decimalliteral suffix for convenient string literals- Clean separation from regular string values at the type level
The design correctly delegates UTF-8 validation to the underlying
utf8_view.
740-843: Comprehensive binary decimal representation.The
binary_viewclass provides excellent coverage of input types through multiple constructor overloads (raw pointers, fixed arrays,std::array,std::vector, C++20std::span). The design correctly:
- Stores scale and unscaled value as big-endian bytes
- Documents constraints (max scale: 76, max mantissa: 127 bytes)
- Provides a
view()method for customization point compatibility
1244-1298: Column overloads correctly route to C API.Both decimal overloads properly:
- Accept
column_name_viewfor the column name- Call the appropriate C API function (
line_sender_buffer_column_decimal_strfor text,line_sender_buffer_column_decimalfor binary)- Use the wrapped error handling pattern
- Return
*thisfor method chainingThe text overload accesses the underlying
utf8_viewvia.view()._impl, and the binary overload correctly extracts scale, data pointer, and size.examples/line_sender_cpp_example_auth_tls.cpp (2)
6-6: LGTM! Namespace import enables decimal literal usage.The addition of
using namespace questdb::ingress::decimal;allows the example to use the_decimalsuffix on line 33.
33-33: LGTM! Price field now uses decimal literal.The change from a floating-point literal to
"2615.54"_decimalcorrectly demonstrates decimal column usage in an authenticated TLS connection context.CMakeLists.txt (1)
168-173: LGTM! Build targets added for decimal examples.The two new example targets (
line_sender_cpp_example_decimal_customandline_sender_cpp_example_decimal_binary) are correctly added to demonstrate custom decimal type integration and binary decimal usage.examples/line_sender_cpp_example_from_conf.cpp (1)
6-6: LGTM! Configuration-based example updated for decimal support.The changes correctly demonstrate decimal usage when connecting via configuration string. The namespace import (line 6) and decimal literal usage (line 28) are consistent with other examples.
Also applies to: 28-28
examples/line_sender_cpp_example_decimal_binary.cpp (1)
24-32: LGTM! Binary decimal example is clear and correct.The example properly demonstrates binary decimal usage:
- Line 24: Raw unscaled value as bytes
- Line 25-26: Comment explains the decimal representation (123 with scale 1 = 12.3)
- Line 26: Creates
binary_viewwith scale and data- Line 32: Uses the decimal value in a column
The example correctly shows that the unscaled value is stored as bytes in two's complement big-endian format.
examples/line_sender_cpp_example_decimal_custom.cpp (2)
8-45: Excellent demonstration of custom decimal type integration.The
custom_decimal::Decimal32class effectively shows how users can integrate their own decimal types with the QuestDB client. The implementation correctly:
- Stores scale and unscaled value as 32-bit integers
- Provides a
view()method that returnsbinary_view- Converts the unscaled value to big-endian byte format
- Uses
std::arrayfor automatic lifetime managementThis example serves as a clear template for users with custom decimal types.
64-71: LGTM! Usage demonstrates the customization point.The example correctly shows:
- Creating a custom decimal instance (line 65)
- Calling
.view()to convert tobinary_view(line 71)- Passing the view to the
column()methodThis validates that the customization point design works as intended.
questdb-rs-ffi/src/lib.rs (4)
47-47: LGTM! Decimal module correctly integrated.The
mod decimal;declaration (line 47) and theuse crate::decimal::Decimal;statement (line 1803) properly integrate the decimal module into the FFI layer.Also applies to: 1803-1803
229-230: LGTM! Error code properly added and mapped.The new
line_sender_error_invalid_decimalvariant and its mapping fromErrorCode::InvalidDecimalcorrectly extend the error handling for decimal parsing and validation failures.Also applies to: 259-259
942-961: LGTM! Text decimal FFI function is correct.The
line_sender_buffer_column_decimal_strfunction properly:
- Takes validated UTF-8 input via
line_sender_utf8- Converts to Rust string slice with
.as_str()- Routes to
buffer.column_decimal(name, value)- Uses the
bubble_err_to_c!macro for consistent error handlingThe function signature and implementation are safe and correct.
963-986: LGTM! Binary decimal FFI function is correct and safe.The
line_sender_buffer_column_decimalfunction properly:
- Takes scale as
u32- Takes raw byte data as pointer and length
- Creates a safe Rust slice with
slice::from_raw_parts(data, data_len)- Constructs
Decimal::new(scale, slice)- Routes to
buffer.column_decimal(name, decimal)- Uses consistent error handling
The function correctly handles binary decimal data in two's complement big-endian format. The use of
unsafeis appropriate for FFI, and the slice construction is safe as long as callers uphold the safety contract documented in the C header.include/questdb/ingress/line_sender.h (3)
86-88: LGTM! Clear and well-positioned error code.The new
line_sender_error_invalid_decimalerror code is properly documented and positioned at the end of the enum, maintaining backward compatibility. The naming follows the established convention.
502-516: LGTM! Consistent API design for decimal string input.The function declaration follows the established pattern for column functions. Documentation is clear, parameter types are appropriate, and error handling is consistent with existing APIs.
518-536: Consider documenting scale parameter constraints.The function declaration is well-designed and consistent with existing patterns. The binary format specification (two's complement, big-endian) is clear.
However, consider documenting any constraints on the
scaleparameter. For example:
- Is there a maximum scale value?
- What happens if scale exceeds supported precision?
Additionally, while
unsigned intis typically 32-bit and matches the FFI'su32, consider whetheruint32_t(fromstdint.h) would be more explicit about the size guarantees. This is a minor point and may not be necessary if the codebase already usesunsigned intconsistently, but it could improve portability clarity.To check if there are existing conventions or constraints:
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
questdb-rs-ffi/src/decimal.rs (1)
140-143: Fix wire-format byte order for decimals.Scale is emitted before the length/value, so the server treats the scale as the length and desyncs on decode. Emit length first, then value bytes, then append the scale byte.
- out.push(self.scale as u8); - out.push(self.value.len() as u8); - out.extend_from_slice(self.value); + out.push(self.value.len() as u8); + out.extend_from_slice(self.value); + out.push(self.scale as u8);
🧹 Nitpick comments (1)
questdb-rs-ffi/src/decimal.rs (1)
88-94: Align documentation with the actual wire layout.The bullet list still says “scale byte” precedes “length byte”, contradicting both the protocol spec and the fix above. Please rewrite the doc comment to state length → value → scale.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
examples/line_sender_cpp_example_decimal_custom.cpp(1 hunks)questdb-rs-ffi/src/decimal.rs(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
examples/line_sender_cpp_example_decimal_custom.cpp (2)
include/questdb/ingress/line_sender.hpp (7)
_scale(815-818)buffer(1921-1927)buffer(1921-1921)buffer(1938-1953)buffer(1938-1938)buffer(1976-1998)buffer(1976-1977)examples/line_sender_cpp_example_decimal_binary.cpp (2)
example(8-51)example(8-8)
🔇 Additional comments (3)
examples/line_sender_cpp_example_decimal_custom.cpp (3)
1-7: LGTM!The includes and namespace declarations are appropriate for demonstrating custom decimal type integration with QuestDB ILP.
47-90: Example logic is correct (assuming view() fix).The example demonstrates proper usage of the custom decimal type with QuestDB ILP, including:
- Configuration with protocol_version=2 required for decimal support
- Inline view() call within the method chain
- Appropriate error handling
Once the
view()method is fixed to avoid dangling references, this example will correctly demonstrate custom decimal integration.
110-123: LGTM!The main function correctly handles command-line arguments with sensible defaults (localhost:9009) and returns appropriate exit codes.
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 (1)
questdb-rs/build.rs(4 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-08T14:57:00.111Z
Learnt from: amunra
PR: questdb/c-questdb-client#121
File: cpp_test/test_line_sender.cpp:381-388
Timestamp: 2025-10-08T14:57:00.111Z
Learning: In tests that explicitly set `protocol_version` to a specific value (e.g., `opts.protocol_version(questdb::ingress::protocol_version::v2)`), timestamp suffix expectations should match that protocol version without requiring runtime branching, since the protocol version is constant for that test.
Applied to files:
questdb-rs/build.rs
🧬 Code graph analysis (1)
questdb-rs/build.rs (2)
include/questdb/ingress/line_sender.hpp (30)
column(907-907)column(960-993)column(960-961)column(1008-1024)column(1008-1009)column(1052-1058)column(1052-1052)column(1074-1077)column(1074-1074)column(1177-1183)column(1177-1177)column(1187-1193)column(1187-1189)column(1198-1203)name(866-872)name(866-866)name(880-886)name(880-880)name(914-920)name(914-914)name(927-933)name(927-927)name(940-946)name(940-940)name(1065-1071)name(1065-1065)name(1079-1082)name(1079-1079)name(1084-1087)name(1084-1084)system_test/questdb_line_sender.py (2)
column(689-746)column(931-935)
🔇 Additional comments (5)
questdb-rs/build.rs (5)
65-78: LGTM: DecimalColumn struct and enum variant follow established patterns.The
DecimalColumnstruct andColumn::Decimalvariant are consistent with other column types. UsingStringfor the decimal value is appropriate and avoids floating-point precision issues during deserialization.
98-110: LGTM: Default function and minimum protocol version field.The
default_minimum_protocol_versionfunction correctly defaults to version 1 for backward compatibility. The serde attributes properly map the JSON field name.
145-146: LGTM: BigDecimal imports align with best practices.The imports of
BigDecimalandFromStrare appropriate for decimal column handling. This approach aligns with best practices for BigDecimal usage, as string-based construction avoids floating-point precision issues.Based on learnings.
154-154: LGTM: Enhanced error output for debugging.Converting line bytes to a String for error output improves test diagnostics. Using
from_utf8_lossyis appropriate for error messages.
219-223: LGTM: Decimal column handling follows best practices.The generated code correctly uses
BigDecimal::from_strfor decimal column values, which is the recommended approach to avoid floating-point precision issues. Usingunwrap()is acceptable in test code, as it will produce clear panic messages if the JSON contains malformed decimal values.Based on learnings.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
include/questdb/ingress/line_sender_decimal.hpp (1)
135-139: Scale width vs. documented max 76.You keep scale as uint32_t while max is 76. If ABI across C/Rust uses u32, keeping u32 is sensible. Consider a debug-time assert to catch user mistakes early.
- decimal_view(uint32_t scale, const uint8_t* data, size_t data_size) noexcept + decimal_view(uint32_t scale, const uint8_t* data, size_t data_size) noexcept : _scale{scale} @@ { + // Optional guard (compiled out in NDEBUG): + assert(scale <= 76 && "decimal scale must be <= 76"); + assert(data_size <= 127 && "decimal mantissa must be <= 127 bytes"); }Based on learnings
Also applies to: 151-158, 178-185, 195-201, 228-232
🧹 Nitpick comments (5)
include/questdb/ingress/line_sender.hpp (3)
434-458: Add: text-decimal overload LGTM; minor nit on docs.API shape is good and matches C/Rust FFI. Tiny doc nit: “validated UTF-8 string” could mislead; validation is only UTF‑8, not numeric. Optional: clarify that numeric format is validated server-side.
460-489: Binary decimal overload: consider local precondition guard (optional).Server/FFI will reject out-of-bounds, but a cheap guard here would fail fast before crossing FFI:
- scale <= 76
- data_size <= 127
If undesired, ignore; current behavior is acceptable.
945-952: Doc typo.“adedd” → “added”.
- * calculated from minimum throughput is adedd to the value of + * calculated from minimum throughput is added to the value ofinclude/questdb/ingress/line_sender_decimal.hpp (2)
188-201: Accept const spans for binary mantissa.Current ctor takes std::span<uint8_t>. That blocks passing const data. Prefer std::span.
-#if __cplusplus >= 202002L - /** - * Construct a binary decimal view from a std::span (C++20). - * - * @param scale Number of decimal places (must be ≤ 76) - * @param span Span containing the unscaled value - */ - decimal_view(uint32_t scale, const std::span<uint8_t>& span) - : _scale{scale} - , _data{span.data()} - , _data_size{span.size()} - { - } -#endif +#if __cplusplus >= 202002L + /** + * Construct a binary decimal view from a std::span (C++20). + */ + decimal_view(uint32_t scale, std::span<const uint8_t> span) + : _scale{scale} + , _data{span.data()} + , _data_size{span.size()} + {} +#endif
133-145: Optionally mark trivial accessors/ctors noexcept.Small improvement; not required. Helps optimization and communicates intent.
- decimal_view(uint32_t scale, const uint8_t* data, size_t data_size) + decimal_view(uint32_t scale, const uint8_t* data, size_t data_size) noexcept @@ - decimal_view(uint32_t scale, const uint8_t (&data)[N]) + decimal_view(uint32_t scale, const uint8_t (&data)[N]) noexcept @@ - decimal_view(uint32_t scale, const std::vector<uint8_t>& vec) + decimal_view(uint32_t scale, const std::vector<uint8_t>& vec) noexcept @@ - uint32_t scale() const + uint32_t scale() const noexcept @@ - const uint8_t* data() const + const uint8_t* data() const noexcept @@ - size_t data_size() const + size_t data_size() const noexcept @@ - const decimal_view& view() const + const decimal_view& view() const noexceptAlso applies to: 150-159, 175-186, 203-227
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
include/questdb/ingress/line_sender.hpp(3 hunks)include/questdb/ingress/line_sender_core.hpp(1 hunks)include/questdb/ingress/line_sender_decimal.hpp(1 hunks)questdb-rs/src/tests/http.rs(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- questdb-rs/src/tests/http.rs
🧰 Additional context used
🧬 Code graph analysis (3)
include/questdb/ingress/line_sender.hpp (3)
questdb-rs-ffi/src/lib.rs (2)
line_sender_buffer_column_dec_str(958-969)line_sender_buffer_column_dec(981-994)include/questdb/ingress/line_sender_decimal.hpp (2)
decimal(251-256)decimal(251-251)system_test/questdb_line_sender.py (2)
column(689-746)column(931-935)
include/questdb/ingress/line_sender_decimal.hpp (1)
examples/line_sender_cpp_example_decimal_custom.cpp (1)
scale(12-15)
include/questdb/ingress/line_sender_core.hpp (2)
questdb-rs-ffi/src/lib.rs (3)
line_sender_error_get_code(390-394)line_sender_error_msg(400-407)line_sender_error_free(411-415)include/questdb/ingress/line_sender.hpp (13)
_impl(109-115)_impl(118-124)_impl(127-133)_impl(140-146)_impl(156-164)_impl(191-195)_impl(201-205)_impl(633-644)_impl(653-663)_impl(1012-1019)_impl(1238-1241)_impl(1246-1253)_impl(1261-1266)
🔇 Additional comments (2)
include/questdb/ingress/line_sender.hpp (1)
517-528: Customization-point overload: OK; doc matches design via .view() on decimal_view.Good use of ADL + uniform
.view()contract; no issues.include/questdb/ingress/line_sender_core.hpp (1)
419-424: operator== correctness is fine; relies on include.Once is added, this is good.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
questdb-rs/build.rs (1)
320-327: Fix incorrect rerun-if-changed path; tests won’t regenerate on JSON changes.You read from src/tests/interop/… but the rerun path uses src/test/interop/… (missing “s”).
- println!("cargo:rerun-if-changed=src/test/interop/ilp-client-interop-test.json"); + println!("cargo:rerun-if-changed=src/tests/interop/ilp-client-interop-test.json");system_test/test.py (1)
139-151: Make V3 reachable: check DECIMAL first, then arrays.Returning V2 before checking DECIMAL prevents V3 from ever being selected on 9.2.0+. Reorder:
def expected_protocol_version(self) -> qls.ProtocolVersion: """The protocol version that we expect to be handling.""" if QDB_FIXTURE.protocol_version is None: if not QDB_FIXTURE.http: return qls.ProtocolVersion.V1 - - if QDB_FIXTURE.version >= FIRST_ARRAYS_RELEASE: - return qls.ProtocolVersion.V2 - - if QDB_FIXTURE.version >= DECIMAL_RELEASE: - return qls.ProtocolVersion.V3 + if QDB_FIXTURE.version >= DECIMAL_RELEASE: + return qls.ProtocolVersion.V3 + if QDB_FIXTURE.version >= FIRST_ARRAYS_RELEASE: + return qls.ProtocolVersion.V2 return qls.ProtocolVersion.V1
♻️ Duplicate comments (1)
questdb-rs/build.rs (1)
170-174: Fix type mismatch in generated test: compare enum-to-enum or cast to u32.Current code compares (version as u8) to a u32 literal, which won’t compile. Either compare enums or cast to u32.
Minimal fix:
- if (version as u8) < {} { + if (version as u32) < {} {Better (enum-to-enum, clearer):
- for (index, spec) in specs.iter().enumerate() { + for (index, spec) in specs.iter().enumerate() { + let min_ver_expr = match spec.minimum_protocol_version { + 1 => "ProtocolVersion::V1", + 2 => "ProtocolVersion::V2", + 3 => "ProtocolVersion::V3", + _ => "ProtocolVersion::V3", + }; write!( output, indoc! {r#" @@ - if (version as u8) < {} { + if version < {} { return Ok(()); } let mut buffer = Buffer::new(version); "#}, spec.test_name, index, slugify!(&spec.test_name, separator = "_"), - spec.minimum_protocol_version + min_ver_expr )?;Also applies to: 175-179
🧹 Nitpick comments (3)
questdb-rs/src/tests/decimal.rs (1)
521-527: Minor: avoid magic number when slicing decimal binary.Compute the prefix length instead of hard‑coding 17 for clarity:
- // Skip the prefix and \n suffix - let dec_binary = &bytes[17..bytes.len() - 1]; + // Skip the "test,sym=val dec=" prefix and trailing '\n' + let prefix_len = b"test,sym=val dec=".len(); + let dec_binary = &bytes[prefix_len..bytes.len() - 1];questdb-rs/examples/http.rs (1)
1-2: Remove optional dependency from example; use &str decimal.To keep examples buildable without extra features, pass a string to column_dec:
-use std::str::FromStr; @@ -use rust_decimal::Decimal; @@ - let price = Decimal::from_str("2615.54").unwrap(); buffer .table("trades")? .symbol("symbol", "ETH-USD")? .symbol("side", "sell")? - .column_dec("price", &price)? + .column_dec("price", "2615.54")?If you prefer showcasing binary decimals, I can gate this example behind
--features rust_decimaland add a Cargo example feature stanza.Also applies to: 8-16, 20-21
questdb-rs/src/ingress/mod.rs (1)
1186-1189: Remove debug eprintln! from library code or gate behind logging.Unconditional stderr printing in SenderBuilder::build is noisy and leaks into users’ apps.
- eprintln!( - "Using protocol version {:?} with max_name_len={}", - protocol_version, max_name_len - ); + // Consider: log::debug!("Using protocol version {:?} with max_name_len={}", protocol_version, max_name_len);If you want, I can wire up
log(debug) behind an optional feature.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
questdb-rs-ffi/src/decimal.rs(1 hunks)questdb-rs-ffi/src/lib.rs(8 hunks)questdb-rs/Cargo.toml(5 hunks)questdb-rs/build.rs(4 hunks)questdb-rs/examples/basic.rs(1 hunks)questdb-rs/examples/http.rs(1 hunks)questdb-rs/examples/protocol_version.rs(3 hunks)questdb-rs/src/ingress/buffer.rs(3 hunks)questdb-rs/src/ingress/decimal.rs(1 hunks)questdb-rs/src/ingress/mod.rs(5 hunks)questdb-rs/src/ingress/sender/http.rs(1 hunks)questdb-rs/src/tests/decimal.rs(1 hunks)questdb-rs/src/tests/http.rs(2 hunks)questdb-rs/src/tests/sender.rs(5 hunks)system_test/test.py(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- questdb-rs/examples/protocol_version.rs
- questdb-rs/src/tests/http.rs
- questdb-rs/examples/basic.rs
- questdb-rs/src/ingress/buffer.rs
- questdb-rs/src/ingress/sender/http.rs
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-08T14:57:00.111Z
Learnt from: amunra
PR: questdb/c-questdb-client#121
File: cpp_test/test_line_sender.cpp:381-388
Timestamp: 2025-10-08T14:57:00.111Z
Learning: In tests that explicitly set `protocol_version` to a specific value (e.g., `opts.protocol_version(questdb::ingress::protocol_version::v2)`), timestamp suffix expectations should match that protocol version without requiring runtime branching, since the protocol version is constant for that test.
Applied to files:
questdb-rs/src/tests/sender.rsquestdb-rs/build.rs
🧬 Code graph analysis (9)
questdb-rs/examples/http.rs (4)
questdb-rs/examples/basic.rs (1)
main(8-34)questdb-rs/examples/protocol_version.rs (1)
main(10-40)questdb-rs/src/ingress/mod.rs (1)
from_conf(405-581)questdb-rs/src/ingress/sender/mod.rs (1)
from_conf(115-117)
questdb-rs/src/tests/sender.rs (2)
system_test/questdb_line_sender.py (1)
ProtocolVersion(102-122)include/questdb/ingress/line_sender.hpp (2)
version(991-999)version(991-991)
questdb-rs-ffi/src/decimal.rs (1)
questdb-rs/src/ingress/decimal.rs (4)
serialize(73-73)serialize(102-124)serialize(132-149)serialize(154-196)
questdb-rs/src/ingress/decimal.rs (1)
questdb-rs-ffi/src/decimal.rs (1)
serialize(105-136)
system_test/test.py (2)
include/questdb/ingress/line_sender.hpp (22)
version(991-999)version(991-991)column(252-252)column(305-338)column(305-306)column(353-369)column(353-354)column(397-403)column(397-397)column(419-422)column(419-419)column(522-528)column(522-522)column(532-538)column(532-534)column(543-548)buffer(1151-1157)buffer(1151-1151)buffer(1168-1183)buffer(1168-1168)buffer(1206-1228)buffer(1206-1207)system_test/questdb_line_sender.py (7)
ProtocolVersion(102-122)table(673-679)table(923-925)column(689-746)column(931-935)buffer(876-877)peek(654-661)
questdb-rs-ffi/src/lib.rs (2)
questdb-rs/src/ingress/buffer.rs (6)
column_dec(1052-1069)new(99-104)new(183-190)new(206-265)new(305-352)new(488-490)questdb-rs-ffi/src/decimal.rs (1)
new(78-80)
questdb-rs/src/ingress/mod.rs (4)
system_test/questdb_line_sender.py (3)
ProtocolVersion(102-122)protocol_version(913-916)max_name_len(919-921)questdb-rs/src/ingress/buffer.rs (2)
fmt(1373-1381)protocol_version(512-514)questdb-rs/src/ingress/sender/mod.rs (3)
fmt(70-72)protocol_version(287-289)max_name_len(297-299)include/questdb/ingress/line_sender.hpp (4)
version(991-999)version(991-991)max_name_len(927-932)max_name_len(927-927)
questdb-rs/src/tests/decimal.rs (2)
questdb-rs/src/tests/mod.rs (1)
assert_err_contains(44-69)questdb-rs/src/ingress/buffer.rs (7)
new(99-104)new(183-190)new(206-265)new(305-352)new(488-490)len(525-527)with_max_name_len(502-510)
questdb-rs/build.rs (2)
include/questdb/ingress/line_sender.hpp (30)
column(252-252)column(305-338)column(305-306)column(353-369)column(353-354)column(397-403)column(397-397)column(419-422)column(419-419)column(522-528)column(522-522)column(532-538)column(532-534)column(543-548)name(211-217)name(211-211)name(225-231)name(225-225)name(259-265)name(259-259)name(272-278)name(272-272)name(285-291)name(285-285)name(410-416)name(410-410)name(424-427)name(424-424)name(429-432)name(429-429)system_test/questdb_line_sender.py (2)
column(689-746)column(931-935)
⏰ 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). (9)
- GitHub Check: questdb.c-questdb-client (Building and testing on windows-msvc-2019)
- GitHub Check: questdb.c-questdb-client (Building and testing on windows-msvc-2022)
- GitHub Check: questdb.c-questdb-client (Building and testing on mac)
- GitHub Check: questdb.c-questdb-client (Building and testing on linux-nightly)
- GitHub Check: questdb.c-questdb-client (Building and testing on linux-beta)
- GitHub Check: questdb.c-questdb-client (Building and testing on linux-stable)
- GitHub Check: questdb.c-questdb-client (Building and testing cargo fmt and clippy)
- GitHub Check: questdb.c-questdb-client (Building and testing on linux)
- GitHub Check: questdb.c-questdb-client (Building and testing Vs QuestDB 'master')
🔇 Additional comments (20)
questdb-rs/build.rs (1)
145-147: BigDecimal usage in generated tests: ensure feature is enabled or guard imports and calls.The generated tests unconditionally import bigdecimal and use BigDecimal::from_str in Column::Decimal arms. If the crate isn’t built with the bigdecimal feature, tests will fail to compile.
Two options:
- Ensure the json_tests CI profile enables the bigdecimal feature.
- Or generate feature-gated code and fall back to text decimals when the feature is off.
Example guard:
- use bigdecimal::BigDecimal; - use std::str::FromStr; + #[cfg(feature = "bigdecimal")] + use bigdecimal::BigDecimal; + #[cfg(feature = "bigdecimal")] + use std::str::FromStr;And for the arm:
- Column::Decimal(column) => writeln!( + #[cfg(feature = "bigdecimal")] + Column::Decimal(column) => writeln!( output, "{} .column_dec({:?}, &BigDecimal::from_str({:?}).unwrap())?", indent, column.name, column.value )?, + #[cfg(not(feature = "bigdecimal"))] + Column::Decimal(column) => writeln!( + output, + "{} .column_dec({:?}, {:?})?", + indent, column.name, column.value + )?,Would you like me to send a PR update wiring the json_tests profile to build with the bigdecimal feature?
Also applies to: 219-224
questdb-rs/src/tests/sender.rs (1)
244-246: V3 parity for f64 and tests looks good.
- Param sets include V3 where relevant.
- f64 binary branch shared for V2|V3 is correct.
Also applies to: 275-279, 869-874
questdb-rs/src/ingress/decimal.rs (1)
130-148: Confirming binary order matches tests.“=, type, scale, length, mantissa” ordering is consistent with parse_binary_decimal() in tests. The 13‑byte mantissa slice for rust_decimal preserves sign extension as intended. LGTM.
If you want, I can add a debug_assert on the top 3 bytes of mantissa for positive values to document the sign-extension intent.
questdb-rs/Cargo.toml (7)
42-43: LGTM! Decimal dependencies added correctly.The optional dependencies for
rust_decimal(1.38.0) andbigdecimal(0.4.8) are appropriate for feature-gated decimal support. Both versions are current and stable.
72-78: LGTM! Formatting improved for consistency.The multiline formatting of the
sync-sender-httpfeature enhances readability without altering functionality.
101-105: LGTM! Feature declarations are properly structured.The
rust_decimalandbigdecimalfeatures are correctly declared with clear documentation about their purpose for ILP serialization.
125-127: LGTM! Meta-feature updated to include new dependencies.The
almost-all-featuresmeta-feature correctly includes the newndarray,rust_decimal, andbigdecimaldependencies, maintaining consistency with the documented pattern.
144-144: LGTM! Example demonstratesrust_decimalintegration.The
httpexample correctly requires therust_decimalfeature, allowing users to see practical usage of decimal support with therust_decimalcrate.
148-148: LGTM! Example demonstratesbigdecimalintegration.The
protocol_versionexample correctly requires thebigdecimalfeature, showcasing high-precision decimal usage with thebigdecimalcrate.
96-96: No changes needed—thebigdecimaldependency forjson_testsis necessary.The
json_testsfeature generates test code at build time that directly usesBigDecimal::from_str()when handling decimal columns. At line 217-221 ofbuild.rs, the code generator writes test code that callsBigDecimal::from_str()for each decimal column, making the dependency essential for compilation of the generated tests.questdb-rs-ffi/src/lib.rs (10)
46-46: LGTM! Decimal module declared for FFI support.The internal
decimalmodule is properly declared to support the new FFI decimal APIs.
228-229: LGTM! New error code variant added.The
line_sender_error_invalid_decimalerror code is properly declared with clear documentation.
258-258: LGTM! Error code mapping implemented.The mapping from
ErrorCode::InvalidDecimaltoline_sender_error_invalid_decimalis correctly implemented.
317-321: LGTM! Protocol Version 3 added with clear documentation.The
V3variant is well-documented, specifying decimal data type support and the minimum required QuestDB server version (9.2.0).
329-329: LGTM! V3 conversion added.The conversion from FFI
ProtocolVersion::V3to internalingress::ProtocolVersion::V3is correctly implemented.
339-339: LGTM! Reverse V3 conversion added.The conversion from internal
ingress::ProtocolVersion::V3to FFIProtocolVersion::V3is correctly implemented.
1005-1026: LGTM! Decimal string API implemented correctly.The
line_sender_buffer_column_dec_strfunction provides a clean C API for decimal string values. The implementation follows established patterns for column functions, with proper error handling and documentation.
1922-1922: LGTM! Decimal type imported for FFI use.The
Decimaltype is correctly imported from thedecimalmodule. Note that this import is placed at the end of the file, which is unconventional but valid Rust.
1028-1053: LGTM! Decimal binary API implementation verified and correct.The
line_sender_buffer_column_decfunction correctly implements the C API for decimal values. Verification confirms thatDecimal::new(scale: u32, value: &[u8])matches the usage pattern at line 1049, wherescaleis passed asu32andslice::from_raw_parts(data, data_len)correctly provides a&[u8]slice. The serialization logic properly validates scale (≤76) and value size (≤127 bytes) constraints per QuestDB server limitations.
591-591: No issues found; the change is safe and follows Rust best practices.The implementations of
line_sender_column_name::as_name(line 591) andline_sender_table_name::as_name(line 570) are identical—both containfrom_utf8_unchecked()andnew_unchecked()calls within an internal unsafe block. The signature change fromunsafe fntofnis a correct application of Rust's safe wrapper pattern: the unsafe internals are properly encapsulated, and the method maintains safety invariants. This is confirmed by line 1020 in the codebase, whereas_name()is successfully called outside an unsafe block, proving the change is intentional and works correctly.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
questdb-rs-ffi/src/decimal.rs (1)
46-49: Unify wire-format docs with implementation (scale before length).Docs say "= type len value scale" but code emits "= type scale len value". Align the docs to the implemented order (and to ingress), or change code everywhere to match server if that’s the actual spec. For now, fix the docs to match the current impl.
-/// The binary serialization format is: -/// ```text -/// '=' marker (1 byte) + type ID (1 byte) + length (1 byte) + value bytes + scale (1 byte) -/// ``` +/// The binary serialization format is: +/// ```text +/// '=' (1B) + type (1B) + scale (1B) + length (1B) + value (len B) +/// ```questdb-rs/src/ingress/decimal.rs (1)
63-67: Doc: mention protocol gating (V3).Binary decimals are negotiated under Protocol V3. Add note to avoid confusion.
/// # Binary Format Notes /// - The unscaled value must be encoded in two's complement big-endian format /// - Maximum scale is 76 -/// - Length byte indicates how many bytes follow for the unscaled value +/// - Length byte indicates how many bytes follow for the unscaled value +/// - Available starting with Protocol V3 (text format is always available)
🧹 Nitpick comments (4)
questdb-rs-ffi/src/decimal.rs (2)
38-43: Clarify signed representation (two’s‑complement, big‑endian).State explicitly that negative values must be two’s‑complement, big‑endian. Helps C/C++ callers construct correct bytes.
-/// - An unscaled integer value (the mantissa), represented as raw bytes in big-endian format +/// - An unscaled integer value (the mantissa), as two's‑complement big‑endian bytes
71-81: Optional: validate at construction.Early-check scale and length in
new()to fail fast for invalid inputs (still keep checks in serialize).pub(super) fn new(scale: u32, value: &'a [u8]) -> Self { - Self { scale, value } + // Optional early validation; keep runtime checks in serialize() as source of truth + debug_assert!(scale <= 76, "scale must be <= 76"); + debug_assert!(value.len() <= i8::MAX as usize, "value must be <= 127 bytes"); + Self { scale, value } }questdb-rs/src/ingress/decimal.rs (2)
136-146: Optional: assert sign-extension invariants for mantissa.Add a debug assertion to document the 13‑byte choice and catch regressions.
- // We skip the upper 3 bytes (which are sign-extended) and write the lower 13 bytes - let mantissa = self.mantissa(); - out.push(13); - out.extend_from_slice(&mantissa.to_be_bytes()[3..]); + // We skip the upper 3 bytes (sign-extended) and write the lower 13 bytes. + let mantissa = self.mantissa(); + let bytes = mantissa.to_be_bytes(); + debug_assert!( + bytes[0] == bytes[1] && bytes[1] == bytes[2] && (bytes[2] == 0x00 || bytes[2] == 0xFF), + "top 32 bits must be sign extension" + ); + out.push(13); + out.extend_from_slice(&bytes[3..]);
169-176: Safer cast for negative scale upscaling.Guard already limits |scale| ≤ 76, but using
try_fromdocuments intent and avoids future mistakes if the guard changes.- let unscaled = - unscaled.into_owned() * num_bigint::BigInt::from(10).pow((-scale) as u32); + let exp = u32::try_from(-scale).expect("guard ensures 0 <= -scale <= 76"); + let unscaled = unscaled.into_owned() * num_bigint::BigInt::from(10).pow(exp);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
questdb-rs-ffi/src/decimal.rs(1 hunks)questdb-rs/src/ingress/decimal.rs(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
questdb-rs-ffi/src/decimal.rs (1)
questdb-rs/src/ingress/decimal.rs (4)
serialize(73-73)serialize(101-123)serialize(131-148)serialize(153-195)
questdb-rs/src/ingress/decimal.rs (2)
questdb-rs-ffi/src/decimal.rs (1)
serialize(104-135)questdb-rs-ffi/src/lib.rs (7)
from(233-260)from(281-288)from(292-299)from(325-331)from(335-341)from(362-371)from(375-384)
⏰ 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). (9)
- GitHub Check: questdb.c-questdb-client (Building and testing on windows-msvc-2019)
- GitHub Check: questdb.c-questdb-client (Building and testing on windows-msvc-2022)
- GitHub Check: questdb.c-questdb-client (Building and testing on mac)
- GitHub Check: questdb.c-questdb-client (Building and testing on linux-nightly)
- GitHub Check: questdb.c-questdb-client (Building and testing on linux-beta)
- GitHub Check: questdb.c-questdb-client (Building and testing on linux-stable)
- GitHub Check: questdb.c-questdb-client (Building and testing on linux)
- GitHub Check: questdb.c-questdb-client (Building and testing Vs QuestDB 'master')
- GitHub Check: questdb.c-questdb-client (Building and testing cargo fmt and clippy)
🔇 Additional comments (1)
questdb-rs/src/ingress/decimal.rs (1)
132-146: No issues found—all decimal binary format implementations are consistent.All three implementations (rust_decimal, bigdecimal, and FFI) follow the identical byte order:
'='→DECIMAL_BINARY_FORMAT_TYPE→ scale → length → value. The test parser and documentation confirm this order. There is no discrepancy between emitters or a "different order" in FFI docs to fix.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
questdb-rs/src/ingress/mod.rs (1)
81-81: Derive total ordering for ProtocolVersion.Consider
Eq+Ordso ordering is total and usable in ordered collections. This was raised before.-#[derive(Debug, Copy, Clone, PartialEq, PartialOrd)] +#[derive(Debug, Copy, Clone, PartialEq, Eq, PartialOrd, Ord)]
🧹 Nitpick comments (5)
include/questdb/ingress/line_sender.hpp (3)
392-397: Avoid copies in array customization overload; use perfect forwarding and decay in SFINAE.Pass user types by forwarding ref and check trait on std::decay_t to prevent unnecessary copies and enable rvalues.
-template < - typename ToArrayViewT, - std::enable_if_t< - questdb::ingress::array::has_array_view_state_v<ToArrayViewT>, - int> = 0> -line_sender_buffer& column(column_name_view name, ToArrayViewT array) +template < + typename ToArrayViewT, + std::enable_if_t< + questdb::ingress::array::has_array_view_state_v<std::decay_t<ToArrayViewT>>, + int> = 0> +line_sender_buffer& column(column_name_view name, ToArrayViewT&& array) { may_init(); - const auto array_view_state = - questdb::ingress::array::to_array_view_state(array); + const auto array_view_state = + questdb::ingress::array::to_array_view_state(std::forward<ToArrayViewT>(array)); return column(name, array_view_state.view()); }
477-489: Binary decimal: consider early client-side validation (optional).Optionally reject invalid scale (>76) or oversize mantissa (>127 bytes) here to fail fast before crossing FFI. If already enforced in decimal::decimal_view/serializer, ignore.
517-528: Avoid copies in decimal customization overload; prefer perfect forwarding.Mirror the array overload pattern to avoid copying custom decimal wrappers.
-template < - typename ToDecimalViewT, - std::enable_if_t< - questdb::ingress::decimal::has_decimal_view_state_v<ToDecimalViewT>, - int> = 0> -line_sender_buffer& column(column_name_view name, ToDecimalViewT decimal) +template < + typename ToDecimalViewT, + std::enable_if_t< + questdb::ingress::decimal::has_decimal_view_state_v<std::decay_t<ToDecimalViewT>>, + int> = 0> +line_sender_buffer& column(column_name_view name, ToDecimalViewT&& decimal) { may_init(); - const auto decimal_view_state = - questdb::ingress::decimal::to_decimal_view_state(decimal); + const auto decimal_view_state = + questdb::ingress::decimal::to_decimal_view_state(std::forward<ToDecimalViewT>(decimal)); return column(name, decimal_view_state.view()); }questdb-rs/src/ingress/mod.rs (1)
77-79: Scope DECIMAL_BINARY_FORMAT_TYPE or justify public export.If not used outside this crate, make it
pub(crate)and drop#[allow(dead_code)]. If other crates (e.g., FFI) consume it, keep itpuband add a brief doc explaining when to use it.questdb-rs-ffi/src/lib.rs (1)
1005-1026: Doc hint (optional): note V3 requirement.Consider adding a brief note that decimal types require ProtocolVersion::V3 to help C users diagnose errors sooner.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
CMakeLists.txt(1 hunks)include/questdb/ingress/line_sender.hpp(3 hunks)questdb-rs-ffi/src/decimal.rs(1 hunks)questdb-rs-ffi/src/lib.rs(8 hunks)questdb-rs/Cargo.toml(5 hunks)questdb-rs/src/ingress/mod.rs(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- questdb-rs-ffi/src/decimal.rs
- CMakeLists.txt
🧰 Additional context used
🧬 Code graph analysis (3)
questdb-rs-ffi/src/lib.rs (2)
questdb-rs/src/ingress/buffer.rs (6)
column_dec(1052-1069)new(99-104)new(183-190)new(206-265)new(305-352)new(488-490)questdb-rs-ffi/src/decimal.rs (1)
new(78-80)
questdb-rs/src/ingress/mod.rs (4)
system_test/questdb_line_sender.py (3)
ProtocolVersion(102-122)protocol_version(913-916)max_name_len(919-921)questdb-rs/src/ingress/buffer.rs (2)
fmt(1373-1381)protocol_version(512-514)questdb-rs/src/ingress/sender/mod.rs (3)
fmt(70-72)protocol_version(287-289)max_name_len(297-299)include/questdb/ingress/line_sender.hpp (4)
version(991-999)version(991-991)max_name_len(927-932)max_name_len(927-927)
include/questdb/ingress/line_sender.hpp (4)
include/questdb/ingress/line_sender_core.hpp (5)
wrapped_call(183-191)wrapped_call(183-183)_impl(237-240)_impl(242-245)_impl(247-250)questdb-rs-ffi/src/lib.rs (2)
line_sender_buffer_column_dec_str(1013-1026)line_sender_buffer_column_dec(1038-1053)include/questdb/ingress/line_sender_decimal.hpp (2)
decimal(251-256)decimal(251-251)system_test/questdb_line_sender.py (2)
column(689-746)column(931-935)
🔇 Additional comments (13)
questdb-rs/Cargo.toml (3)
42-43: Well‑structured dependency additions for decimal support.New optional dependencies are cleanly declared with appropriate versions. The optional designation ensures backward compatibility for existing users.
72-78: Feature definitions and gating are properly integrated.The refactored
sync-sender-httpfeature group, new decimal feature definitions, and thejson_testsdependency onbigdecimalall follow established patterns and conventions. Feature comments clearly document the purpose of each new feature.Also applies to: 96-96, 101-105
125-127: Example and cross-feature coverage appropriately configured.Adding
rust_decimalandbigdecimaltoalmost-all-featuresenables comprehensive testing of both decimal serialization pathways. Examplerequired-featuresdeclarations are intentional and follow standard cargo conventions—users can skip examples if features are unavailable.Also applies to: 144-144, 148-148
include/questdb/ingress/line_sender.hpp (2)
27-28: Modular includes look good.Header surface is clearer by factoring arrays/decimals out.
434-458: Decimal text overload: API and docs read well.Nice, matches FFI name and clearly documents server requirement.
questdb-rs/src/ingress/mod.rs (4)
65-67: Good separation: decimal module and re-export.Keeps the public surface tidy while isolating impl details.
101-107: Negotiation preference list LGTM.Clear highest-to-lowest ordering; simplifies future additions.
439-445: Config parse: allowed values updated correctly.Accepts "3" and improves error message.
1169-1180: Highest-common-version selection LGTM.Straightforward and robust against future version additions.
questdb-rs-ffi/src/lib.rs (4)
46-46: Decimal module import looks good.Keeps FFI surface clean while encapsulating decimal specifics.
228-230: New InvalidDecimal error code mapping: good addition.Provides a precise category for validation failures.
Also applies to: 258-258
1005-1026: Decimal string FFI: matches Rust Buffer::column_dec path.Clear error bubbling and consistent naming with other column_* APIs.
591-597: Verification complete: no unsafe wrapping ofas_name()calls found.The search confirms that all 13 callsites to
as_name()are using the method correctly as a safe function. Notably, line 1020 demonstrates clean usage outside any outerunsafeblock, while others appropriately reside withinunsafeblocks for unrelated FFI operations (buffer manipulation). None wrap only theas_name()call itself in anunsafeblock, confirming the safety change is properly integrated across the codebase.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
questdb-rs-ffi/src/decimal.rs (1)
104-135: Consider pre-allocating capacity in serialize().The serialize method writes 4 fixed-size fields (2 for header, 1 for scale, 1 for length) plus the value bytes. Pre-allocating capacity can reduce allocations, especially for repeated calls.
Apply this diff:
fn serialize(self, out: &mut Vec<u8>) -> Result<()> { // Validate scale constraint (QuestDB server limitation) // The server's decimal implementation supports a maximum scale of 76 if self.scale > 76 { return Err(fmt_error!( InvalidDecimal, "QuestDB ILP does not support decimal scale greater than 76, got {}", self.scale )); } + // Validate value size constraint (protocol limitation) + // The length field is a single byte (i8), limiting value size to 127 bytes + let value_len = self.value.len(); + if value_len > i8::MAX as usize { + return Err(fmt_error!( + InvalidDecimal, + "QuestDB ILP does not support decimal longer than {} bytes, got {}", + i8::MAX, + value_len + )); + } + + // Reserve: header (2) + scale (1) + length (1) + value bytes + out.reserve(4 + value_len); + // Write binary format header out.push(b'='); // Binary encoding marker out.push(DECIMAL_BINARY_FORMAT_TYPE); // Type ID = 23 - // Validate value size constraint (protocol limitation) - // The length field is a single byte (i8), limiting value size to 127 bytes - if self.value.len() > i8::MAX as usize { - return Err(fmt_error!( - InvalidDecimal, - "QuestDB ILP does not support decimal longer than {} bytes, got {}", - i8::MAX, - self.value.len() - )); - } - out.push(self.scale as u8); - out.push(self.value.len() as u8); + out.push(value_len as u8); out.extend_from_slice(self.value); Ok(()) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
CMakeLists.txt(1 hunks)include/questdb/ingress/line_sender.hpp(3 hunks)questdb-rs-ffi/src/decimal.rs(1 hunks)questdb-rs-ffi/src/lib.rs(8 hunks)questdb-rs/Cargo.toml(5 hunks)questdb-rs/src/ingress/mod.rs(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- questdb-rs/Cargo.toml
🧰 Additional context used
🧬 Code graph analysis (4)
include/questdb/ingress/line_sender.hpp (4)
include/questdb/ingress/line_sender_core.hpp (5)
wrapped_call(183-191)wrapped_call(183-183)_impl(237-240)_impl(242-245)_impl(247-250)questdb-rs-ffi/src/lib.rs (2)
line_sender_buffer_column_dec_str(1013-1026)line_sender_buffer_column_dec(1038-1053)include/questdb/ingress/line_sender_decimal.hpp (2)
decimal(251-256)decimal(251-251)system_test/questdb_line_sender.py (2)
column(689-746)column(931-935)
questdb-rs-ffi/src/lib.rs (2)
questdb-rs/src/ingress/buffer.rs (6)
column_dec(1052-1069)new(99-104)new(183-190)new(206-265)new(305-352)new(488-490)questdb-rs-ffi/src/decimal.rs (1)
new(78-80)
questdb-rs/src/ingress/mod.rs (4)
system_test/questdb_line_sender.py (3)
ProtocolVersion(102-122)protocol_version(913-916)max_name_len(919-921)questdb-rs/src/ingress/buffer.rs (2)
fmt(1373-1381)protocol_version(512-514)questdb-rs/src/ingress/sender/mod.rs (3)
fmt(70-72)protocol_version(287-289)max_name_len(297-299)include/questdb/ingress/line_sender.hpp (4)
version(991-999)version(991-991)max_name_len(927-932)max_name_len(927-927)
questdb-rs-ffi/src/decimal.rs (2)
examples/line_sender_cpp_example_decimal_custom.cpp (1)
scale(12-15)questdb-rs/src/ingress/decimal.rs (4)
serialize(73-73)serialize(101-123)serialize(131-148)serialize(153-195)
⏰ 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). (9)
- GitHub Check: questdb.c-questdb-client (Building and testing on windows-msvc-2019)
- GitHub Check: questdb.c-questdb-client (Building and testing on windows-msvc-2022)
- GitHub Check: questdb.c-questdb-client (Building and testing on mac)
- GitHub Check: questdb.c-questdb-client (Building and testing on linux-nightly)
- GitHub Check: questdb.c-questdb-client (Building and testing on linux-beta)
- GitHub Check: questdb.c-questdb-client (Building and testing on linux-stable)
- GitHub Check: questdb.c-questdb-client (Building and testing on linux)
- GitHub Check: questdb.c-questdb-client (Building and testing Vs QuestDB 'master')
- GitHub Check: questdb.c-questdb-client (Building and testing cargo fmt and clippy)
🔇 Additional comments (10)
CMakeLists.txt (1)
168-173: Cleanly integrated decimal C++ examples into build system.The two new compile_example targets follow the established pattern, maintain naming consistency, and are properly placed within the QUESTDB_TESTS_AND_EXAMPLES conditional block alongside other examples. The additions align with the PR objective to provide C++ examples for decimal support (custom types and binary encoding).
questdb-rs-ffi/src/lib.rs (2)
1005-1026: LGTM! Text-based decimal column API is correct.The function correctly unwraps the buffer, column name, and UTF-8 string value, then delegates to
buffer.column_dec(). Error handling viabubble_err_to_c!is appropriate.
1028-1053: LGTM! Binary decimal column API is correct.The function correctly constructs a
Decimalfrom the raw scale and byte slice, then delegates tobuffer.column_dec(). The use ofslice::from_raw_partsis safe here as the FFI boundary guarantees the pointer and length are valid for the duration of the call.questdb-rs/src/ingress/mod.rs (3)
65-66: LGTM! Decimal module and serializer are properly exposed.The
DecimalSerializertrait is correctly re-exported for public consumption, allowing buffer operations and external implementations to serialize decimal values.
101-106: LGTM! Protocol version preference ordering is correct.The
SUPPORTED_PROTOCOL_VERSIONSconstant correctly lists protocol versions in descending order (V3, V2, V1), ensuring the client negotiates the highest mutually supported version with the server.
1169-1179: LGTM! Negotiation logic correctly uses the supported versions constant.The protocol negotiation now uses
SUPPORTED_PROTOCOL_VERSIONS.iter().find(...)to select the highest mutually supported version, replacing the previous hardcoded checks. This is more maintainable and automatically handles V3.include/questdb/ingress/line_sender.hpp (4)
27-28: LGTM! Modular header includes improve organization.Replacing the previous includes with
line_sender_array.hppandline_sender_decimal.hppmodularizes the codebase and makes dependencies explicit.
434-458: LGTM! Text-based decimal column API is well-documented.The overload for
decimal_str_viewcorrectly delegates to the underlying FFI function. The documentation clearly explains the trade-offs between text and binary formats and notes the server version requirement.
460-489: LGTM! Binary decimal column API is well-documented.The overload for
decimal_viewcorrectly passes the scale and data to the FFI layer. The documentation clearly describes the wire format constraints (scale ≤ 76, mantissa ≤ 127 bytes).
517-528: LGTM! Customization point for decimal types follows established pattern.The templated overload with
has_decimal_view_state_vconstraint mirrors the array customization point pattern, allowing users to extend support to custom decimal types. The documentation clearly explains the customization requirements.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
questdb-rs-ffi/src/lib.rs (1)
1038-1058: Reject NULL when data_len>0 and enforce protocol bounds.Current code treats NULL as empty slice regardless of length; also no guards for scale/mantissa. This risks silent data corruption and violates FFI expectations. Apply strict checks and return InvalidDecimal on violations.
pub unsafe extern "C" fn line_sender_buffer_column_dec( @@ ) -> bool { - unsafe { - let data = if data.is_null() { - &[] - } else { - slice::from_raw_parts(data, data_len) - }; - let buffer = unwrap_buffer_mut(buffer); - let name = name.as_name(); - let decimal = Decimal::new(scale, data); - bubble_err_to_c!(err_out, buffer.column_dec(name, decimal)); - } - true + unsafe { + // Protocol constraints + const MAX_SCALE: u32 = 76; + const MAX_MANTISSA: usize = 127; + if scale > MAX_SCALE { + set_err_out( + err_out, + ErrorCode::InvalidDecimal, + format!("scale {} exceeds maximum {}", scale, MAX_SCALE), + ); + return false; + } + if data_len > MAX_MANTISSA { + set_err_out( + err_out, + ErrorCode::InvalidDecimal, + format!("mantissa length {} exceeds maximum {}", data_len, MAX_MANTISSA), + ); + return false; + } + // Enforce pointer validity when length > 0 + let bytes: &[u8] = if data_len == 0 { + &[] + } else { + if data.is_null() { + set_err_out( + err_out, + ErrorCode::InvalidDecimal, + "null data pointer with non-zero length".to_string(), + ); + return false; + } + slice::from_raw_parts(data, data_len) + }; + let buffer = unwrap_buffer_mut(buffer); + let name = name.as_name(); + let decimal = Decimal::new(scale, bytes); + bubble_err_to_c!(err_out, buffer.column_dec(name, decimal)); + true + } }
🧹 Nitpick comments (5)
questdb-rs/src/ingress/mod.rs (3)
81-81: Consider deriving Eq/Ord for total ordering.Enum has a natural total order by discriminant. Deriving Eq + Ord makes sorting/Map keys straightforward and future-proofs comparisons.
-#[derive(Debug, Copy, Clone, PartialEq, PartialOrd)] +#[derive(Debug, Copy, Clone, Eq, PartialEq, Ord, PartialOrd)]
101-107: Prefer a slice to avoid array-length maintenance.Using a slice removes the need to update the const length when adding V4.
-const SUPPORTED_PROTOCOL_VERSIONS: [ProtocolVersion; 3] = [ +const SUPPORTED_PROTOCOL_VERSIONS: &[ProtocolVersion] = &[ ProtocolVersion::V3, ProtocolVersion::V2, ProtocolVersion::V1, ];
1169-1180: Negotiation: good; improve error for diagnostics (optional).Include both client and server sets in the error to ease support.
- .ok_or_else(|| { - fmt!( - ProtocolVersionError, - "Server does not support any of the client protocol versions: {:?}", - SUPPORTED_PROTOCOL_VERSIONS - ) - })? + .ok_or_else(|| { + fmt!( + ProtocolVersionError, + "No common protocol version. server={:?}, client={:?}", + protocol_versions, + SUPPORTED_PROTOCOL_VERSIONS + ) + })?questdb-rs-ffi/src/lib.rs (2)
591-598: Safety contract inconsistency with table_name.as_name.column_name.as_name is now safe but still uses unchecked UTF-8 and naming invariants; table_name.as_name remains unsafe. Consider aligning both (either make both safe with clear invariants or both unsafe) to avoid confusion.
Would you prefer making table_name.as_name safe too, given both go through validated init?
1927-1928: Move use crate::decimal::Decimal near other imports.Keeps imports localized and avoids split scopes.
+// near other use statements at the top of the file: +use crate::decimal::Decimal; @@ -use crate::decimal::Decimal;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
questdb-rs-ffi/src/lib.rs(8 hunks)questdb-rs/src/ingress/mod.rs(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
questdb-rs/src/ingress/mod.rs (2)
system_test/questdb_line_sender.py (1)
ProtocolVersion(102-122)include/questdb/ingress/line_sender.hpp (2)
version(991-999)version(991-991)
questdb-rs-ffi/src/lib.rs (2)
questdb-rs/src/ingress/buffer.rs (6)
column_dec(1052-1069)new(99-104)new(183-190)new(206-265)new(305-352)new(488-490)questdb-rs-ffi/src/decimal.rs (1)
new(78-80)
⏰ 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). (9)
- GitHub Check: questdb.c-questdb-client (Building and testing on windows-msvc-2019)
- GitHub Check: questdb.c-questdb-client (Building and testing on windows-msvc-2022)
- GitHub Check: questdb.c-questdb-client (Building and testing on mac)
- GitHub Check: questdb.c-questdb-client (Building and testing on linux-nightly)
- GitHub Check: questdb.c-questdb-client (Building and testing on linux-beta)
- GitHub Check: questdb.c-questdb-client (Building and testing on linux-stable)
- GitHub Check: questdb.c-questdb-client (Building and testing on linux)
- GitHub Check: questdb.c-questdb-client (Building and testing cargo fmt and clippy)
- GitHub Check: questdb.c-questdb-client (Building and testing Vs QuestDB 'master')
🔇 Additional comments (9)
questdb-rs/src/ingress/mod.rs (5)
65-67: Re-export looks good.Publicly exposing DecimalSerializer here keeps buffer API ergonomics clean.
77-79: OK to expose DECIMAL_BINARY_FORMAT_TYPE.Public visibility is appropriate for FFI use; allow(dead_code) is fine.
94-99: V3 docs/readiness: LGTM.Clear statement of decimal support and server ≥ 9.2.0 requirement.
113-114: Display for V3: LGTM.
439-445: Config parser accepts “3”: LGTM.Value mapping and allowed-values message updated correctly.
questdb-rs-ffi/src/lib.rs (4)
46-46: Module wiring for decimal: LGTM.
228-230: New InvalidDecimal error code and mapping: LGTM.Also applies to: 258-259
314-322: ProtocolVersion V3 in FFI: LGTM.
1005-1026: Remove this review comment—no naming divergence exists.The grep results show no references to
line_sender_buffer_column_decimal_stranywhere in the codebase. All usages—implementation, headers, examples, Python tests, C++ bindings, and documentation—consistently useline_sender_buffer_column_dec_str. There is no divergence to reconcile and no alias is needed.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
examples/line_sender_cpp_example_decimal_binary.cpp (1)
23-26: Add documentation for byte array representation and endianness.The example shows a single-byte unsigned array
{123}, but doesn't clarify:
- Endianness for multi-byte values (little-endian vs big-endian)
- How to represent negative decimals (signed interpretation, two's complement?)
- Why
uint8_tis used vsint8_tor other signed typesUsers extending this example to larger or negative values may construct the byte array incorrectly.
Consider adding a comment explaining the byte representation:
const uint8_t price_unscaled_value[] = {123}; - // 123 with a scale of 1 gives a decimal of 12.3 + // 123 with a scale of 1 gives a decimal of 12.3 + // The unscaled value is represented as a little-endian byte array. + // For multi-byte values, use: {low_byte, high_byte, ...} + // For negative values, use two's complement representation. const auto price_value = questdb::ingress::decimal::decimal_view(1, price_unscaled_value);Alternatively, add a reference to documentation that explains the binary encoding format.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
examples/line_sender_cpp_example_decimal_binary.cpp(1 hunks)examples/line_sender_cpp_example_decimal_custom.cpp(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
examples/line_sender_cpp_example_decimal_binary.cpp (3)
include/questdb/ingress/line_sender.hpp (6)
buffer(1151-1157)buffer(1151-1151)buffer(1168-1183)buffer(1168-1168)buffer(1206-1228)buffer(1206-1207)examples/line_sender_cpp_example_decimal_custom.cpp (2)
example(62-105)example(62-62)examples/line_sender_cpp_example.cpp (2)
example(8-48)example(8-8)
examples/line_sender_cpp_example_decimal_custom.cpp (2)
include/questdb/ingress/line_sender.hpp (6)
buffer(1151-1157)buffer(1151-1151)buffer(1168-1183)buffer(1168-1168)buffer(1206-1228)buffer(1206-1207)examples/line_sender_cpp_example_decimal_binary.cpp (2)
example(7-51)example(7-7)
🪛 Clang (14.0.6)
examples/line_sender_cpp_example_decimal_binary.cpp
[error] 1-1: 'questdb/ingress/line_sender.hpp' file not found
(clang-diagnostic-error)
examples/line_sender_cpp_example_decimal_custom.cpp
[error] 1-1: 'questdb/ingress/line_sender.hpp' file not found
(clang-diagnostic-error)
⏰ 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). (9)
- GitHub Check: questdb.c-questdb-client (Building and testing on windows-msvc-2019)
- GitHub Check: questdb.c-questdb-client (Building and testing on windows-msvc-2022)
- GitHub Check: questdb.c-questdb-client (Building and testing on mac)
- GitHub Check: questdb.c-questdb-client (Building and testing on linux-nightly)
- GitHub Check: questdb.c-questdb-client (Building and testing on linux-beta)
- GitHub Check: questdb.c-questdb-client (Building and testing on linux-stable)
- GitHub Check: questdb.c-questdb-client (Building and testing on linux)
- GitHub Check: questdb.c-questdb-client (Building and testing cargo fmt and clippy)
- GitHub Check: questdb.c-questdb-client (Building and testing Vs QuestDB 'master')
🔇 Additional comments (8)
examples/line_sender_cpp_example_decimal_binary.cpp (2)
60-64: Help text correctly uses dynamic program name.The previous issue with hardcoded program name has been resolved. Using
argv[0]ensures the help text displays the actual executable name.
71-84: LGTM!The argument parsing and program flow are correct. Default values are appropriate, and the return value logic correctly maps the example's boolean result to standard exit codes.
examples/line_sender_cpp_example_decimal_custom.cpp (6)
1-6: LGTM: Clean includes and namespace usage.The includes and using declarations are appropriate for the example.
8-16: LGTM: ViewHolder pattern is safe for synchronous serialization.The ViewHolder struct correctly demonstrates the intermediate state holder pattern. As confirmed in previous reviews, the
view()method safely returns adecimal_viewreferencing thedataarray because:
to_decimal_view_state_implreturns a ViewHolder by value- The templated
columnmethod stores it as a local variabledecimal_view_state- Serialization via
line_sender_buffer_column_deccompletes synchronously- The
dataarray remains valid throughout this scopeThis pattern effectively shows how to serialize custom decimal types without storing the byte array as a class member.
20-42: LGTM: Clean decimal value class.The
Decimal32class is well-structured with appropriate types (uint32_t for scale, int32_t for signed unscaled values) and provides clear accessors.
47-59: LGTM: Correct big-endian encoding in customization point.The
to_decimal_view_state_implfunction correctly demonstrates the ADL customization point pattern for integrating custom decimal types. The big-endian byte encoding is correct, properly handling signed int32_t values through the cast to uint8_t.
62-105: LGTM: Example demonstrates correct decimal usage with protocol V3.The example function correctly:
- Uses protocol version 3 for decimal column support
- Demonstrates the custom decimal type integration via the customization point
- Follows established patterns from other examples (pre-validated names, literals usage)
- Includes appropriate error handling
107-138: LGTM: Help and main functions follow best practices.The helper and main functions correctly:
- Use
argv[0]for dynamic executable name in usage string (addressing previous review feedback)- Provide sensible defaults (localhost:9009)
- Follow standard patterns from other examples
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
examples/line_sender_c_example_decimal_binary.c (1)
54-58: Consider documenting byte order for multi-byte values.The decimal binary usage is correct. The single-byte unscaled value (123) with scale 1 correctly represents 12.3.
For educational purposes, consider adding a comment about byte order for users who need larger values:
// 123 with a scale of 1 gives a decimal of 12.3 +// For multi-byte values, use big-endian byte order (most significant byte first) const uint8_t price_unscaled_value[] = {123};This would help users understand how to represent larger unscaled values that don't fit in a single byte.
examples/line_sender_cpp_example_decimal_binary.cpp (1)
22-25: Consider moving the explanatory comment before the variable declaration.The comment explaining that "123 with a scale of 1 gives a decimal of 12.3" appears after the
price_unscaled_valuearray declaration. Moving it before or inline would improve readability.Apply this diff to improve comment placement:
- const auto amount_name = "amount"_cn; - const uint8_t price_unscaled_value[] = {123}; - // 123 with a scale of 1 gives a decimal of 12.3 + const auto amount_name = "amount"_cn; + // 123 with a scale of 1 gives a decimal of 12.3 + const uint8_t price_unscaled_value[] = {123}; const auto price_value =
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
CMakeLists.txt(2 hunks)examples/line_sender_c_example_decimal_binary.c(1 hunks)examples/line_sender_cpp_example_decimal_binary.cpp(1 hunks)examples/line_sender_cpp_example_decimal_custom.cpp(1 hunks)examples/line_sender_cpp_example_http.cpp(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- CMakeLists.txt
🧰 Additional context used
🧬 Code graph analysis (3)
examples/line_sender_c_example_decimal_binary.c (2)
examples/line_sender_c_example_http.c (3)
example(7-89)displayed_help(91-107)main(109-122)questdb-rs-ffi/src/lib.rs (14)
line_sender_utf8_init(521-537)line_sender_from_conf(1722-1735)line_sender_buffer_new_for_sender(1787-1795)line_sender_buffer_reserve(762-770)line_sender_buffer_table(890-900)line_sender_buffer_symbol(910-921)line_sender_buffer_column_dec(1038-1058)line_sender_buffer_column_f64(970-981)line_sender_buffer_at_nanos(1254-1265)line_sender_flush(1845-1856)line_sender_buffer_free(730-736)line_sender_close(1810-1816)line_sender_error_msg(399-408)line_sender_error_free(412-418)
examples/line_sender_cpp_example_decimal_custom.cpp (1)
examples/line_sender_cpp_example_decimal_binary.cpp (2)
example(7-50)example(7-7)
examples/line_sender_cpp_example_decimal_binary.cpp (4)
include/questdb/ingress/line_sender.hpp (6)
buffer(1151-1157)buffer(1151-1151)buffer(1168-1183)buffer(1168-1168)buffer(1206-1228)buffer(1206-1207)examples/line_sender_c_example_decimal_binary.c (1)
example(7-92)examples/line_sender_cpp_example_decimal_custom.cpp (2)
example(62-104)example(62-62)examples/line_sender_cpp_example_http.cpp (2)
example(8-47)example(8-8)
🪛 Clang (14.0.6)
examples/line_sender_c_example_decimal_binary.c
[error] 1-1: 'questdb/ingress/line_sender.h' file not found
(clang-diagnostic-error)
examples/line_sender_cpp_example_decimal_custom.cpp
[error] 1-1: 'questdb/ingress/line_sender.hpp' file not found
(clang-diagnostic-error)
examples/line_sender_cpp_example_decimal_binary.cpp
[error] 1-1: 'questdb/ingress/line_sender.hpp' file not found
(clang-diagnostic-error)
⏰ 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). (9)
- GitHub Check: questdb.c-questdb-client (Building and testing on windows-msvc-2019)
- GitHub Check: questdb.c-questdb-client (Building and testing on windows-msvc-2022)
- GitHub Check: questdb.c-questdb-client (Building and testing on mac)
- GitHub Check: questdb.c-questdb-client (Building and testing on linux-nightly)
- GitHub Check: questdb.c-questdb-client (Building and testing on linux-beta)
- GitHub Check: questdb.c-questdb-client (Building and testing on linux-stable)
- GitHub Check: questdb.c-questdb-client (Building and testing on linux)
- GitHub Check: questdb.c-questdb-client (Building and testing Vs QuestDB 'master')
- GitHub Check: questdb.c-questdb-client (Building and testing cargo fmt and clippy)
🔇 Additional comments (12)
examples/line_sender_cpp_example_http.cpp (4)
6-6: LGTM! Decimal namespace import enables the literal syntax.The namespace import correctly brings the
_decimaluser-defined literal into scope for use on line 28.
28-28: LGTM! Demonstrates the new decimal feature correctly.The decimal literal syntax effectively showcases the new decimal API. The C++ design elegantly uses
.column()overloading with the decimal type created by the_decimalliteral operator.
56-61: LGTM! Help text improvements are clear and correct.The reformatted help text enhances readability. The port change from 9009 to 9000 is correct—9000 is the standard HTTP port for QuestDB, while 9009 is for TCP/ILP.
76-76: LGTM! Default port is consistent and correct.The default port change to 9000 is consistent with the help text and appropriate for this HTTP example.
examples/line_sender_cpp_example_decimal_custom.cpp (1)
1-137: LGTM! Clean example demonstrating custom decimal type integration.The implementation correctly demonstrates the ADL customization point pattern for integrating a custom decimal type with the QuestDB line sender. All previous review concerns have been properly addressed:
- Big-endian encoding correctly handles signed int32_t values
- ViewHolder lifetime is safe (lives as a local variable during serialization)
- Protocol negotiation handles V3 requirement automatically
- Error handling and resource management follow best practices
The example clearly illustrates the three key components: custom decimal type, ViewHolder for binary representation, and the
to_decimal_view_state_implcustomization point placed in the type's namespace for ADL.examples/line_sender_c_example_decimal_binary.c (3)
1-5: LGTM: Includes are appropriate.All necessary headers are included for the example.
Note: The static analysis warning about the missing header is a false positive—the header exists in the actual build environment.
7-92: Excellent implementation with proper resource cleanup.The example function correctly demonstrates the decimal binary format API. All previous issues have been properly addressed:
- ✅ Memory leak fixed: Buffer is now freed on the success path (line 78) in addition to the error path (line 89).
- ✅ Unused variable removed: The example now focuses solely on binary decimal format.
The resource management is correct with proper cleanup on both success and error paths.
94-129: LGTM: Helper and main functions are well-structured.The help display and main function follow standard patterns with appropriate defaults and clear usage information.
examples/line_sender_cpp_example_decimal_binary.cpp (4)
1-5: LGTM: Includes and literal imports are appropriate.The file includes the necessary headers and imports literal namespaces, which is acceptable practice for example code.
7-50: LGTM: Example demonstrates decimal binary usage correctly.The example properly demonstrates:
- Configuration using HTTP protocol
- Decimal construction with explicit scale and unscaled byte array
- Usage of
decimal_viewAPI for binary decimal representation- Proper error handling and resource management via RAII
The mathematical representation (scale=1, unscaled=123 → 12.3) is correct and well-documented.
52-69: LGTM: Help display logic is correct and self-documenting.The function correctly uses
argv[0]for the program name (avoiding the hardcoded name issue from past reviews) and accurately documents the defaults.
71-84: LGTM: Main function follows standard example pattern.The argument parsing and exit code handling are correct and consistent with other examples in the repository.
questdb-rs/src/ingress/decimal.rs
Outdated
| /// - The unscaled value must be encoded in two's complement big-endian format | ||
| /// - Maximum scale is 76 | ||
| /// - Length byte indicates how many bytes follow for the unscaled value | ||
| pub trait DecimalSerializer { |
There was a problem hiding this comment.
This might need changing.
Issues:
- Lacks encapsulation meaning we ask the user to encode our binary format. We lose control to change how it's packed over the wire.
- It allows the implementer of the trait to write any unbounded data on the wire.
Suggestion:
- It should probably be a more restrictive trait that exposes either a string binary buffer, or a DecimalView of some sorts.
- As an idea (take it or leave it), you might want to approach this by implementing a
TryInto<DecimalView>type, wherepub enum DecimalView<'a> { ScaledView(..), StringView(..) }
There was a problem hiding this comment.
Fair point, replaced it in 76104aa.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
questdb-rs-ffi/src/lib.rs (1)
1038-1058: Critical: NULL+len>0 silently coerced to empty; add bounds checks and proper NULL handlingCurrent code maps
data==NULLto an empty slice regardless ofdata_len, losing user input and hiding errors. Also add early guards for protocol limits before touching raw pointers. This mirrors Rust‑side validation and prevents UB/logic bugs previously flagged.Apply:
pub unsafe extern "C" fn line_sender_buffer_column_dec( @@ -) -> bool { - unsafe { - let data = if data.is_null() { - &[] - } else { - slice::from_raw_parts(data, data_len) - }; - let buffer = unwrap_buffer_mut(buffer); - let name = name.as_name(); - let decimal = bubble_err_to_c!(err_out, DecimalView::try_new_scaled(scale, data)); - bubble_err_to_c!(err_out, buffer.column_dec(name, decimal)); - } - true +) -> bool { + unsafe { + const MAX_SCALE: u32 = 76; + const MAX_MANTISSA: usize = 127; + if scale > MAX_SCALE { + set_err_out( + err_out, + ErrorCode::InvalidDecimal, + format!( + "QuestDB ILP does not support decimal scale greater than {}, got {}", + MAX_SCALE, scale + ), + ); + return false; + } + if data_len > MAX_MANTISSA { + set_err_out( + err_out, + ErrorCode::InvalidDecimal, + format!( + "QuestDB ILP does not support decimal longer than {} bytes, got {}", + i8::MAX, + data_len + ), + ); + return false; + } + let bytes: &[u8] = match (data.is_null(), data_len) { + (true, 0) => &[], + (true, _) => { + set_err_out( + err_out, + ErrorCode::InvalidDecimal, + "null data pointer with non-zero length".to_string(), + ); + return false; + } + (false, len) => slice::from_raw_parts(data, len), + }; + let buffer = unwrap_buffer_mut(buffer); + let name = name.as_name(); + let decimal = bubble_err_to_c!(err_out, DecimalView::try_new_scaled(scale, bytes)); + bubble_err_to_c!(err_out, buffer.column_dec(name, decimal)); + true + } }questdb-rs/src/ingress/decimal.rs (1)
230-245: Critical: unchecked cast and unbounded pow on negative BigDecimal scale can wrap or blow up CPU/RAM
(-scale) as u32silently wraps for very large negative scales and10^kcan allocate astronomically large BigInts before we later reject on length. Guard the conversion and short‑circuit using a size bound derived from the 127‑byte mantissa limit.Apply:
@@ - // QuestDB binary ILP doesn't support negative scale, we need to upscale the - // unscaled value to be compliant - let bytes = if scale < 0 { - use bigdecimal::num_bigint; - let unscaled = - unscaled.into_owned() * num_bigint::BigInt::from(10).pow((-scale) as u32); - scale = 0; - unscaled.to_signed_bytes_be() - } else { - unscaled.to_signed_bytes_be() - }; + // QuestDB ILP doesn't support negative scales; upscale to an integer mantissa. + // Safety guards: + // 1) Convert |scale| to u32 without wrap. + // 2) Estimate final byte length before doing a potentially huge pow/mul. + let bytes = if scale < 0 { + use bigdecimal::num_bigint; + // abs_neg = (-scale) as u32 (checked) + let abs_neg = match scale.checked_neg().and_then(|s| u32::try_from(s).ok()) { + Some(v) => v, + None => { + return Err(error::fmt!( + InvalidDecimal, + "QuestDB ILP does not support decimal scale magnitude greater than {}", + u32::MAX + )); + } + }; + // Predict resulting byte length: len_bytes(unscaled) + ceil(abs_neg*log2(10)/8) + let (_sign, raw) = unscaled.to_bytes_be(); + let current_len = raw.len().max(1); + let growth_bits = (abs_neg as f64) * 3.321_928_094_887_362; // log2(10) + let predicted_len = current_len as f64 + (growth_bits / 8.0).ceil(); + if predicted_len as usize > i8::MAX as usize { + return Err(error::fmt!( + InvalidDecimal, + "QuestDB ILP does not support decimal longer than {} bytes, got ~{} bytes", + i8::MAX, + predicted_len as usize + )); + } + let unscaled = + unscaled.into_owned() * num_bigint::BigInt::from(10u8).pow(abs_neg); + scale = 0; + unscaled.to_signed_bytes_be() + } else { + unscaled.to_signed_bytes_be() + };This avoids wrap, bounds the operation, and preserves the existing error wording ("decimal longer than ...").
🧹 Nitpick comments (3)
questdb-rs/src/ingress/decimal.rs (2)
168-177: Minor: pre‑reserve for binary serializationAvoid reallocations by reserving exactly the bytes we will write.
fn serialize_scaled(scale: u8, value: &[u8], out: &mut Vec<u8>) { - // Write binary format: '=' marker + type + scale + length + mantissa bytes + // Write binary format: '=' marker + type + scale + length + mantissa bytes + out.reserve(4 + value.len()); out.push(b'='); out.push(crate::ingress::DECIMAL_BINARY_FORMAT_TYPE); out.push(scale); out.push(value.len() as u8); out.extend_from_slice(value); }
82-101: Minor: share protocol limits via module constants
76andi8::MAXappear here and in FFI. Defineconst MAX_DEC_SCALE: u32 = 76; const MAX_MANTISSA_LEN: usize = i8::MAX as usize;at top of this module and reuse to avoid drift. Mirror the same constants in FFI to keep error messages consistent.questdb-rs/src/tests/decimal.rs (1)
486-502: Test idea: exercise extreme negative scales without huge powAdd a case like "1e-5000" (negative scale) that should error early without allocating, once guards are added, asserting the message contains "decimal longer than". This protects against regressions in the pre‑check.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
questdb-rs-ffi/src/lib.rs(7 hunks)questdb-rs/src/ingress/buffer.rs(3 hunks)questdb-rs/src/ingress/decimal.rs(1 hunks)questdb-rs/src/ingress/mod.rs(5 hunks)questdb-rs/src/tests/decimal.rs(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
questdb-rs/src/ingress/buffer.rs (2)
questdb-rs/src/ingress/mod.rs (4)
fmt(109-115)fmt(228-230)fmt(1325-1347)protocol_version(768-772)questdb-rs/src/ingress/sender/mod.rs (2)
fmt(70-72)protocol_version(287-289)
questdb-rs/src/tests/decimal.rs (3)
questdb-rs/src/tests/mod.rs (1)
assert_err_contains(44-69)questdb-rs/src/ingress/buffer.rs (7)
new(99-104)new(183-190)new(206-265)new(305-352)new(488-490)len(525-527)with_max_name_len(502-510)questdb-rs/src/ingress/decimal.rs (1)
try_new_string(114-142)
questdb-rs/src/ingress/decimal.rs (2)
examples/line_sender_cpp_example_decimal_custom.cpp (1)
scale(12-15)questdb-rs-ffi/src/lib.rs (7)
from(233-260)from(281-288)from(292-299)from(325-331)from(335-341)from(362-371)from(375-384)
questdb-rs/src/ingress/mod.rs (2)
system_test/questdb_line_sender.py (1)
ProtocolVersion(102-122)include/questdb/ingress/line_sender.hpp (2)
version(991-999)version(991-991)
questdb-rs-ffi/src/lib.rs (4)
system_test/questdb_line_sender.py (2)
ProtocolVersion(102-122)buffer(876-877)include/questdb/ingress/line_sender.hpp (29)
version(991-999)version(991-991)buffer(1151-1157)buffer(1151-1151)buffer(1168-1183)buffer(1168-1168)buffer(1206-1228)buffer(1206-1207)line_sender_buffer(35-44)line_sender_buffer(35-38)line_sender_buffer(46-53)line_sender_buffer(55-63)line_sender_buffer(646-650)name(211-217)name(211-211)name(225-231)name(225-225)name(259-265)name(259-259)name(272-278)name(272-272)name(285-291)name(285-285)name(410-416)name(410-410)name(424-427)name(424-424)name(429-432)name(429-429)questdb-rs/src/ingress/buffer.rs (1)
column_dec(1052-1071)questdb-rs/src/ingress/decimal.rs (1)
try_new_scaled(78-102)
⏰ 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). (9)
- GitHub Check: questdb.c-questdb-client (Building and testing on windows-msvc-2019)
- GitHub Check: questdb.c-questdb-client (Building and testing on windows-msvc-2022)
- GitHub Check: questdb.c-questdb-client (Building and testing on mac)
- GitHub Check: questdb.c-questdb-client (Building and testing on linux-nightly)
- GitHub Check: questdb.c-questdb-client (Building and testing on linux-beta)
- GitHub Check: questdb.c-questdb-client (Building and testing on linux-stable)
- GitHub Check: questdb.c-questdb-client (Building and testing cargo fmt and clippy)
- GitHub Check: questdb.c-questdb-client (Building and testing on linux)
- GitHub Check: questdb.c-questdb-client (Building and testing Vs QuestDB 'master')
🔇 Additional comments (3)
questdb-rs/src/ingress/buffer.rs (1)
1059-1071: LGTM: protocol gating and serialization path for decimalsThe V3 check, TryInto, and reuse of existing column key plumbing look correct and align with tests.
questdb-rs/src/ingress/mod.rs (1)
101-114: LGTM: ProtocolVersion V3 and preferred negotiation orderAdding PartialOrd and negotiating against SUPPORTED_PROTOCOL_VERSIONS is clear and correct. Display impl updated accordingly.
questdb-rs/src/tests/decimal.rs (1)
147-191: Nice binary parser helper; asserts match on‑wire formatHelper validates marker/type and reconstructs i128 with sign‑extension; good coverage for both features.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
questdb-rs/src/ingress/decimal.rs (2)
59-63: Doc: mention protocol gating (V3) for binary decimalsBinary decimals are only available when protocol version >= V3. Please add this to the “Binary Format Notes” to avoid confusion during negotiation.
/// #### Binary Format Notes /// - The unscaled value must be encoded in two's complement big-endian format /// - Maximum scale is 76 /// - Length byte indicates how many bytes follow for the unscaled value +/// - Binary decimal encoding is available only when the negotiated protocol version is >= V3
223-246: BigDecimal negative-scale: unchecked cast and unbounded pow can wrap or DoSCasting (-scale) as u32 can wrap for large magnitudes; pow(10, huge) risks OOM/CPU blow-ups. Bound and checked-convert the exponent, and reject values that cannot fit under the mantissa byte limit without materializing enormous BigInts.
fn try_into(self) -> Result<DecimalView<'a>> { let (unscaled, mut scale) = self.as_bigint_and_scale(); // QuestDB binary ILP doesn't support negative scale, we need to upscale the // unscaled value to be compliant - let bytes = if scale < 0 { - use bigdecimal::num_bigint; - let unscaled = - unscaled.into_owned() * num_bigdecimal::BigInt::from(10).pow((-scale) as u32); - scale = 0; - unscaled.to_signed_bytes_be() - } else { - unscaled.to_signed_bytes_be() - }; + let bytes = if scale < 0 { + use bigdecimal::num_bigint::{self, BigInt}; + // checked conversion of exponent + let k: u32 = match (-scale).try_into() { + Ok(k) => k, + Err(_) => { + return Err(error::fmt!( + InvalidDecimal, + "negative scale magnitude exceeds supported range, got {}", + scale + )) + } + }; + // Guard against pathological expansion: 127 bytes mantissa ~= 1016 bits ~= ~306 decimal digits. + // If digits(unscaled) + k exceeds this, we cannot represent it within ILP limits. + let unscaled_digits = unscaled.to_str_radix(10).len(); + const MAX_DECIMAL_DIGITS_APPROX: usize = 306; // from 127 bytes * 8 / log2(10) + if unscaled_digits + (k as usize) > MAX_DECIMAL_DIGITS_APPROX { + return Err(error::fmt!( + InvalidDecimal, + "negative scale too large to fit within {} bytes mantissa (digits={} + scale_shift={} > {})", + i8::MAX, + unscaled_digits, + k, + MAX_DECIMAL_DIGITS_APPROX + )); + } + let unscaled = unscaled.into_owned() * BigInt::from(10u8).pow(k); + scale = 0; + unscaled.to_signed_bytes_be() + } else { + unscaled.to_signed_bytes_be() + };
🧹 Nitpick comments (1)
questdb-rs/src/ingress/decimal.rs (1)
114-141: Tighten special-token validation (NaN/Infinity) while keeping lightweight checksCurrent whitelist accepts arbitrary strings like "infinite" or "nan" (lowercase) that server won’t accept. Special-case tokens first; then fall back to the existing lightweight scan for numeric forms.
- pub fn try_new_string(value: &'a str) -> Result<Self> { - // Basic validation: ensure only numerical characters are present (accepts NaN, Inf[inity], and e-notation) + pub fn try_new_string(value: &'a str) -> Result<Self> { + // Fast-path accepted special tokens + if value == "NaN" || value == "Infinity" || value == "-Infinity" { + return Ok(DecimalView::String { value }); + } + // Basic validation: allow only numerical characters and e-notation (no arbitrary words) for b in value.chars() { match b { '0'..='9' | '.' | '-' | '+' | 'e' | 'E' - | 'N' - | 'a' - | 'I' - | 'n' - | 'f' - | 'i' - | 't' - | 'y' => {} + => {} _ => { return Err(error::fmt!( InvalidDecimal, "Decimal string contains ILP reserved character {:?}", b )); } } } Ok(DecimalView::String { value }) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
questdb-rs/src/ingress/decimal.rs(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). (9)
- GitHub Check: questdb.c-questdb-client (Building and testing on windows-msvc-2019)
- GitHub Check: questdb.c-questdb-client (Building and testing on windows-msvc-2022)
- GitHub Check: questdb.c-questdb-client (Building and testing on mac)
- GitHub Check: questdb.c-questdb-client (Building and testing on linux-nightly)
- GitHub Check: questdb.c-questdb-client (Building and testing on linux-beta)
- GitHub Check: questdb.c-questdb-client (Building and testing on linux-stable)
- GitHub Check: questdb.c-questdb-client (Building and testing cargo fmt and clippy)
- GitHub Check: questdb.c-questdb-client (Building and testing on linux)
- GitHub Check: questdb.c-questdb-client (Building and testing Vs QuestDB 'master')
2 participants
This PR adds decimal support.
This is a tandem pr for:
Rust support
Decimals can be recorded with the
column_decimalmethod inBuffer.Supported types
&str- Always uses text format&rust_decimal::Decimal(feature:rust_decimal): text and binary formats.&bigdecimal::BigDecimal(feature:bigdecimal): text and binary formats.Usage
C
C++
Rust - Basic Usage
Rust - With rust_decimal
Rust - With bigdecimal
Progress
Summary by CodeRabbit
New Features
Bug Fixes / Errors
Documentation
Tests
Chores