feat(sql): array_stddev(), array_min(), array_max() functions#5992
feat(sql): array_stddev(), array_min(), array_max() functions#5992bluestreak01 merged 17 commits intomasterfrom
Conversation
mtopolnik
changed the title
...rc/main/java/io/questdb/griffin/engine/functions/array/DoubleArrayStdDevFunctionFactory.java
Outdated
Show resolved
Hide resolved
...rc/main/java/io/questdb/griffin/engine/functions/array/DoubleArrayStdDevFunctionFactory.java
Outdated
Show resolved
Hide resolved
...rc/main/java/io/questdb/griffin/engine/functions/array/DoubleArrayStdDevFunctionFactory.java
Outdated
Show resolved
Hide resolved
mtopolnik
force-pushed
the
mt_array-stddev
branch
from
7186756 to
8c902d1
Compare
|
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 WalkthroughThis change set introduces new SQL functions for computing the maximum, minimum, and standard deviation (sample and population) of double arrays, including support for multi-dimensional arrays. It refactors array arithmetic and aggregation functions to use explicit recursive traversal instead of interface-based callbacks, removes legacy operator base classes and interfaces, consolidates array preparation logic, and adds comprehensive tests for the new features. Changes
Sequence Diagram(s)Example: array_max(D[]) Function FlowsequenceDiagram
participant SQLUser
participant DoubleArrayMaxFunctionFactory
participant Func
participant Record
participant ArrayView
SQLUser->>DoubleArrayMaxFunctionFactory: Request array_max(D[])
DoubleArrayMaxFunctionFactory->>Func: Create Func instance
SQLUser->>Func: getDouble(Record)
Func->>Record: Retrieve array argument
Record-->>Func: Return ArrayView
alt ArrayView is null
Func-->>SQLUser: Return Double.NaN
else ArrayView is vanilla
Func->>ArrayView: flatView().maxDouble(lo, length)
ArrayView-->>Func: Return max value
Func-->>SQLUser: Return max value
else
Func->>Func: calculateRecursive(ArrayView, dim, flatIndex)
Func-->>SQLUser: Return max value
end
Example: DoubleArrayAddScalarFunctionFactory (Refactored)sequenceDiagram
participant SQLUser
participant DoubleArrayAddScalarFunctionFactory
participant Func
participant Record
participant ArrayView
participant DirectArray
SQLUser->>DoubleArrayAddScalarFunctionFactory: Request array + scalar
DoubleArrayAddScalarFunctionFactory->>Func: Create Func instance
SQLUser->>Func: getArray(Record)
Func->>Record: Retrieve array and scalar arguments
Record-->>Func: Return ArrayView and scalar
Func->>DirectArray: prepare(type, ArrayView)
DirectArray-->>Func: Return MemoryA
alt ArrayView is vanilla
Func->>Func: Loop over flat indices, add scalar
else
Func->>Func: calculateRecursive(ArrayView, dim, flatIndex)
end
Func-->>SQLUser: Return result ArrayView
Estimated code review effort🎯 5 (Critical) | ⏱️ ~90 minutes Poem
✨ 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
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: 2
🔭 Outside diff range comments (2)
core/src/main/java/io/questdb/griffin/engine/functions/array/DoubleArrayAndScalarDotProductFunctionFactory.java (1)
79-101: Remove redundant null check.The array null check on line 89 is redundant since it's already checked on lines 80-82. This creates unnecessary nesting and confusion in the control flow.
ArrayView arr = leftArg.getArray(rec); if (arr.isNull()) { return Double.NaN; } scalar = rightArg.getDouble(rec); if (Numbers.isNull(scalar)) { return Double.NaN; } value = 0d; -if (!arr.isNull()) { - if (arr.isVanilla()) { - FlatArrayView flatView = arr.flatView(); - for (int i = arr.getLo(), n = arr.getHi(); i < n; i++) { - double v = flatView.getDoubleAtAbsIndex(i); - if (Numbers.isFinite(v)) { - value += v * scalar; - } +if (arr.isVanilla()) { + FlatArrayView flatView = arr.flatView(); + for (int i = arr.getLo(), n = arr.getHi(); i < n; i++) { + double v = flatView.getDoubleAtAbsIndex(i); + if (Numbers.isFinite(v)) { + value += v * scalar; } - } else { - calculateRecursive(arr, 0, 0); } +} else { + calculateRecursive(arr, 0, 0); }core/src/main/java/io/questdb/griffin/engine/functions/array/DoubleArrayCumSumFunctionFactory.java (1)
84-109: Remove redundant null check and consolidate duplicate logic.The code has a redundant null check on line 96 (array was already checked on line 85) and duplicate logic for checking if no non-null values were found (lines 103-106 and 110-113).
ArrayView arr = arrayArg.getArray(rec); if (arr.isNull()) { array.ofNull(); return array; } currentSum = Double.NaN; compensation = 0d; array.setType(getType()); array.setDimLen(0, arr.getCardinality()); array.applyShape(); memory = array.startMemoryA(); -if (arr.isNull()) { - array.ofNull(); -} else if (arr.isVanilla()) { +if (arr.isVanilla()) { FlatArrayView flatView = arr.flatView(); for (int i = arr.getLo(), n = arr.getHi(); i < n; i++) { accumulate(flatView.getDoubleAtAbsIndex(i)); } - if (compensation == 0d && Numbers.isNull(currentSum)) { - // no non-null values so return null - array.ofNull(); - } } else { calculateRecursive(arr, 0, 0); } if (compensation == 0d && Numbers.isNull(currentSum)) { // no non-null values so return null array.ofNull(); }
🧹 Nitpick comments (7)
.gitignore (2)
50-50: Clarify whethergorunneris a file or directoryIf
gorunneris a directory produced by the Go test harness, append a trailing slash to avoid unintentionally ignoring any future file namedgorunnerelsewhere in the tree.-compat/src/test/golang/gorunner +compat/src/test/golang/gorunner/
51-51: Anchor the ignore rule to the repository rootPrefixing the pattern with a slash ensures only the top-level
CLAUDE.mdis ignored, not any file with the same name nested inside sub-directories.-CLAUDE.md +/CLAUDE.mdcore/src/main/java/io/questdb/griffin/engine/functions/array/DoubleArrayStdDevPopFunctionFactory.java (1)
64-66: Add @OverRide annotation to getName() method.The
getName()method overrides the parent class method and should have the@Overrideannotation for consistency and compile-time verification.+@Override public String getName() { return FUNCTION_NAME; }core/src/main/java/io/questdb/griffin/engine/functions/array/DoubleNegArrayFunctionFactory.java (1)
97-99: Add @OverRide annotation to getName() method.The
getName()method should have the@Overrideannotation for consistency with other overridden methods.+@Override public String getName() { return OPERATOR_NAME; }core/src/main/java/io/questdb/griffin/engine/functions/array/DoubleArrayAvgFunctionFactory.java (1)
83-86: Handle division by zero when no finite values are found.When the array contains only non-finite values (NaN, Infinity), count will be 0, resulting in division by zero and returning NaN. While this may be intentional, it should be explicitly handled for clarity.
count = 0; sum = 0d; calculateRecursive(arr, 0, 0); -return sum / count; +return count > 0 ? sum / count : Double.NaN;core/src/main/java/io/questdb/griffin/engine/functions/array/DoubleArrayStdDevSampFunctionFactory.java (1)
115-119: Consider using epsilon for floating-point comparisonWhile setting negative variance to 0 handles precision issues, consider using an epsilon comparison for more robust handling:
-if (variance < 0d) { +if (variance < -1e-10) { + // Log warning - significant negative variance indicates numerical issues + variance = 0d; +} else if (variance < 0d) { variance = 0d; }core/src/test/java/io/questdb/test/griffin/engine/functions/array/DoubleArrayStdDevFunctionFactoryTest.java (1)
443-470: Consider adding test for large arraysWhile the random data tests are good, consider adding a test with a larger array to verify performance and numerical stability:
@Test public void testLargeArrayNumericalStability() throws SqlException { // Test with 10000 elements to verify numerical stability execute("CREATE TABLE large_array_test AS (" + "SELECT ARRAY_AGG(x) as arr FROM (" + " SELECT x::double FROM long_sequence(10000)" + "))"); // Verify the function completes without numerical issues assertSql( "count\n1\n", "SELECT count(*) FROM (SELECT array_stddev(arr) FROM large_array_test WHERE array_stddev(arr) IS NOT NULL)" ); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (36)
.gitignore(1 hunks)core/src/main/java/io/questdb/cairo/arr/ArrayView.java(3 hunks)core/src/main/java/io/questdb/cairo/arr/DirectArray.java(1 hunks)core/src/main/java/io/questdb/cairo/arr/FlatArrayView.java(1 hunks)core/src/main/java/io/questdb/griffin/engine/functions/array/DoubleArrayAddScalarFunctionFactory.java(2 hunks)core/src/main/java/io/questdb/griffin/engine/functions/array/DoubleArrayAndScalarDotProductFunctionFactory.java(3 hunks)core/src/main/java/io/questdb/griffin/engine/functions/array/DoubleArrayAndScalarDoubleArrayOperator.java(0 hunks)core/src/main/java/io/questdb/griffin/engine/functions/array/DoubleArrayAndScalarIntArrayOperator.java(0 hunks)core/src/main/java/io/questdb/griffin/engine/functions/array/DoubleArrayAvgFunctionFactory.java(2 hunks)core/src/main/java/io/questdb/griffin/engine/functions/array/DoubleArrayBinaryOperator.java(1 hunks)core/src/main/java/io/questdb/griffin/engine/functions/array/DoubleArrayCountFunctionFactory.java(3 hunks)core/src/main/java/io/questdb/griffin/engine/functions/array/DoubleArrayCumSumFunctionFactory.java(3 hunks)core/src/main/java/io/questdb/griffin/engine/functions/array/DoubleArrayDivScalarFunctionFactory.java(2 hunks)core/src/main/java/io/questdb/griffin/engine/functions/array/DoubleArrayDotProductFunctionFactory.java(1 hunks)core/src/main/java/io/questdb/griffin/engine/functions/array/DoubleArrayMaxFunctionFactory.java(1 hunks)core/src/main/java/io/questdb/griffin/engine/functions/array/DoubleArrayMinFunctionFactory.java(1 hunks)core/src/main/java/io/questdb/griffin/engine/functions/array/DoubleArrayMultiplyScalarFunctionFactory.java(2 hunks)core/src/main/java/io/questdb/griffin/engine/functions/array/DoubleArrayRoundFunctionFactory.java(8 hunks)core/src/main/java/io/questdb/griffin/engine/functions/array/DoubleArrayShiftDefaultNaNFunctionFactory.java(1 hunks)core/src/main/java/io/questdb/griffin/engine/functions/array/DoubleArrayShiftFunctionFactory.java(1 hunks)core/src/main/java/io/questdb/griffin/engine/functions/array/DoubleArrayStdDevFunctionFactory.java(1 hunks)core/src/main/java/io/questdb/griffin/engine/functions/array/DoubleArrayStdDevPopFunctionFactory.java(1 hunks)core/src/main/java/io/questdb/griffin/engine/functions/array/DoubleArrayStdDevSampFunctionFactory.java(1 hunks)core/src/main/java/io/questdb/griffin/engine/functions/array/DoubleArraySubtractScalarFunctionFactory.java(2 hunks)core/src/main/java/io/questdb/griffin/engine/functions/array/DoubleArraySumFunctionFactory.java(2 hunks)core/src/main/java/io/questdb/griffin/engine/functions/array/DoubleNegArrayFunctionFactory.java(3 hunks)core/src/main/java/io/questdb/griffin/engine/functions/array/DoubleScalarDivArrayFunctionFactory.java(2 hunks)core/src/main/java/io/questdb/griffin/engine/functions/array/DoubleScalarSubtractArrayFunctionFactory.java(2 hunks)core/src/main/java/io/questdb/griffin/engine/functions/array/DoubleUnaryArrayAccessor.java(0 hunks)core/src/main/java/io/questdb/griffin/engine/functions/constants/ArrayConstant.java(1 hunks)core/src/main/java/io/questdb/griffin/engine/functions/groupby/StdDevPopGroupByFunctionFactory.java(1 hunks)core/src/main/java/io/questdb/griffin/engine/functions/groupby/StdDevSampleGroupByFunctionFactory.java(1 hunks)core/src/test/java/io/questdb/test/griffin/engine/functions/array/DoubleArrayMaxFunctionFactoryTest.java(1 hunks)core/src/test/java/io/questdb/test/griffin/engine/functions/array/DoubleArrayMinFunctionFactoryTest.java(1 hunks)core/src/test/java/io/questdb/test/griffin/engine/functions/array/DoubleArrayRoundFunctionFactoryTest.java(0 hunks)core/src/test/java/io/questdb/test/griffin/engine/functions/array/DoubleArrayStdDevFunctionFactoryTest.java(1 hunks)
💤 Files with no reviewable changes (4)
- core/src/test/java/io/questdb/test/griffin/engine/functions/array/DoubleArrayRoundFunctionFactoryTest.java
- core/src/main/java/io/questdb/griffin/engine/functions/array/DoubleArrayAndScalarIntArrayOperator.java
- core/src/main/java/io/questdb/griffin/engine/functions/array/DoubleArrayAndScalarDoubleArrayOperator.java
- core/src/main/java/io/questdb/griffin/engine/functions/array/DoubleUnaryArrayAccessor.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/main/java/io/questdb/griffin/engine/functions/array/DoubleArrayDotProductFunctionFactory.javacore/src/main/java/io/questdb/griffin/engine/functions/array/DoubleArrayBinaryOperator.javacore/src/main/java/io/questdb/griffin/engine/functions/array/DoubleArrayShiftFunctionFactory.javacore/src/main/java/io/questdb/griffin/engine/functions/array/DoubleArrayShiftDefaultNaNFunctionFactory.javacore/src/main/java/io/questdb/griffin/engine/functions/constants/ArrayConstant.javacore/src/main/java/io/questdb/griffin/engine/functions/array/DoubleArrayStdDevFunctionFactory.javacore/src/main/java/io/questdb/cairo/arr/ArrayView.javacore/src/test/java/io/questdb/test/griffin/engine/functions/array/DoubleArrayMaxFunctionFactoryTest.javacore/src/main/java/io/questdb/cairo/arr/FlatArrayView.javacore/src/main/java/io/questdb/griffin/engine/functions/array/DoubleArrayCumSumFunctionFactory.javacore/src/main/java/io/questdb/griffin/engine/functions/array/DoubleArrayCountFunctionFactory.javacore/src/main/java/io/questdb/griffin/engine/functions/array/DoubleArrayStdDevPopFunctionFactory.javacore/src/main/java/io/questdb/griffin/engine/functions/array/DoubleArrayMinFunctionFactory.javacore/src/main/java/io/questdb/griffin/engine/functions/array/DoubleArrayAvgFunctionFactory.javacore/src/main/java/io/questdb/griffin/engine/functions/array/DoubleArrayMaxFunctionFactory.javacore/src/main/java/io/questdb/griffin/engine/functions/array/DoubleArraySumFunctionFactory.javacore/src/main/java/io/questdb/griffin/engine/functions/array/DoubleScalarDivArrayFunctionFactory.javacore/src/test/java/io/questdb/test/griffin/engine/functions/array/DoubleArrayMinFunctionFactoryTest.javacore/src/main/java/io/questdb/griffin/engine/functions/array/DoubleArrayDivScalarFunctionFactory.javacore/src/main/java/io/questdb/griffin/engine/functions/array/DoubleArrayMultiplyScalarFunctionFactory.javacore/src/main/java/io/questdb/griffin/engine/functions/array/DoubleArraySubtractScalarFunctionFactory.javacore/src/main/java/io/questdb/griffin/engine/functions/array/DoubleNegArrayFunctionFactory.javacore/src/main/java/io/questdb/griffin/engine/functions/array/DoubleArrayAndScalarDotProductFunctionFactory.javacore/src/main/java/io/questdb/griffin/engine/functions/array/DoubleArrayAddScalarFunctionFactory.javacore/src/main/java/io/questdb/griffin/engine/functions/array/DoubleScalarSubtractArrayFunctionFactory.javacore/src/main/java/io/questdb/griffin/engine/functions/array/DoubleArrayStdDevSampFunctionFactory.javacore/src/main/java/io/questdb/griffin/engine/functions/array/DoubleArrayRoundFunctionFactory.javacore/src/test/java/io/questdb/test/griffin/engine/functions/array/DoubleArrayStdDevFunctionFactoryTest.java
🔇 Additional comments (40)
core/src/main/java/io/questdb/cairo/arr/FlatArrayView.java (1)
196-224: LGTM! Well-implemented min/max aggregation methods.The implementation correctly handles edge cases:
- Uses
Numbers.isFinite()to filter out NaN and infinity values- Returns
Double.NaNwhen no finite values are found- Properly initializes with positive/negative infinity for comparison
- Consistent with the existing
avgDoubleandsumDoublepatternscore/src/main/java/io/questdb/griffin/engine/functions/array/DoubleArrayDotProductFunctionFactory.java (1)
93-93: LGTM! Method rename improves code clarity.The change from
!left.shapeEquals(right)toleft.shapeDiffers(right)makes the intent more explicit while maintaining the same semantic behavior - throwing an exception when array shapes differ.core/src/main/java/io/questdb/griffin/engine/functions/array/DoubleArrayShiftFunctionFactory.java (1)
143-143: LGTM! Clean refactoring consolidates array preparation.The change to use
array.prepare(getType(), arr)consolidates the type setting, shape copying, and memory allocation into a single method call, reducing code duplication and improving maintainability.core/src/main/java/io/questdb/griffin/engine/functions/array/DoubleArrayShiftDefaultNaNFunctionFactory.java (1)
85-85: LGTM! Consistent refactoring improves code uniformity.Same beneficial refactoring as seen in other array functions - consolidating array preparation into the
prepare()method improves consistency and maintainability across the codebase.core/src/main/java/io/questdb/griffin/engine/functions/array/DoubleArrayBinaryOperator.java (1)
80-87: LGTM! Method rename improves broadcasting logic clarity.The three changes from
shapeEqualstoshapeDiffersmake the broadcasting logic more explicit:
- Line 80: Triggers broadcasting when shapes differ
- Line 82: Broadcasts left array when it differs from output shape
- Line 87: Broadcasts right array when it differs from output shape
The semantic behavior remains identical while improving code readability.
core/src/main/java/io/questdb/cairo/arr/DirectArray.java (1)
112-125: LGTM! Well-designed consolidation method.The new
preparemethod effectively consolidates array preparation steps into a single, well-documented helper method. This reduces code duplication and provides a cleaner API for setting up arrays with a specific type and shape.core/src/main/java/io/questdb/griffin/engine/functions/constants/ArrayConstant.java (1)
52-53: LGTM! Clean refactoring using the new prepare method.The constructor now uses the consolidated
preparemethod instead of manual array setup steps, making the code more concise and consistent with other array function implementations in this PR.core/src/main/java/io/questdb/griffin/engine/functions/array/DoubleArrayStdDevFunctionFactory.java (1)
27-33: LGTM! Proper alias implementation for array_stddev.This class correctly implements
array_stddevas an alias forarray_stddev_sampby extending the sample standard deviation factory and only overriding the signature. This follows the established pattern for providing both population and sample standard deviation variants.core/src/main/java/io/questdb/griffin/engine/functions/groupby/StdDevPopGroupByFunctionFactory.java (1)
49-55: LGTM! Improved formatting for better readability.The multi-line parameter formatting improves readability of the method signature without changing functionality.
core/src/main/java/io/questdb/griffin/engine/functions/groupby/StdDevSampleGroupByFunctionFactory.java (1)
50-56: LGTM! Consistent formatting improvement.The multi-line parameter formatting matches the pattern used in
StdDevPopGroupByFunctionFactoryand improves readability without changing functionality.core/src/main/java/io/questdb/cairo/arr/ArrayView.java (3)
184-184: Good API improvement with the inverted logic.The change from
shapeEqualstoshapeDiffersimproves code readability by making early-exit conditions more intuitive. This is a positive refactoring that aligns with common programming patterns.
408-414: Useful accessor methods for flat view boundaries.The addition of
getHi()andgetLo()provides clean access to the flat view range boundaries. This improves API usability and reduces the need for manual offset calculations in client code.
517-534: Method correctly implements shape difference logic.The implementation properly checks for dimension count mismatch first, then iterates through dimensions to compare lengths. The inverted return values (true for differences) are correct and consistent with the method name.
core/src/test/java/io/questdb/test/griffin/engine/functions/array/DoubleArrayMaxFunctionFactoryTest.java (1)
31-133: Comprehensive test coverage for array_max function.The test class provides excellent coverage including:
- Edge cases (empty, null, single element)
- Multi-dimensional arrays
- Special double values (NaN, ±Infinity)
- Negative values
- Table data integration
The tests correctly verify that infinite and NaN values are ignored in the maximum calculation, which is an important behavior to document.
core/src/test/java/io/questdb/test/griffin/engine/functions/array/DoubleArrayMinFunctionFactoryTest.java (1)
31-133: Well-structured test coverage mirroring array_max tests.The test class provides comprehensive coverage for array_min with the same scenarios as array_max, ensuring consistent behavior across both functions. The tests correctly verify that infinite and NaN values are ignored when computing the minimum.
core/src/main/java/io/questdb/griffin/engine/functions/array/DoubleArrayMinFunctionFactory.java (2)
74-84: Efficient implementation with proper edge case handling.The implementation correctly:
- Returns NaN for null arrays
- Uses a fast path for vanilla (flat) arrays
- Handles multi-dimensional arrays via recursion
- Filters out non-finite values (NaN, ±Infinity)
- Returns NaN when no finite values exist
The use of
Double.POSITIVE_INFINITYas the initial min value is appropriate.
113-115: Thread safety correctly documented.The function is appropriately marked as not thread-safe due to the mutable
minfield used during evaluation.core/src/main/java/io/questdb/griffin/engine/functions/array/DoubleArrayCountFunctionFactory.java (2)
53-78: Clean refactoring to explicit recursive implementation.The refactoring from interface-based callbacks to explicit implementation improves code clarity. The implementation correctly:
- Returns 0 for null arrays
- Uses fast path for vanilla arrays via
flatView.countDouble()- Handles multi-dimensional arrays recursively
- Counts only finite values (excludes NaN and ±Infinity)
90-108: Well-structured recursive traversal.The recursive method properly handles multi-dimensional arrays by:
- Checking if at the deepest dimension before processing values
- Using stride-based iteration for efficient traversal
- Filtering non-finite values consistently with other array functions
core/src/main/java/io/questdb/griffin/engine/functions/array/DoubleArrayMultiplyScalarFunctionFactory.java (1)
127-142: LGTM!The recursive traversal correctly uses
flatIndexfor array element access, properly handling multi-dimensional arrays.core/src/main/java/io/questdb/griffin/engine/functions/array/DoubleArrayMaxFunctionFactory.java (1)
59-116: LGTM!The implementation correctly handles:
- Null arrays returning NaN
- Fast path for vanilla arrays using
flatView().maxDouble()- Recursive traversal for multi-dimensional arrays
- Proper handling of non-finite values (NaN, Infinity)
- Thread safety correctly marked as false due to mutable state
core/src/main/java/io/questdb/griffin/engine/functions/array/DoubleArraySumFunctionFactory.java (1)
100-124: Excellent numerical stability implementation!The recursive calculation correctly implements Kahan summation algorithm for improved numerical accuracy when summing floating-point values. The initialization logic properly handles the transition from NaN to 0 when the first finite value is encountered.
core/src/main/java/io/questdb/griffin/engine/functions/array/DoubleArraySubtractScalarFunctionFactory.java (1)
56-139: LGTM!The refactored implementation correctly:
- Handles null arrays by returning a null array wrapper
- Provides fast path optimization for vanilla (flat) arrays
- Implements recursive traversal for multi-dimensional arrays with correct flatIndex usage
- Properly manages resources with the close() method
- Maintains consistency with other refactored array-scalar operators
core/src/main/java/io/questdb/griffin/engine/functions/array/DoubleArrayDivScalarFunctionFactory.java (1)
123-138: Well-implemented recursive array traversalThe recursive method correctly handles multi-dimensional arrays by:
- Traversing dimensions using stride
- Applying the operation only at the deepest dimension
- Maintaining the flat index throughout recursion
core/src/main/java/io/questdb/griffin/engine/functions/array/DoubleArrayStdDevSampFunctionFactory.java (2)
96-101: Excellent use of Welford's algorithm for numerical stabilityThe implementation correctly uses Welford's online algorithm which provides better numerical stability compared to the naive approach of calculating variance. This prevents catastrophic cancellation when dealing with large numbers.
155-157: Correct implementation of Bessel's correctionThe method properly returns
count - 1for sample variance calculation, which is the correct denominator for unbiased sample standard deviation.core/src/main/java/io/questdb/griffin/engine/functions/array/DoubleScalarSubtractArrayFunctionFactory.java (1)
56-141: Consistent implementation with other array operatorsThe refactored implementation correctly:
- Handles null arrays by returning null
- Processes vanilla arrays efficiently with direct iteration
- Handles multi-dimensional arrays with recursive traversal
- Properly manages resources with close() method
core/src/main/java/io/questdb/griffin/engine/functions/array/DoubleArrayAddScalarFunctionFactory.java (1)
56-60: Correct handling of commutative operationThe
shouldSwapArgs()method correctly returnstrue, allowing the SQL engine to swap arguments for this commutative operation (array + scalar = scalar + array).core/src/test/java/io/questdb/test/griffin/engine/functions/array/DoubleArrayStdDevFunctionFactoryTest.java (1)
31-496: Excellent comprehensive test coverageThe test suite thoroughly covers:
- Edge cases (null, empty, single element arrays)
- Multi-dimensional arrays and slices
- Special floating-point values (NaN, Infinity)
- Both sample and population standard deviation
- Integration with GROUP BY aggregation
- Numerical accuracy with precise expected values
core/src/main/java/io/questdb/griffin/engine/functions/array/DoubleArrayRoundFunctionFactory.java (11)
37-37: Import addition aligns with architectural refactoring.The addition of
BinaryFunctionimport is consistent with the refactoring whereDoubleArrayRoundVarScaleFunctionnow implementsBinaryFunctiondirectly instead of using interface-based callbacks.
76-76: Class declaration correctly implements UnaryFunction.The class now extends
ArrayFunctionand implementsUnaryFunction, which is consistent with the architectural shift away from interface-based element processing to explicit function interfaces.
108-112: Simplified array preparation and processing logic.The refactored code uses the new
DirectArray.prepare()method to consolidate array setup, then processes elements with a simple loop. This is cleaner than the previous interface-based approach.
131-131: Class declaration correctly implements UnaryFunction.Similar to the degenerate scale function, this class now properly implements
UnaryFunctionas part of the architectural refactoring.
165-174: Enhanced array processing with multi-dimensional support.The refactored implementation now handles both vanilla (flat) and multi-dimensional arrays explicitly. The vanilla case uses direct flat view access, while complex arrays use recursive traversal.
192-207: Recursive traversal implementation looks correct.The recursive method properly handles multi-dimensional arrays by:
- Calculating stride and count for each dimension
- Detecting the deepest dimension for actual value processing
- Recursively traversing intermediate dimensions
- Applying the rounding function at leaf elements
217-231: Variable scale function correctly implements BinaryFunction.The class declaration and constructor properly handle the transition to
BinaryFunctioninterface, maintaining both array and scalar arguments as expected for a binary operation.
234-237: Resource management correctly updated.The
close()method now callsBinaryFunction.super.close()instead of the previous interface method, which is correct for the new architecture.
240-258: Array processing logic correctly handles variable scale.The implementation properly:
- Retrieves the scalar value from the record
- Uses the new
DirectArray.prepare()method- Handles both vanilla and multi-dimensional arrays
- Passes the scalar value to the rounding function
260-283: BinaryFunction interface methods correctly implemented.All required
BinaryFunctioninterface methods (getLeft(),getRight(),getName(),isOperator(),isThreadSafe()) are properly implemented with appropriate return values.
285-300: Recursive traversal for variable scale is consistent.The recursive method implementation matches the pattern used in the positive scale function, correctly handling multi-dimensional arrays and passing the scalar value to the rounding function at leaf elements.
...main/java/io/questdb/griffin/engine/functions/array/DoubleArrayDivScalarFunctionFactory.java
Outdated
Show resolved
Hide resolved
...main/java/io/questdb/griffin/engine/functions/array/DoubleScalarDivArrayFunctionFactory.java
Outdated
Show resolved
Hide resolved
…tddev # Conflicts: # core/src/main/java/io/questdb/griffin/engine/functions/array/DoubleArrayAddScalarFunctionFactory.java # core/src/main/java/io/questdb/griffin/engine/functions/array/DoubleArrayDivScalarFunctionFactory.java # core/src/main/java/io/questdb/griffin/engine/functions/array/DoubleArrayMultiplyScalarFunctionFactory.java # core/src/main/java/io/questdb/griffin/engine/functions/array/DoubleArrayRoundFunctionFactory.java # core/src/main/java/io/questdb/griffin/engine/functions/array/DoubleArraySubtractScalarFunctionFactory.java # core/src/main/java/io/questdb/griffin/engine/functions/array/DoubleNegArrayFunctionFactory.java # core/src/main/java/io/questdb/griffin/engine/functions/array/DoubleScalarDivArrayFunctionFactory.java # core/src/main/java/io/questdb/griffin/engine/functions/array/DoubleScalarSubtractArrayFunctionFactory.java
[PR Coverage check]😍 pass : 726 / 778 (93.32%) file detail
|
4 participants
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Tests
Chores
.gitignoreto exclude specific files.