feat(ingress): add _opt builder methods to Buffer for Option<T>#134
feat(ingress): add _opt builder methods to Buffer for Option<T>#134bluestreak01 merged 4 commits intoquestdb:mainfrom
_opt builder methods to Buffer for Option<T>#134Conversation
Adds `_opt` variants for all column methods on the `Buffer` struct (e.g., `column_str_opt`, `column_i64_opt`, `column_arr_opt`). This allows users to ergonomically handle nullable database fields without breaking the fluent builder chain with `if let` statements. When passed `None`, the methods act as a no-op, which naturally maps to QuestDB's handling of NULLs (omitted columns in ILP). - Added `*_opt` methods for symbol, bool, i64, f64, str, dec, arr, and ts. - Kept generic constraints identical to standard methods for zero overhead. - Added comprehensive module-level and method-level rustdocs. - Added full integration test suite (`tests/buffer_opt.rs`) verifying exact byte-output matches for `Some`, and correct ILP syntax for `None`.
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 56 minutes and 55 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds eight Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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: 2
🧹 Nitpick comments (1)
questdb-rs/src/tests/buffer_opt.rs (1)
153-156: Prefer byte equality for decimal parity assertions.Line 153-156 converts output to UTF-8 before comparing. For protocol-encoded payloads, direct
as_bytes()equality is a stricter and less encoding-coupled assertion.♻️ Suggested test tweak
- assert_eq!( - std::str::from_utf8(opt_buf.as_bytes())?, - std::str::from_utf8(std_buf.as_bytes())? - ); + assert_eq!(opt_buf.as_bytes(), std_buf.as_bytes());🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@questdb-rs/src/tests/buffer_opt.rs` around lines 153 - 156, Replace the UTF-8 string comparison with a direct byte comparison: instead of converting opt_buf.as_bytes() and std_buf.as_bytes() to UTF-8 and asserting string equality, assert equality of the raw byte slices returned by opt_buf.as_bytes() and std_buf.as_bytes(); locate the assertion using the symbols opt_buf, std_buf and as_bytes() in the test function in buffer_opt.rs and change the assertion to compare the byte slices directly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@questdb-rs/src/ingress/buffer.rs`:
- Around line 1271-1273: Update the doc comment in ingress::buffer.rs that
currently reads “If you have an `Option<Decimal>`, use `.as_ref()` to pass it
without consuming the original string” to remove the word “string” and instead
reference the generic decimal value; e.g. change the phrase to “without
consuming the original value” (or similar) so the tip about using `.as_ref()`
for `Option<Decimal>` applies to any Decimal type, not only strings.
- Around line 1281-1297: The doctest for column_dec_opt uses bigdecimal
unguarded and will fail when the "bigdecimal" feature is disabled; wrap the
doctest block that imports bigdecimal and uses BigDecimal with a #[cfg(feature =
"bigdecimal")] guard (matching the pattern used in the earlier column_dec
doctest) so the use bigdecimal::BigDecimal and the test code only compile when
the feature is enabled; update the doctest surrounding the column_dec_opt
example accordingly.
---
Nitpick comments:
In `@questdb-rs/src/tests/buffer_opt.rs`:
- Around line 153-156: Replace the UTF-8 string comparison with a direct byte
comparison: instead of converting opt_buf.as_bytes() and std_buf.as_bytes() to
UTF-8 and asserting string equality, assert equality of the raw byte slices
returned by opt_buf.as_bytes() and std_buf.as_bytes(); locate the assertion
using the symbols opt_buf, std_buf and as_bytes() in the test function in
buffer_opt.rs and change the assertion to compare the byte slices directly.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
questdb-rs/src/ingress/buffer.rsquestdb-rs/src/ingress/mod.mdquestdb-rs/src/tests/buffer_opt.rsquestdb-rs/src/tests/mod.rs
- Gate bigdecimal doctest behind `#[cfg(feature = "bigdecimal")]` and fix
its `Option<BigDecimal>` usage to go through `.as_ref()` (TryInto is
implemented for `&BigDecimal`, not `BigDecimal`).
- Fix "original string" -> "original value" wording on column_dec_opt.
- Demonstrate `.as_ref()` on `Option<Vec<...>>` in column_arr_opt doctest.
- Expand None-skips test across V1/V2/V3 and drop single-value rstest
wrappers on the other cases.
There was a problem hiding this comment.
🧹 Nitpick comments (3)
questdb-rs/src/ingress/buffer.rs (2)
873-909: Behavior note:column_bool_opt(..., None)omits the column rather than writingfalse.Worth calling out in the doc since, per QuestDB semantics, booleans are non-nullable and missing cells are conventionally filled with
falseon the server side. Skipping the column viaNoneis consistent with the other_optwrappers and with sparse-row handling elsewhere, but end users may expect "None ⇒ false" instead of "None ⇒ column omitted." A one-line clarification in the doc comment would prevent surprise.Based on learnings: "In QuestDB, the boolean column type is non-nullable… when a boolean column is sparse (missing in some rows), the gaps are intentionally encoded as
falserather than null."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@questdb-rs/src/ingress/buffer.rs` around lines 873 - 909, Update the doc comment for the method column_bool_opt to explicitly state that passing None will omit the column (no write) rather than write false, and mention QuestDB semantics that boolean columns are non-nullable and server-side sparse/missing boolean cells are interpreted/filled as false; reference the existing convenience wrapper behavior around Self::column_bool so readers know how None maps to "skip column" vs. an explicit call to column_bool(name, false).
1429-1479: Minor ergonomics asymmetry incolumn_arr_optsignature.Unlike the other wrappers (which take
Option<S>by value),column_arr_opttakesOption<&T>, so callers with an ownedOption<Vec<...>>must remember to call.as_ref()(as the doctest shows). This mirrorscolumn_arr's&Tsignature and is fine, but consider whether acceptingOption<T>whereT: NdArrayView<D>(and internally borrowing) would be more consistent withsymbol_opt/column_str_opt. Not a blocker — the current doc tip is sufficient.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@questdb-rs/src/ingress/buffer.rs` around lines 1429 - 1479, Change column_arr_opt to accept Option<T> by value (pub fn column_arr_opt<'a, N, T, D>(&mut self, name: N, value: Option<T>) ...) where T: NdArrayView<D> so callers can pass owned Option<Vec<...>> without .as_ref(); inside the Some arm borrow the value (e.g., pass &v) into the existing self.column_arr(name, &v) and keep the same trait bounds (N: TryInto<ColumnName<'a>>, D: ArrayElement + ArrayElementSealed, Error: From<N::Error>) and return type crate::Result<&mut Self> to preserve behavior and compatibility with column_arr and related helpers.questdb-rs/src/tests/buffer_opt.rs (1)
124-161: Optional: consolidate overlapping tests.
test_buffer_opt_array_some_binary_match(V2 array) andtest_buffer_opt_decimal_some_matches_standard(V3 decimal) are already covered byte-for-byte bytest_buffer_opt_some_matches_standardat V3 for arrays/decimals. The V2-array case is the only incremental coverage. Consider folding the V2 array check into the primary test (or a parametrized version across protocol versions) to reduce duplication; the decimal-specific test looks fully redundant.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@questdb-rs/src/tests/buffer_opt.rs` around lines 124 - 161, Consolidate overlapping tests by removing the redundant V3 decimal test and folding the V2 array case into the main test_buffer_opt_some_matches_standard (or make that test parameterized by ProtocolVersion); specifically, keep a single canonical test that exercises Buffer::new, column_arr_opt/column_arr for arrays and column_dec_opt/column_dec for decimals across the relevant ProtocolVersion(s) (ensure V2 is included for arrays and V3 for decimals), and delete or merge the tests test_buffer_opt_array_some_binary_match and test_buffer_opt_decimal_some_matches_standard accordingly so byte-for-byte comparisons still call as_bytes() on the same versioned Buffer instances.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@questdb-rs/src/ingress/buffer.rs`:
- Around line 873-909: Update the doc comment for the method column_bool_opt to
explicitly state that passing None will omit the column (no write) rather than
write false, and mention QuestDB semantics that boolean columns are non-nullable
and server-side sparse/missing boolean cells are interpreted/filled as false;
reference the existing convenience wrapper behavior around Self::column_bool so
readers know how None maps to "skip column" vs. an explicit call to
column_bool(name, false).
- Around line 1429-1479: Change column_arr_opt to accept Option<T> by value (pub
fn column_arr_opt<'a, N, T, D>(&mut self, name: N, value: Option<T>) ...) where
T: NdArrayView<D> so callers can pass owned Option<Vec<...>> without .as_ref();
inside the Some arm borrow the value (e.g., pass &v) into the existing
self.column_arr(name, &v) and keep the same trait bounds (N:
TryInto<ColumnName<'a>>, D: ArrayElement + ArrayElementSealed, Error:
From<N::Error>) and return type crate::Result<&mut Self> to preserve behavior
and compatibility with column_arr and related helpers.
In `@questdb-rs/src/tests/buffer_opt.rs`:
- Around line 124-161: Consolidate overlapping tests by removing the redundant
V3 decimal test and folding the V2 array case into the main
test_buffer_opt_some_matches_standard (or make that test parameterized by
ProtocolVersion); specifically, keep a single canonical test that exercises
Buffer::new, column_arr_opt/column_arr for arrays and column_dec_opt/column_dec
for decimals across the relevant ProtocolVersion(s) (ensure V2 is included for
arrays and V3 for decimals), and delete or merge the tests
test_buffer_opt_array_some_binary_match and
test_buffer_opt_decimal_some_matches_standard accordingly so byte-for-byte
comparisons still call as_bytes() on the same versioned Buffer instances.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c39044e7-4733-4a36-be89-eb1a0e987e7c
📒 Files selected for processing (2)
questdb-rs/src/ingress/buffer.rsquestdb-rs/src/tests/buffer_opt.rs
…ecimal test
- column_bool_opt: spell out that None skips the column entirely rather than
writing false — readers wanting an explicit false should call column_bool
directly.
- Remove test_buffer_opt_decimal_some_matches_standard; the main V3 test
already round-trips column_dec_opt("c_dec", Some(...)) against column_dec.
aaaronme
force-pushed
the
feat/buffer-opt-methods
branch
from
6fe6fc8 to
a0e83be
Compare
|
@amunra Failed check bc of unrelated code |
|
@jerrinot and/or @mtopolnik might be able to help. |
- Replaced nested `if` block with a match arm guard as suggested by clippy::collapsible_match. - Improves code readability and adheres to idiomatic Rust patterns.
aaaronme
force-pushed
the
feat/buffer-opt-methods
branch
from
f2448c6 to
d0ccd79
Compare
|
@amunra I added a small commit here to fix the issue, just clippy & fmt got updated after the new rust version. if it is all ok i think we are ready for a review. Version in question: |
|
hello @aaaronme, thanks for this PR, this looks like a useful contribution. Increased API surface is a good price to pay to enable idiomatic Rust code. |
|
@aaaronme there is one thing you could do: a test to verify that every |
|
@jerrinot Thanks for the approval 🙌 I hope to be able to contribute more in the future :) I'm thinking we pull in the If you want I can add it. |
|
@aaaronme no problem at all, thanks for your contribution! |
4 participants
Closes #133
This PR introduces native support for
Option<T>in theBufferbuilder API to drastically improve the developer experience when ingesting real-world data with nullable fields.The Problem:
Previously, handling optional data forced users to break the ergonomic method chain:
The Solution:
This PR adds
_optvariants for all column types (column_str_opt,column_f64_opt,column_arr_opt, etc.). If the value is Some, it behaves exactly like the standard method. If None, it is a no-op, skipping the column perfectly in line with ILP standards.buffer .table("trades")? .symbol("symbol", "BTC-USD")? .column_f64_opt("maker_fee", trade.maker_fee)? .column_str_opt("order_id", trade.order_id.as_deref())? .at_now()?;Changes made
Added
_optmethods directly toimpl Bufferusing the exact same zero-cost generic constraints as their standard counterparts.Added a new section to the module-level documentation (
# Usage Considerations) explaining how to handleNULLs and how to correctly pass ownership of heap-allocatedOptiontypes (e.g., using.as_deref()).Documented every new method using intra-doc links to avoid duplicating complex API constraints.
Testing
As requested in the issue, a rigorous integration test suite was added in
tests/buffer_opt.rs:test_buffer_opt_some_matches_standard: UsesProtocolVersion::V3to construct one buffer with the standard methods, and one buffer with the_optmethods wrapped in Some. Validates that the resulting binary byte arrays are 100% identical (covering Strings, Numbers, Decimals, and Arrays).test_buffer_opt_none_skips_columns: Asserts that passingNoneresults in valid ILP syntax where the columns are entirely omitted from the UTF-8 output.Summary by CodeRabbit
New Features
Documentation
Tests