Enable tests to run in parallel by default#3895
Enable tests to run in parallel by default#3895ExtReMLapin wants to merge 45 commits intoArcadeData:mainfrom
Conversation
This reverts commit be84308.
…penCypherAggregatingFunctionsComprehensiveTest.java Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
…penCypherAggregatingFunctionsComprehensiveTest.java Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
…tion.java Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Code Review
This pull request significantly expands OpenCypher function support, adding geospatial, temporal, vector, and string functions along with GQL-conformant aliases. It also improves test parallelization by updating the TestHelper and implementing resource locks across the test suite. Feedback focuses on fixing a potential NumberFormatException for empty vector strings, ensuring split() preserves trailing empty strings per Cypher specifications, restoring robustness in point() coordinate parsing, and handling potential nulls or incorrect types in map-based distance calculations.
| final String inner = trimmed.startsWith("[") && trimmed.endsWith("]") ? trimmed.substring(1, trimmed.length() - 1) : trimmed; | ||
| final String[] parts = inner.split(","); | ||
| final float[] result = new float[parts.length]; |
There was a problem hiding this comment.
The current parsing logic will throw a NumberFormatException if the input string represents an empty array []. When inner is an empty string, split(",") returns an array containing one empty string, which Float.parseFloat() cannot handle. Additionally, it doesn't handle cases with extra commas or whitespace between values gracefully.
| final String inner = trimmed.startsWith("[") && trimmed.endsWith("]") ? trimmed.substring(1, trimmed.length() - 1) : trimmed; | |
| final String[] parts = inner.split(","); | |
| final float[] result = new float[parts.length]; | |
| final String inner = trimmed.startsWith("[") && trimmed.endsWith("]") ? trimmed.substring(1, trimmed.length() - 1).trim() : trimmed; | |
| if (inner.isEmpty()) | |
| return new float[0]; | |
| final String[] parts = inner.split(","); | |
| final float[] result = new float[parts.length]; |
| return null; | ||
| final String str = args[0].toString(); | ||
| final String delimiter = args[1].toString(); | ||
| return List.of(str.split(Pattern.quote(delimiter))); |
There was a problem hiding this comment.
Java's String.split(regex) discards trailing empty strings by default. To comply with the Cypher specification, which expects trailing empty strings to be preserved (e.g., split('a,b,', ',') should return ['a', 'b', '']), you should use the two-argument version of split with a negative limit.
| return List.of(str.split(Pattern.quote(delimiter))); | |
| return List.of(str.split(Pattern.quote(delimiter), -1)); |
| final double lat = ((Number) args[0]).doubleValue(); | ||
| final double lon = ((Number) args[1]).doubleValue(); |
There was a problem hiding this comment.
This change reduces robustness compared to the previous implementation. The previous code used GeoUtils.getDoubleValue(), which safely handles both Number and String inputs. The direct cast to Number will throw a ClassCastException if the input is a string representation of a coordinate.
| final double lat = ((Number) args[0]).doubleValue(); | |
| final double lon = ((Number) args[1]).doubleValue(); | |
| final double lat = com.arcadedb.function.sql.geo.GeoUtils.getDoubleValue(args[0]); | |
| final double lon = com.arcadedb.function.sql.geo.GeoUtils.getDoubleValue(args[1]); |
| ((Number) p1.get("latitude")).doubleValue(), ((Number) p1.get("longitude")).doubleValue(), | ||
| ((Number) p2.get("latitude")).doubleValue(), ((Number) p2.get("longitude")).doubleValue()); |
There was a problem hiding this comment.
This code is unsafe as it assumes the map values are non-null and of type Number. If a coordinate is null in the map (which is possible in Cypher), this will throw a NullPointerException. If it's a String, it will throw a ClassCastException. It is safer to use GeoUtils.getDoubleValue() after verifying the values are not null.
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 68 |
TIP This summary will be updated as you push new changes. Give us feedback
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #3895 +/- ##
============================================
- Coverage 64.71% 56.53% -8.19%
- Complexity 0 1572 +1572
============================================
Files 1581 1585 +4
Lines 117023 117508 +485
Branches 24858 24957 +99
============================================
- Hits 75730 66431 -9299
- Misses 30933 41743 +10810
+ Partials 10360 9334 -1026 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
To be merged after #3428