feat(ilp): add reshape() and clear() methods for array reusability#5996
feat(ilp): add reshape() and clear() methods for array reusability#5996bluestreak01 merged 13 commits intomasterfrom
Conversation
jerrinot
added
Java
…ility Add reshape() methods to enable runtime dimension changes: - reshape(int... shape) - varargs convenience method - reshape(int[] shape, int nDim) - selective dimension usage - reshape(int dimLen) - single dimension conversion without forcing users to allocate an array Add clear() method to reset append position for error recovery after sender.cancelRow() operations. This addresses some usability issues: - Fixed shape limitation - arrays can now be resized at runtime - Single array instance reuse across different shapes - Error recovery support for partial-fill scenarios Also improves javadoc with accurate examples and clarified method behaviors.
jerrinot
force-pushed
the
jh_sender_array_usability
branch
from
65863ac to
f9d33f5
Compare
core/src/main/java/io/questdb/cutlass/line/array/AbstractArray.java
Outdated
Show resolved
Hide resolved
core/src/main/java/io/questdb/cutlass/line/array/AbstractArray.java
Outdated
Show resolved
Hide resolved
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe changes introduce new methods and documentation enhancements to the Changes
Sequence Diagram(s)sequenceDiagram
participant Test as JUnit Test
participant Array as DoubleArray
participant Sender as LineHttpSender
participant Server as QuestDB Server
Test->>Array: create (with shape)
Test->>Array: append data
Test->>Array: reshape(new shape)
Test->>Array: clear()
Test->>Sender: send(Array)
Sender->>Server: transmit data
Server-->>Test: confirm data stored
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
✨ Finishing Touches🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
![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
♻️ Duplicate comments (1)
core/src/main/java/io/questdb/cutlass/line/array/AbstractArray.java (1)
148-148: Consider limiting varargs usage to uncommon dimensions.As previously discussed, the varargs syntax causes array allocation. Since you've already provided overloads for 1D, 2D, and 3D arrays (which cover most use cases), consider either:
- Making the varargs method package-private to discourage its use, or
- Adding a comment in the Javadoc suggesting users prefer the specific overloads for better performance
🧹 Nitpick comments (1)
core/src/main/java/io/questdb/cutlass/line/array/AbstractArray.java (1)
149-151: Consider using LineSenderException for consistency.While the current exception handling is internally consistent, other methods in this class (like
toFlatOffset) throwLineSenderException. Consider usingLineSenderExceptioninstead ofIllegalStateExceptionandIllegalArgumentExceptionfor better consistency across the codebase.Also applies to: 179-181, 203-205, 229-231
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
core/src/main/java/io/questdb/cutlass/line/array/AbstractArray.java(3 hunks)core/src/main/java/io/questdb/cutlass/line/array/DoubleArray.java(3 hunks)core/src/test/java/io/questdb/test/cutlass/http/line/LineHttpSenderTest.java(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: mtopolnik
PR: questdb/questdb#5997
File: core/src/main/java/io/questdb/griffin/engine/functions/array/DoubleArrayAccessFunctionFactory.java:127-131
Timestamp: 2025-07-31T08:21:17.390Z
Learning: In DoubleArrayAccessFunctionFactory.java, overflow validation for array index casting from long to int is performed in the validateIndexArgs() method rather than at each individual cast site. The validateIndexArgs() method is called early in newInstance() to validate all index arguments, making subsequent casts safe.
📚 Learning: in doublearrayaccessfunctionfactory.java, overflow validation for array index casting from long to i...
Learnt from: mtopolnik
PR: questdb/questdb#5997
File: core/src/main/java/io/questdb/griffin/engine/functions/array/DoubleArrayAccessFunctionFactory.java:127-131
Timestamp: 2025-07-31T08:21:17.390Z
Learning: In DoubleArrayAccessFunctionFactory.java, overflow validation for array index casting from long to int is performed in the validateIndexArgs() method rather than at each individual cast site. The validateIndexArgs() method is called early in newInstance() to validate all index arguments, making subsequent casts safe.
Applied to files:
core/src/main/java/io/questdb/cutlass/line/array/DoubleArray.javacore/src/test/java/io/questdb/test/cutlass/http/line/LineHttpSenderTest.java
🔇 Additional comments (7)
core/src/main/java/io/questdb/cutlass/line/array/AbstractArray.java (2)
63-63: LGTM! Field mutability change is necessary for reshape functionality.The change from
finalto mutable is required to support dynamic reshaping of arrays.
91-113: LGTM! Clear method provides useful reusability feature.The implementation correctly resets only the append position without modifying data, which is perfect for error recovery scenarios like after
sender.cancelRow().core/src/main/java/io/questdb/cutlass/line/array/DoubleArray.java (1)
30-146: Excellent documentation improvements!The enhanced Javadoc provides clear guidance on array lifecycle, memory management, reusability patterns, and error recovery scenarios. The examples effectively demonstrate typical usage patterns.
core/src/test/java/io/questdb/test/cutlass/http/line/LineHttpSenderTest.java (4)
160-196: Well-structured test for clear() functionality.The test effectively verifies that
clear()resets the append position and that subsequent appends overwrite from the beginning.
198-231: Comprehensive reshape test coverage.The 2D reshape test properly validates the functionality by reshaping from 1x12 to 3x4 and verifying the data layout.
233-260: Good error handling test coverage.The tests thoroughly validate error scenarios for all reshape overloads. Note that if you adopt the suggestion to use
LineSenderExceptionin the implementation, these tests will need to be updated accordingly.Also applies to: 295-330, 332-352
447-481: Good example of avoiding varargs allocation.This test demonstrates the recommended pattern of using a pre-allocated
int[]array instead of varargs to avoid allocation, as discussed in the previous review comments.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
core/src/main/java/io/questdb/cutlass/line/array/AbstractArray.java (1)
167-195: Reshape method properly addresses previous concernsThe varargs reshape method is well-implemented with comprehensive validation. The previous concern about varargs allocation has been addressed by providing specific overloads for 1D, 2D, and 3D cases as requested.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
core/src/main/java/io/questdb/cutlass/line/array/AbstractArray.java(3 hunks)core/src/main/java/io/questdb/cutlass/line/array/DoubleArray.java(3 hunks)core/src/test/java/io/questdb/test/cutlass/http/line/LineHttpSenderTest.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- core/src/main/java/io/questdb/cutlass/line/array/DoubleArray.java
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: mtopolnik
PR: questdb/questdb#5997
File: core/src/main/java/io/questdb/griffin/engine/functions/array/DoubleArrayAccessFunctionFactory.java:127-131
Timestamp: 2025-07-31T08:21:17.390Z
Learning: In DoubleArrayAccessFunctionFactory.java, overflow validation for array index casting from long to int is performed in the validateIndexArgs() method rather than at each individual cast site. The validateIndexArgs() method is called early in newInstance() to validate all index arguments, making subsequent casts safe.
📚 Learning: in doublearrayaccessfunctionfactory.java, overflow validation for array index casting from long to i...
Learnt from: mtopolnik
PR: questdb/questdb#5997
File: core/src/main/java/io/questdb/griffin/engine/functions/array/DoubleArrayAccessFunctionFactory.java:127-131
Timestamp: 2025-07-31T08:21:17.390Z
Learning: In DoubleArrayAccessFunctionFactory.java, overflow validation for array index casting from long to int is performed in the validateIndexArgs() method rather than at each individual cast site. The validateIndexArgs() method is called early in newInstance() to validate all index arguments, making subsequent casts safe.
Applied to files:
core/src/test/java/io/questdb/test/cutlass/http/line/LineHttpSenderTest.java
⏰ 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). (25)
- GitHub Check: New pull request (SelfHosted Griffin And Fuzz tests on linux-x64-zfs)
- GitHub Check: New pull request (Rust Test and Lint on linux-jdk17)
- GitHub Check: New pull request (Hosted Running tests on windows-other)
- GitHub Check: New pull request (Hosted Running tests on windows-pgwire)
- GitHub Check: New pull request (Hosted Running tests on windows-cairo)
- 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)
- GitHub Check: New pull request (Hosted Running tests with cover on linux-other)
- GitHub Check: New pull request (Hosted Running tests on mac-other)
- GitHub Check: New pull request (Hosted Running tests with cover on linux-pgwire)
- GitHub Check: New pull request (Hosted Running tests on mac-pgwire)
- GitHub Check: New pull request (Hosted Running tests with cover on linux-cairo)
- GitHub Check: New pull request (Hosted Running tests on mac-cairo-fuzz)
- GitHub Check: New pull request (Hosted Running tests with cover on linux-fuzz)
- GitHub Check: New pull request (Hosted Running tests on mac-cairo)
- GitHub Check: New pull request (SelfHosted Other tests Start ARM Agent)
- GitHub Check: New pull request (Hosted Running tests with cover on linux-griffin)
- GitHub Check: New pull request (Hosted Running tests on mac-griffin)
- GitHub Check: New pull request (SelfHosted Other tests Start X64Zfs Agent)
- GitHub Check: New pull request (SelfHosted Griffin And Fuzz tests Start X64Zfs Agent)
- GitHub Check: New pull request (SelfHosted Griffin And Fuzz tests Start ARM Agent)
- GitHub Check: New pull request (Check Changes Check changes)
- GitHub Check: build
- GitHub Check: build
🔇 Additional comments (4)
core/src/main/java/io/questdb/cutlass/line/array/AbstractArray.java (3)
68-83: Well-structured validation logicThe constructor validation comprehensively checks all edge cases and uses LineSenderException consistently as requested in previous reviews. The error messages are clear and informative.
126-130: Clear method implementation is correctThe
clear()method correctly resets the append position without modifying data, which is ideal for error recovery scenarios as mentioned in the PR objectives.
204-275: Reshape overloads correctly implement requested optimizationThe three reshape overloads (1D, 2D, 3D) properly avoid varargs allocation for common use cases as requested in previous reviews. Each method has appropriate validation and follows a consistent pattern.
core/src/test/java/io/questdb/test/cutlass/http/line/LineHttpSenderTest.java (1)
160-571: Comprehensive test coverage for new array functionalityThe test suite thoroughly covers all new methods and edge cases:
- Constructor validation with various invalid inputs
- All reshape method variants (1D, 2D, 3D, varargs)
- Error conditions (closed arrays, negative dimensions, dimension limits)
- Clear method functionality
- Integration with HTTP sender
The tests follow good practices with clear naming and proper assertions.
core/src/test/java/io/questdb/test/cutlass/http/line/LineHttpSenderTest.java
Outdated
Show resolved
Hide resolved
[PR Coverage check]😍 pass : 78 / 92 (84.78%) file detail
|
4 participants
Add
reshape()methods to enable runtime dimension changes:reshape(int... shape)reshape(int dimLen),reshape(int dim1Len, int dim2Len),reshape(int dim1Len, int dim2Len, int dim3Len)- reshape a 1D, 2D, 3D array without implicitly allocating a Java array for varargsAdd
clear()method to reset append position for error recovery, e.g. aftersender.cancelRow()operations.This addresses some usability issues:
Also improves javadoc with accurate examples and clarified method behaviours.
Summary by CodeRabbit
New Features
Documentation
Tests