Conversation
Reached much better compliance with the latest changes:
```
=======================================================
OpenCypher TCK Compliance Report
=======================================================
Total scenarios: 3897
Passed: 1908 (48%)
Failed: 1939 (49%)
Skipped: 50 (1%)
-------------------------------------------------------
By category:
clauses/call 2/ 52 passed (3%)
clauses/create 64/ 78 passed (82%)
clauses/delete 24/ 41 passed (58%)
clauses/match 292/381 passed (76%)
clauses/match-where 25/ 34 passed (73%)
clauses/merge 47/ 75 passed (62%)
clauses/remove 29/ 33 passed (87%)
clauses/return 35/ 63 passed (55%)
clauses/return-orderby 23/ 35 passed (65%)
clauses/return-skip-limit 26/ 31 passed (83%)
clauses/set 30/ 53 passed (56%)
clauses/union 8/ 12 passed (66%)
clauses/unwind 10/ 14 passed (71%)
clauses/with 14/ 29 passed (48%)
clauses/with-orderBy 124/292 passed (42%)
clauses/with-skip-limit 7/ 9 passed (77%)
clauses/with-where 10/ 19 passed (52%)
expressions/aggregation 23/ 35 passed (65%)
expressions/boolean 150/150 passed (100%)
expressions/comparison 36/ 72 passed (50%)
expressions/conditional 13/ 13 passed (100%)
expressions/existentialSubqueries 4/ 10 passed (40%)
expressions/graph 32/ 61 passed (52%)
expressions/list 120/185 passed (64%)
expressions/literals 120/131 passed (91%)
expressions/map 28/ 44 passed (63%)
expressions/mathematical 3/ 6 passed (50%)
expressions/null 44/ 44 passed (100%)
expressions/path 0/ 7 passed (0%)
expressions/pattern 19/ 50 passed (38%)
expressions/precedence 20/121 passed (16%)
expressions/quantifier 478/604 passed (79%)
expressions/string 22/ 32 passed (68%)
expressions/temporal 0/1004 passed (0%)
expressions/typeConversion 19/ 47 passed (40%)
useCases/countingSubgraphMatches 6/ 11 passed (54%)
useCases/triadicSelection 1/ 19 passed (5%)
=======================================================
```
Summary of ChangesHello @lvca, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly advances the OpenCypher temporal functionality, bringing it closer to 100% TCK completion. It addresses several critical aspects of temporal data handling, including precise arithmetic with durations, correct parsing of complex expressions, and robust duration normalization. Additionally, it improves the query optimizer's decision-making for unsupported Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request introduces several changes to complete the temporal OpenCypher TCK, enhancing temporal arithmetic, duration multiplication, and parsing logic. The changes appear to correctly address issues required for TCK compliance. My review focuses on improving code clarity, maintainability, and robustness. I've suggested replacing magic numbers with named constants, refactoring duplicated code into a helper method, and strengthening a fragile heuristic in the expression parser. Additionally, I've proposed a simplification for a complex method to improve its readability.
| if (listCtx != null) { | ||
| final String listText = listCtx.getText(); | ||
| final String exprText = ctx.getText(); | ||
| if (listText.length() >= exprText.length() - 2) // Top-level list (allow for whitespace) |
There was a problem hiding this comment.
The heuristic listText.length() >= exprText.length() - 2 to detect a top-level list is fragile as it may fail with varying amounts of whitespace. A more robust approach would be to trim the expression's text and perform a string comparison.
| if (listText.length() >= exprText.length() - 2) // Top-level list (allow for whitespace) | |
| if (listText.equals(exprText.trim())) // Top-level list |
| // java.time.LocalDate (from ArcadeDB storage) ± Duration | ||
| if (leftValue instanceof LocalDate ld && rightValue instanceof CypherDuration dur) { | ||
| // For dates, full days from the seconds component must be carried over | ||
| final long extraDays = dur.getSeconds() / 86400; |
There was a problem hiding this comment.
The magic number 86400 for seconds in a day makes the code harder to understand. Consider using java.time.Duration for this conversion to improve readability and type safety.
| final long extraDays = dur.getSeconds() / 86400; | |
| final long extraDays = java.time.Duration.ofSeconds(dur.getSeconds()).toDays(); |
| // For dates, full days from the seconds component must be carried over | ||
| final long extraDays = dur.getSeconds() / 86400; | ||
| final long totalDays = dur.getDays() + extraDays; | ||
| final LocalDate d = cd.getValue().plusMonths(op == Operator.ADD ? dur.getMonths() : -dur.getMonths()) | ||
| .plusDays(op == Operator.ADD ? dur.getDays() : -dur.getDays()); | ||
| .plusDays(op == Operator.ADD ? totalDays : -totalDays); |
There was a problem hiding this comment.
This block of code is nearly identical to lines 197-201, which handles LocalDate. To avoid code duplication and improve maintainability, consider extracting this logic for applying a CypherDuration to a LocalDate into a private helper method.
| final long wholeMonths = (long) newMonths; | ||
| final double monthRemainder = newMonths - wholeMonths; | ||
|
|
||
| final double newDays = days * factor + monthRemainder * (365.2425 / 12); |
There was a problem hiding this comment.
The magic number 365.2425 / 12 represents the average number of days in a month. It would be clearer to define this as a named constant, e.g., private static final double AVG_DAYS_PER_MONTH = 365.2425 / 12.0;. This value is also used in the fromComponents method, so a constant would improve consistency.
| final long wholeDays = (long) newDays; | ||
| final double dayRemainder = newDays - wholeDays; | ||
|
|
||
| final double totalNanos = (seconds * 1_000_000_000.0 + nanosAdjustment) * factor + dayRemainder * 86400_000_000_000.0; |
There was a problem hiding this comment.
The magic numbers for nanoseconds (1_000_000_000.0 and 86400_000_000_000.0) make this line difficult to read. Consider defining constants like NANOS_PER_SECOND and NANOS_PER_DAY to improve clarity and maintainability.
| int nanos = 0; | ||
| if (hasMs) | ||
| nanos += ((Number) map.get("millisecond")).intValue() * 1_000_000; | ||
| else | ||
| nanos += (defaultNanos / 1_000_000) * 1_000_000; | ||
| if (hasUs) | ||
| nanos += ((Number) map.get("microsecond")).intValue() * 1_000; | ||
| else | ||
| nanos += ((defaultNanos % 1_000_000) / 1_000) * 1_000; | ||
| if (hasNs) | ||
| nanos += ((Number) map.get("nanosecond")).intValue(); | ||
| else | ||
| nanos += defaultNanos % 1_000; | ||
| return nanos; |
There was a problem hiding this comment.
The logic for computing nanoseconds is verbose and repetitive. It can be simplified for better readability by calculating each component (milli, micro, nano) based on whether it's present in the map or should be taken from the default, and then combining them at the end.
For example:
final int NANOS_PER_MILLI = 1_000_000;
final int NANOS_PER_MICRO = 1_000;
int milli = hasMs ? ((Number) map.get("millisecond")).intValue() : defaultNanos / NANOS_PER_MILLI;
int micro = hasUs ? ((Number) map.get("microsecond")).intValue() : (defaultNanos % NANOS_PER_MILLI) / NANOS_PER_MICRO;
int nano = hasNs ? ((Number) map.get("nanosecond")).intValue() : defaultNanos % NANOS_PER_MICRO;
return milli * NANOS_PER_MILLI + micro * NANOS_PER_MICRO + nano;
🧪 CI InsightsHere's what we observed from your CI run for 9d5bf37. 🟢 All jobs passed!But CI Insights is watching 👀 |
|
Code Review: Completed 100% temporal OpenCypher TCK Great work achieving 100% compliance with the OpenCypher TCK temporal tests! Overall Assessment: Approve with minor suggestions The changes successfully fix temporal handling in OpenCypher to achieve full TCK compliance. Code quality is generally good with clear comments explaining complex logic. Detailed Findings:
Testing: No new test files in PR (only TCK compliance achieved) Security: No issues identified Code Style: Follows project standards well Action Items BEFORE MERGING:
Future improvements: overflow protection, extract magic constants Summary: Excellent work! Main concern is the potential bug in TemporalUtil line 339. Congratulations on 100% TCK temporal compliance! |
Coverage summary from CodacySee diff coverage on Codacy
Coverage variation details
Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: Diff coverage details
Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: See your quality gate settings Change summary preferences |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #3369 +/- ##
==========================================
+ Coverage 60.59% 60.82% +0.23%
==========================================
Files 1177 1177
Lines 80848 80874 +26
Branches 16231 16235 +4
==========================================
+ Hits 48988 49193 +205
+ Misses 24865 24713 -152
+ Partials 6995 6968 -27 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
* test: including official opencypher tck
* fix: fixed many issues with opencypher from TCK tests
* fix: fixed broken tests from OpenCyphr TCK
* fix: more opencypher issues from tck tests
* fix: opencypher tck more tests pass now
Reached much better compliance with the latest changes:
```
=======================================================
OpenCypher TCK Compliance Report
=======================================================
Total scenarios: 3897
Passed: 1908 (48%)
Failed: 1939 (49%)
Skipped: 50 (1%)
-------------------------------------------------------
By category:
clauses/call 2/ 52 passed (3%)
clauses/create 64/ 78 passed (82%)
clauses/delete 24/ 41 passed (58%)
clauses/match 292/381 passed (76%)
clauses/match-where 25/ 34 passed (73%)
clauses/merge 47/ 75 passed (62%)
clauses/remove 29/ 33 passed (87%)
clauses/return 35/ 63 passed (55%)
clauses/return-orderby 23/ 35 passed (65%)
clauses/return-skip-limit 26/ 31 passed (83%)
clauses/set 30/ 53 passed (56%)
clauses/union 8/ 12 passed (66%)
clauses/unwind 10/ 14 passed (71%)
clauses/with 14/ 29 passed (48%)
clauses/with-orderBy 124/292 passed (42%)
clauses/with-skip-limit 7/ 9 passed (77%)
clauses/with-where 10/ 19 passed (52%)
expressions/aggregation 23/ 35 passed (65%)
expressions/boolean 150/150 passed (100%)
expressions/comparison 36/ 72 passed (50%)
expressions/conditional 13/ 13 passed (100%)
expressions/existentialSubqueries 4/ 10 passed (40%)
expressions/graph 32/ 61 passed (52%)
expressions/list 120/185 passed (64%)
expressions/literals 120/131 passed (91%)
expressions/map 28/ 44 passed (63%)
expressions/mathematical 3/ 6 passed (50%)
expressions/null 44/ 44 passed (100%)
expressions/path 0/ 7 passed (0%)
expressions/pattern 19/ 50 passed (38%)
expressions/precedence 20/121 passed (16%)
expressions/quantifier 478/604 passed (79%)
expressions/string 22/ 32 passed (68%)
expressions/temporal 0/1004 passed (0%)
expressions/typeConversion 19/ 47 passed (40%)
useCases/countingSubgraphMatches 6/ 11 passed (54%)
useCases/triadicSelection 1/ 19 passed (5%)
=======================================================
```
* fix: opencypher implemented missing temporal functions + precedence
Issue #3357 and #3355
* fix: opencypher implemented more missing functions
Issue #3357 and #3355
* fix: opencypher more code fixed thank to the tck
* fix: OpenCypher fixed temporal functions (from TCK results)
* fix: OpenCypher completed 100% of temporal tests (!)
Issue #3357
(cherry picked from commit a4f11f1)
No description provided.