feat(sql): support negative index in array access#5997
Conversation
|
@mtopolnik please add PR description, examples. Title is not sufficient |
mtopolnik
changed the title
mtopolnik
changed the title
WalkthroughThe changes introduce enhancements and refinements to array access and slicing logic, especially around index handling, in both the core implementation and associated tests. They update the Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant DoubleArrayAccessFunctionFactory
participant DerivedArrayView
Client->>DoubleArrayAccessFunctionFactory: Access array with index (long)
DoubleArrayAccessFunctionFactory->>DoubleArrayAccessFunctionFactory: Validate index (1-based, negative, overflow)
DoubleArrayAccessFunctionFactory->>DerivedArrayView: slice/subArray with validated indices
DerivedArrayView->>DerivedArrayView: Apply assertion checks, perform slice
DerivedArrayView-->>DoubleArrayAccessFunctionFactory: Return sliced/subarray
DoubleArrayAccessFunctionFactory-->>Client: Return result or error
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. ✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.gitignore(2 hunks)core/src/main/java/io/questdb/cairo/arr/DerivedArrayView.java(2 hunks)core/src/main/java/io/questdb/griffin/engine/functions/array/DoubleArrayAccessFunctionFactory.java(10 hunks)core/src/test/java/io/questdb/test/cairo/ArrayTest.java(6 hunks)core/src/test/java/io/questdb/test/cutlass/pgwire/PGJobContextTest.java(9 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). (25)
- GitHub Check: New pull request (SelfHosted Other tests Start X64Zfs Agent)
- GitHub Check: New pull request (SelfHosted Other tests Start X64Zfs Agent)
- 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 on mac-pgwire)
- GitHub Check: New pull request (Hosted Running tests with cover on linux-pgwire)
- GitHub Check: New pull request (Hosted Running tests on mac-cairo-fuzz)
- GitHub Check: New pull request (Hosted Running tests with cover on linux-cairo)
- 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 on mac-griffin)
- GitHub Check: New pull request (SelfHosted Griffin And Fuzz tests Start ARM Agent)
- GitHub Check: New pull request (Hosted Running tests with cover on linux-griffin)
- GitHub Check: New pull request (SelfHosted Griffin And Fuzz tests Start X64Zfs Agent)
- GitHub Check: New pull request (Rust Test and Lint on linux-jdk17)
- GitHub Check: New pull request (Check Changes Check changes)
- GitHub Check: build
- GitHub Check: build
🔇 Additional comments (16)
.gitignore (1)
6-6: Addition looks goodIgnoring
CLAUDE.mdis harmless and keeps transient or tooling-specific files out of the repository.core/src/main/java/io/questdb/cairo/arr/DerivedArrayView.java (2)
241-247: LGTM! Clean separation of validation concerns.The removal of the
argPosparameter and replacement of runtime checks with assertions appropriately shifts validation responsibility to the caller layer. This aligns with the broader changes to support negative indexing where bounds validation and conversion likely occurs upstream.
280-290: LGTM! Consistent with slice method updates.The signature change and removal of the
argPosparameter maintains consistency with the updatedslicemethod. The functional behavior remains intact while following the new validation pattern.core/src/test/java/io/questdb/test/cutlass/pgwire/PGJobContextTest.java (1)
6952-6952: Test configuration changes are valid and unrelated to negative array indexing.The additions of
() -> staticOverrides.setProperty(PropertyKey.CAIRO_WAL_APPLY_ENABLED, "false")across the PGJobContextTest methods consistently disable WAL apply for multi-connection and deferred-commit tests, improving stability. There’s no array-indexing logic in these modified tests, so they don’t impact the PR’s negative-array-indexing feature.No further action required—approving these changes.
core/src/main/java/io/questdb/griffin/engine/functions/array/DoubleArrayAccessFunctionFactory.java (5)
53-55: LGTM! Signature updated to support long indices.The change from
IVtoLVcorrectly updates the function signature to accept long indices, which is necessary for supporting the full range of array indices and proper overflow detection.
142-156: Well-implemented negative index support!The method correctly handles both positive and negative indices with PostgreSQL-style 1-based indexing:
- Negative indices are converted using
pgIndex + dimLenwhich elegantly handles the 1-based offset- Positive indices are properly decremented for 1-based to 0-based conversion
- Appropriate bounds checking is performed
162-198: Robust validation logic with proper overflow checking.The validation correctly enforces PostgreSQL-style 1-based indexing requirements:
- Prevents zero indices/bounds which are invalid in 1-based indexing
- Properly detects integer overflow when converting from long to int
- Handles both single indices and interval (slice) arguments
217-239: Proper runtime validation for constant indices.The implementation correctly validates zero indices at runtime and provides clear error messages including dimension information. The delegation to
flatIndexDelta()ensures consistent index handling.
269-302: Comprehensive runtime validation for dynamic indices.The implementation properly handles all edge cases:
- NULL indices return NaN as expected
- Zero indices are caught with descriptive errors
- Integer overflow is detected at runtime
- Dimension indexing is correctly adjusted
core/src/test/java/io/questdb/test/cairo/ArrayTest.java (7)
99-101: LGTM! Good test coverage for array index type conversions.These test cases properly verify that array indexing works with different type conversions (long cast, string literal, and nested array element cast).
139-148: Well-designed test setup for integer overflow validation.The updated test effectively validates integer overflow handling by using a large index value (999_999_999_999). The enhanced error message format with dimension and index details improves debugging experience.
152-172: Excellent error validation coverage for array access edge cases.The enhanced tests properly validate:
- Zero index rejection (consistent with 1-based indexing)
- Dimension count mismatches with clear error messages
- Dynamic index validation for both zero and overflow cases
The descriptive error messages with contextual information will greatly help users debug issues.
173-196: Comprehensive test coverage for the new negative indexing feature.This test thoroughly validates the negative indexing functionality including:
- Basic negative index access (last and second-to-last elements)
- Array slicing with negative bounds
- Mixed positive/negative slice bounds
- Dynamic indexing using column values
- Both 1D and 2D array support
The test cases align perfectly with the PR objectives.
198-214: Proper null handling validation for array access.The test correctly validates that null indices propagate to null results, which is consistent with SQL null semantics. Coverage includes:
- Direct null index access
- Column-based null values
- Null in slice bounds (both lower and upper)
- Multi-dimensional array null handling
224-233: Consistent out-of-bounds handling for negative indices.The tests properly validate that negative indices beyond array bounds behave consistently with positive indices - returning null for element access and empty arrays for slices.
2493-2507: Enhanced slice bounds validation with detailed error messages.The updated tests properly validate that slice bounds must be non-zero (consistent with 1-based indexing) and provide detailed error context including dimension and bound values.
...rc/main/java/io/questdb/griffin/engine/functions/array/DoubleArrayAccessFunctionFactory.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
core/src/main/java/io/questdb/griffin/engine/functions/array/DoubleArrayAccessFunctionFactory.java (1)
409-434: Replace assertion with runtime overflow check.The
toIndex()method has good logic for handling negative indices and 1-based indexing, but line 415 uses an assertion for overflow checking which can be disabled in production.Replace the assertion with a runtime check:
- assert index == indexLong : "int overflow on interval " + boundName + " bound: " + indexLong; + if (index != indexLong) { + throw CairoException.nonCritical() + .position(argPos) + .put("int overflow on interval ") + .put(boundName) + .put(" bound [dim=").put(dim + 1) + .put(", bound=").put(indexLong) + .put(']'); + }
🧹 Nitpick comments (1)
core/src/main/java/io/questdb/griffin/engine/functions/array/DoubleArrayAccessFunctionFactory.java (1)
105-107: Improve error message clarity.The error message could be more descriptive about what constitutes valid arguments.
- .put("too many array access arguments [nDims=").put(nDims) - .put(", nArgs=").put(nArgs) + .put("too many array access arguments [maxDims=").put(nDims) + .put(", providedArgs=").put(nArgs)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
core/src/main/java/io/questdb/griffin/engine/functions/array/DoubleArrayAccessFunctionFactory.java(10 hunks)core/src/test/java/io/questdb/test/cairo/ArrayTest.java(6 hunks)
🔇 Additional comments (13)
core/src/main/java/io/questdb/griffin/engine/functions/array/DoubleArrayAccessFunctionFactory.java (7)
54-54: LGTM: Signature updated to support long indices.The signature change from
[](D[]IV)to[](D[]LV)correctly reflects the switch from integer to long index arguments, enabling support for larger index values and negative indexing.
142-156: LGTM: Robust index calculation with negative index support.The
flatIndexDeltamethod correctly implements:
- Negative index handling by adding to dimension length
- 1-based to 0-based index conversion for positive indices
- Proper bounds checking with null return for out-of-bounds access
159-159: LGTM: Type validation updated for long indices.The change from
INTtoLONGcorrectly reflects the updated index argument type.
177-196: LGTM: Comprehensive index validation with overflow protection.The validation logic correctly:
- Enforces non-zero bounds for slice operations
- Handles null indices appropriately
- Includes overflow detection when casting long to int
- Provides clear error messages with context
225-231: LGTM: Runtime validation for zero indices.The zero index check with proper error reporting ensures PostgreSQL-style 1-based indexing is enforced at runtime.
277-294: LGTM: Comprehensive runtime index validation.The validation includes:
- Null index handling
- Zero index rejection with clear error messages
- Overflow detection with proper exception handling
- Consistent error message format
344-390: LGTM: Enhanced slice function with proper index handling.The slice function correctly:
- Handles both interval ranges and single indices
- Validates zero indices at runtime
- Includes overflow protection
- Supports negative indexing through the index calculation logic
core/src/test/java/io/questdb/test/cairo/ArrayTest.java (6)
99-101: Good test coverage for different index types.The addition of test cases with explicit long casting and string literals properly validates the array access implementation's support for various index types.
139-178: Comprehensive error validation tests with correct messages.The updated error tests properly validate:
- Integer overflow detection when using large long values
- Error messages correctly changed from "must be positive" to "must be non-zero" to support negative indices
- Type coercion operator validation
- Argument count validation
These align well with the implementation changes for negative index support and long-to-int casting validation.
181-202: Excellent test coverage for negative index functionality.This test comprehensively validates the new negative index support:
- Direct negative indices (-1 for last element, -2 for second-to-last)
- Variable-based negative indices
- Slice operations with negative bounds
- Multi-dimensional array access with negative indices
The implementation correctly follows Python-style negative indexing semantics.
204-220: Proper null index handling tests.Good coverage of null index scenarios - all operations correctly return null when any index is null, ensuring safe null propagation.
230-239: Good boundary condition tests for negative indices.The tests properly validate that out-of-bounds negative indices return null for element access and empty arrays for slice operations, maintaining consistency with positive out-of-bounds behavior.
499-512: Consistent slice bound validation.The slice operation tests correctly enforce non-zero bounds with updated error messages, maintaining consistency with the 1-based indexing approach.
...rc/main/java/io/questdb/griffin/engine/functions/array/DoubleArrayAccessFunctionFactory.java
Show resolved
Hide resolved
[PR Coverage check]😍 pass : 79 / 85 (92.94%) file detail
|
4 participants
Negative array index selects elements from the back, so -1 is the last element.
Given
These now work:
Summary by CodeRabbit
Bug Fixes
New Features
Tests
Chores
.gitignoreto exclude additional files.