BREAKING CHANGE: designated and non-designated timestamp feature parity for micros and nanos #121
Conversation
amunra
changed the title
amunra
changed the title
|
@CodeRabbit review |
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
WalkthroughBumps QuestDB test version to 9.1.0; consolidates Rust timestamp encoding to a (number, suffix) model; exposes at_micros and TimestampNanos in system tests; and updates C++, Rust, and Python tests to use protocol-aware timestamp suffixes and nanos/micros expectations. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Sender as system_test.Sender
participant Buffer as system_test.Buffer
participant Ingress as questdb-rs::ingress::buffer
participant Network as LineProtocol
User->>Sender: column(...)/at(...) / at_micros(...)
Sender->>Buffer: delegate column/at/at_micros
Buffer->>Ingress: write timestamp value (micros or nanos)
Note right of Ingress: Compute (number, suffix) based on protocol & timestamp variant
alt Protocol V1
Ingress->>Ingress: convert to micros, suffix 't'
else Protocol V2
Ingress->>Ingress: choose Micros('t') or Nanos('n') per field
end
Ingress->>Network: emit line bytes with suffixed timestamp
Network->>QuestDB: deliver payload
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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🧠 Learnings (2)📓 Common learnings📚 Learning: 2025-10-08T14:57:00.111ZApplied to files:
🧬 Code graph analysis (1)system_test/test.py (2)
🔇 Additional comments (6)
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: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
cpp_test/test_line_sender.cpp (2)
330-345: Mismatch with column precision fallback.When the designated timestamp is nanos (with
nsuffix) but a timestamp column is written viacolumn("a4", a4)and the table is auto-created as micros, QuestDB downcasts silently. The test now assumes nanos even in this mixed case, so it masks regressions. Please keep the original expectation for V1 (micro suffix) and split assertions per protocol version.
921-949: New V1 test now serializes nanos wrongly.Inside
Test timestamp column V1you append" 10000000\n"previously; now,ts3formatting divides nanos by 1000, but when streamed to a proto-v1 buffer the designated timestamp should remain micros. Please restore the v1-specific expectation to avoid accidental protocol drift.
🧹 Nitpick comments (1)
cpp_test/test_line_sender.cpp (1)
1196-1202: Protocol v2 expectation should tolerate auto-created tables.If column precision is micros (default schema), the writer still emits nanos but QuestDB coerces to micros. The test enforcing
nsuffix for every column makes it brittle. Suggest checking the actual buffer contents instead of assuming suffix.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
ci/run_all_tests.py(1 hunks)cpp_test/test_line_sender.cpp(9 hunks)questdb-rs/src/ingress/buffer.rs(3 hunks)questdb-rs/src/tests/http.rs(1 hunks)questdb-rs/src/tests/sender.rs(9 hunks)system_test/questdb_line_sender.py(2 hunks)system_test/test.py(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 Vs QuestDB 'master')
- GitHub Check: questdb.c-questdb-client (Building and testing on linux)
- GitHub Check: questdb.c-questdb-client (Building and testing cargo fmt and clippy)
🔇 Additional comments (12)
ci/run_all_tests.py (1)
46-46: Keep version in sync with QuestDB fixtures.Bumping
qdb_vto9.1.0is fine, but please ensuresystem_test/fixture.py(and any other place the QuestDB version is pinned) is bumped to the same release so CI doesn’t mix binaries. Based on past regressions we’ve hit, skewed versions make the Python system tests flaky.questdb-rs/src/ingress/buffer.rs (2)
1151-1164: Protocol v1 must reject nanos columns.
column_tsnow accepts nanos for V1, converting throughTimestampMicros::try_into(). When the source isTimestamp::Nanos,try_intofails in V1 and we bubble the error, but the new match swallows that by calling.try_into()?inside the V1 arm? Actuallytimestamp.try_into()?raisesError; fine. No issue. -> skip comment.
1213-1235: Negative nanos slip through.In the V1 branch,
timestamp.try_into()converts to nanos and errors only if out of range, but negative micro timestamps should still be rejected. The new code checksnumber < 0after conversion, so fine. Skip.system_test/questdb_line_sender.py (2)
932-933: LGTM! Proper delegation pattern.The
Sender.at_microsmethod correctly delegates to the buffer'sat_microsmethod, maintaining consistency with the existingatmethod implementation.
790-794: Approveat_microsimplementationThe new
at_microsmethods mirror the existingatAPI, correctly delegate to the DLL with error wrapping, and are exercised insystem_test/test.py:376.questdb-rs/src/tests/sender.rs (7)
89-93: Protocol-version conditional logic looks correct.The conditional logic properly handles the timestamp formatting differences between V1 and V2:
- V1: Converts nanos to micros (divides by 1000) and uses 't' suffix for column timestamps, no suffix for designated timestamp
- V2: Preserves nanos precision and uses 'n' suffix for both column and designated timestamps
98-101: Expected payload formatting is consistent with protocol version.The format string correctly uses the version-dependent values and suffixes computed above. The backslash-escaped line continuations maintain readability.
149-149: Array test designated timestamps use correct V2 suffix.The 'n' suffix for nanosecond timestamps is properly applied in the array tests, consistent with protocol version 2 behavior.
Also applies to: 227-227
516-555: V1 test properly renamed and expectations updated.The test has been correctly renamed to
test_timestamp_overloads_v1and the expected payload (line 548) shows:
- Micros and converted nanos both use 't' suffix for column timestamps
- Designated timestamps have no suffix (V1 behavior)
557-597: Good test coverage for V2 timestamp behavior.The new
test_timestamp_overloads_v2test provides equivalent coverage for protocol version 2, with expectations (lines 590-591) showing:
- Micros use 't' suffix, nanos use 'n' suffix for column timestamps
- Designated timestamps include appropriate suffixes ('t' for micros, 'n' for nanos)
This split ensures both protocol versions are thoroughly tested.
611-611: Chrono timestamp test expectation updated for V2.The expected payload now correctly includes the 'n' suffix for nanosecond timestamps in protocol version 2, consistent with the broader timestamp formatting changes.
685-689: TLS tests properly handle version-dependent designated timestamps.The conditional logic for
designated_tscorrectly formats the designated timestamp based on protocol version:
- V1: No suffix (e.g., " 10000000\n")
- V2: 'n' suffix (e.g., " 10000000n\n")
This is consistently applied across both TLS tests (
test_tls_with_file_caandtest_tls_insecure_skip_verify).Also applies to: 695-695, 798-802, 806-806
|
Hi @amunra Can you share the link to the releated server PR? |
|
3 participants
Overview
Whenever protocol version 2 is enabled (explicitly, or as automatically read over HTTP from the settings endpoint) respect the timestamp precision as specified via the API.
This resolves a long-running confusion of different handling between the designated timestamps and all other timestamps. These are now treated the same.
What's changed
New
protocol_version=2behaviourIn practice if
protocol_version>=2, this means for both the designated timestamp and other timestamp columns, when using the API:Retained
protocol_version=1behaviourThe
protocol_version==1behaviour is retained as before this release:Existing schema continues to override
Just as always, if you create a table's schema via a SQL
CREATE TABLEcommand, the server will convert the timestamps to the schema-specified precision. This will happen regardless of the protocol version used by the client.Practical upgrading advice
TIMESTAMP_NSdatatype and nanosecond precision timestamps are available from QuestDB 9.1.0, but we encourage you to upgrade to QuestDB 9.1.1 which respects the client's precision when auto-creating timestamp columns.TIMESTAMP_NScolumn type will continue to use theTIMESTAMPcolumn type. This change does not break compatibility with these older releases.Summary by CodeRabbit