fix(ilp): backwards incompatible way of writing null arrays#6396
fix(ilp): backwards incompatible way of writing null arrays#6396bluestreak01 merged 1 commit intomasterfrom
Conversation
puzpuzpuz
added
Bug
WalkthroughNull handling logic refactored across Line Protocol sender implementations (TCP and HTTP, versions 2 and 3) by removing a private Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10–15 minutes
Suggested labels
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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 |
[PR Coverage check]😍 pass : 10 / 18 (55.56%) file detail
|
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
core/src/main/java/io/questdb/cutlass/line/tcp/ArrayBinaryFormatParser.java (1)
77-84: Consider adding version or timeline info to the TODO.The TODO correctly identifies the backwards compatibility issue, but lacks specifics about when this code can be removed. Consider adding:
- A target version for removal (e.g., "remove in v10.0")
- A tracking issue link
- Minimum client version that supports the new behavior
This will make it easier to track and eventually clean up this temporary workaround.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
core/src/main/java/io/questdb/cutlass/line/LineTcpSenderV2.java(4 hunks)core/src/main/java/io/questdb/cutlass/line/LineTcpSenderV3.java(3 hunks)core/src/main/java/io/questdb/cutlass/line/http/LineHttpSenderV2.java(7 hunks)core/src/main/java/io/questdb/cutlass/line/http/LineHttpSenderV3.java(6 hunks)core/src/main/java/io/questdb/cutlass/line/tcp/ArrayBinaryFormatParser.java(1 hunks)core/src/test/java/io/questdb/test/cutlass/http/line/LineHttpSenderTest.java(3 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). (34)
- GitHub Check: New pull request (Coverage Report Coverage Report)
- GitHub Check: New pull request (SelfHosted Running tests with cover on linux-other)
- GitHub Check: New pull request (SelfHosted Running tests with cover on linux-pgwire)
- GitHub Check: New pull request (SelfHosted Running tests with cover on linux-cairo-sub)
- GitHub Check: New pull request (SelfHosted Running tests with cover on linux-cairo-root)
- GitHub Check: New pull request (SelfHosted Running tests with cover on linux-fuzz2)
- GitHub Check: New pull request (SelfHosted Running tests with cover on linux-fuzz1)
- GitHub Check: New pull request (SelfHosted Running tests with cover on linux-griffin-sub)
- GitHub Check: New pull request (Rust Test and Lint on linux-jdk17)
- GitHub Check: New pull request (SelfHosted Running tests with cover on linux-griffin-root)
- GitHub Check: New pull request (SelfHosted Griffin tests on linux-arm64)
- GitHub Check: New pull request (SelfHosted Other tests on linux-x64-zfs)
- GitHub Check: New pull request (SelfHosted Griffin tests on linux-x64-zfs)
- GitHub Check: New pull request (SelfHosted Other tests on linux-x86-graal)
- GitHub Check: New pull request (SelfHosted Griffin tests on linux-x86-graal)
- GitHub Check: New pull request (Hosted Running tests on windows-other-2)
- GitHub Check: New pull request (Hosted Running tests on windows-other-1)
- GitHub Check: New pull request (Hosted Running tests on windows-pgwire)
- GitHub Check: New pull request (Hosted Running tests on windows-cairo-2)
- GitHub Check: New pull request (Hosted Running tests on windows-cairo-1)
- GitHub Check: New pull request (Hosted Running tests on windows-fuzz2)
- GitHub Check: New pull request (Hosted Running tests on windows-fuzz1)
- GitHub Check: New pull request (Hosted Running tests on windows-griffin-sub)
- GitHub Check: New pull request (Hosted Running tests on windows-griffin-base)
- GitHub Check: New pull request (Hosted Running tests on mac-other)
- GitHub Check: New pull request (Hosted Running tests on mac-pgwire)
- GitHub Check: New pull request (Hosted Running tests on mac-cairo-fuzz)
- GitHub Check: New pull request (Hosted Running tests on mac-cairo)
- GitHub Check: New pull request (Hosted Running tests on mac-griffin)
- GitHub Check: New pull request (SelfHosted Cairo tests on linux-x86-graal)
- GitHub Check: New pull request (SelfHosted Other tests on linux-arm64)
- GitHub Check: New pull request (SelfHosted Cairo tests on linux-x64-zfs)
- GitHub Check: New pull request (SelfHosted Cairo tests on linux-arm64)
- GitHub Check: New pull request (Check Changes Check changes)
🔇 Additional comments (15)
core/src/test/java/io/questdb/test/cutlass/http/line/LineHttpSenderTest.java (1)
2154-2181: LGTM! Test coverage expanded for null array handling.The test correctly validates the new null array behavior by:
- Adding a 3D array column (
a3 DOUBLE[][][])- Sending null values for all array dimensions (1D, 2D, 3D)
- Verifying null values are properly stored and retrieved
This confirms the backwards-compatible fix works correctly across different array dimensionalities.
core/src/main/java/io/questdb/cutlass/line/http/LineHttpSenderV2.java (4)
53-84: Constructor parameter formatting updated.The constructor parameters are now formatted with one parameter per line. This improves readability but has no functional impact.
202-211: LGTM! Null array handling simplified and fixed.The change from checking a method on the array object to checking if the reference itself is null correctly implements the backwards-compatible fix. Now null arrays are simply omitted from the ILP message instead of encoding an unreliable
ColumnType.NULLvalue.This aligns with the PR objective to fix backwards incompatibility issues where
ColumnType.NULLnumeric value changes with new column types.
245-254: LGTM! Consistent null handling for LongArray.Same pattern as the DoubleArray method - checking for null reference and returning early to skip the field. This ensures consistent null array handling across all array types.
270-289: LGTM! Generic array method follows same null-skipping pattern.The generic
arrayColumnmethod now uses direct null checks instead of the removedprocessNullArrayhelper, ensuring all array column methods consistently skip null values rather than encoding them.core/src/main/java/io/questdb/cutlass/line/LineTcpSenderV3.java (3)
69-82: LGTM! Null-safety improved for Decimal256 columns.The added null reference check prevents potential NPE when a null Decimal256 object is passed. The method now safely handles both null references and Decimal objects with null values.
86-97: LGTM! Null-safety improved for Decimal128 columns.Consistent null-safety pattern applied - checking both null reference and null value.
101-111: LGTM! Null-safety improved for Decimal64 columns.All three decimal column overloads now have consistent null-safety checks.
core/src/main/java/io/questdb/cutlass/line/http/LineHttpSenderV3.java (4)
45-83: Constructor parameter formatting updated.One parameter per line improves readability with no functional change.
139-152: LGTM! Null-safety improved for Decimal256 columns.Consistent with the changes in LineTcpSenderV3, this prevents NPE when null Decimal256 references are passed.
156-167: LGTM! Null-safety improved for Decimal128 columns.Consistent null-safety pattern applied across HTTP sender variant.
171-181: LGTM! Null-safety improved for Decimal64 columns.All decimal column overloads now have consistent null-safety checks in both TCP and HTTP sender variants.
core/src/main/java/io/questdb/cutlass/line/LineTcpSenderV2.java (3)
114-123: LGTM! Null array handling simplified and fixed.The change to check for null reference instead of calling a method on the array object correctly implements the backwards-compatible fix. This ensures null arrays are omitted from the ILP message rather than encoding an unreliable
ColumnType.NULLvalue.
161-170: LGTM! Consistent null handling for LongArray.Same pattern applied to LongArray - null references are detected early and the field is skipped.
231-250: LGTM! Generic array method updated consistently.The generic
arrayColumnmethod now uses direct null checks, ensuring all array methods in the TCP sender variant consistently handle nulls by skipping the field rather than encoding a value.
3 participants
Senderimplementations were encodingColumnType.NULLwhen sendingnullarray values to the server. This is problematic from the compatibility perspective sinceColumnType.NULLchanges each time we add a new column type, e.g. this code changed to 40 recently in v9.2. To fix this, senders now omit the field values fornullarrays, so that the column is not included into the ILP message, like it works for other column types.