feat(ingress): add _opt builder methods to Buffer for Option<T>#134
Open
aaaronme wants to merge 1 commit intoquestdb:mainfrom
Open
feat(ingress): add _opt builder methods to Buffer for Option<T>#134aaaronme wants to merge 1 commit intoquestdb:mainfrom
_opt builder methods to Buffer for Option<T>#134aaaronme wants to merge 1 commit intoquestdb:mainfrom
Conversation
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`.
📝 WalkthroughWalkthroughAdds eight optional wrapper methods to the Buffer struct for ergonomic handling of Option-wrapped values in the fluent API. Each method delegates to its non-optional counterpart when Some(value) is provided and returns Ok(self) as a no-op when None, preserving method chaining without breaking the builder pattern. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 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)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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
Comment on lines
+1271
to
+1273
| /// **Tip:** If you have an `Option<Decimal>`, use `.as_ref()` to pass it | ||
| /// without consuming the original string. | ||
| /// |
There was a problem hiding this comment.
Fix wording in ownership tip for decimal options.
Line 1272 says “without consuming the original string,” but this method accepts generic decimal values, not only strings.
✏️ Suggested text change
- /// without consuming the original string.
+ /// without consuming the original value.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| /// **Tip:** If you have an `Option<Decimal>`, use `.as_ref()` to pass it | |
| /// without consuming the original string. | |
| /// | |
| /// **Tip:** If you have an `Option<Decimal>`, use `.as_ref()` to pass it | |
| /// without consuming the original value. | |
| /// |
🤖 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 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.
Comment on lines
+1281
to
+1297
| /// ```no_run | ||
| /// # use questdb::Result; | ||
| /// # use questdb::ingress::{Buffer, SenderBuilder}; | ||
| /// use bigdecimal::BigDecimal; | ||
| /// use std::str::FromStr; | ||
| /// # fn main() -> Result<()> { | ||
| /// # let mut sender = SenderBuilder::from_conf("https::addr=localhost:9000;")?.build()?; | ||
| /// # let mut buffer = sender.new_buffer(); | ||
| /// # buffer.table("x")?; | ||
| /// let val: Option<BigDecimal> = Some(BigDecimal::from_str("0.123456789012345678901234567890").unwrap()); | ||
| /// buffer.column_dec_opt("col_name", val)?; | ||
| /// | ||
| /// let no_val: Option<BigDecimal> = None; | ||
| /// buffer.column_dec_opt("skipped_col", no_val)?; | ||
| /// # Ok(()) | ||
| /// # } | ||
| /// ``` |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check if file exists first
if [ ! -f "questdb-rs/src/ingress/buffer.rs" ]; then
echo "File not found. Listing repository structure..."
find . -name "buffer.rs" -type f 2>/dev/null | head -20
exit 0
fi
# Verify bigdecimal doc blocks and feature guards in buffer docs
echo "=== Searching for feature gates and bigdecimal usage ==="
rg -n -C3 'cfg\(feature = "bigdecimal"\)|use bigdecimal::BigDecimal|column_dec_opt' questdb-rs/src/ingress/buffer.rs | head -100
# Inspect the new column_dec_opt example area (lines 1278-1302)
echo ""
echo "=== Lines 1278-1302 in buffer.rs ==="
nl -ba questdb-rs/src/ingress/buffer.rs | sed -n '1278,1302p'Repository: questdb/c-questdb-client
Length of output: 1500
Gate the bigdecimal doctest behind its feature flag.
Line 1284 introduces use bigdecimal::BigDecimal; without #[cfg(feature = "bigdecimal")], unlike the existing column_dec doctest (lines 1223-1233). This will break doctest compilation when bigdecimal is not enabled.
Proposed doc fix
- /// ```no_run
- /// # use questdb::Result;
- /// # use questdb::ingress::{Buffer, SenderBuilder};
- /// use bigdecimal::BigDecimal;
- /// use std::str::FromStr;
+ /// ```no_run
+ /// # #[cfg(feature = "bigdecimal")]
+ /// # {
+ /// # use questdb::Result;
+ /// # use questdb::ingress::{Buffer, SenderBuilder};
+ /// use bigdecimal::BigDecimal;
+ /// use std::str::FromStr;
/// # fn main() -> Result<()> {
/// # let mut sender = SenderBuilder::from_conf("https::addr=localhost:9000;")?.build()?;
/// # let mut buffer = sender.new_buffer();
/// # buffer.table("x")?;
/// let val: Option<BigDecimal> = Some(BigDecimal::from_str("0.123456789012345678901234567890").unwrap());
/// buffer.column_dec_opt("col_name", val)?;
///
/// let no_val: Option<BigDecimal> = None;
/// buffer.column_dec_opt("skipped_col", no_val)?;
/// # Ok(())
/// # }
+ /// # }
/// ```📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| /// ```no_run | |
| /// # use questdb::Result; | |
| /// # use questdb::ingress::{Buffer, SenderBuilder}; | |
| /// use bigdecimal::BigDecimal; | |
| /// use std::str::FromStr; | |
| /// # fn main() -> Result<()> { | |
| /// # let mut sender = SenderBuilder::from_conf("https::addr=localhost:9000;")?.build()?; | |
| /// # let mut buffer = sender.new_buffer(); | |
| /// # buffer.table("x")?; | |
| /// let val: Option<BigDecimal> = Some(BigDecimal::from_str("0.123456789012345678901234567890").unwrap()); | |
| /// buffer.column_dec_opt("col_name", val)?; | |
| /// | |
| /// let no_val: Option<BigDecimal> = None; | |
| /// buffer.column_dec_opt("skipped_col", no_val)?; | |
| /// # Ok(()) | |
| /// # } | |
| /// ``` | |
| /// |
🤖 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 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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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