-
Notifications
You must be signed in to change notification settings - Fork 29
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
(#Closes 2875) Implementation to handle elemental functions in wheres #2883
base: master
Are you sure you want to change the base?
Conversation
Gonna send this for integration |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #2883 +/- ##
=======================================
Coverage 99.89% 99.89%
=======================================
Files 359 359
Lines 50997 51069 +72
=======================================
+ Hits 50942 51017 +75
+ Misses 55 52 -3 ☔ View full report in Codecov by Sentry. |
Ready for review assuming integration is ok @sergisiso @arporter |
Integration passed successfully. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this fix @LonelyCat124, as I mentioned in my in-line comments it is not obvious to me why this should work, but I suggest starting with my comments requesting additional tests as I will be ok with it if it is proven to work.
@sergisiso Comments should be addressed - had to reorder a couple of statments and add an additional test to directly test some code that is currently unreachable, but may be necessary to handle future improvements. I will also rerun the integration tests as there are more impvoements here. |
Previous branch had some deviation on nemo 5, will resubmit |
Pushed the wrong branch to integration last night, set the right one going today. |
Ah, the detail I has missing was that we don't support Imports or Unresolved anyway and therefore we don't have to worry about unknown 'elemental' attribute, it is much clearer now. I will wait until integration tests are done to finish the review. |
Integration is now passing. |
NEMOv5 CPU parallelisation was slower, but there were lots of people heavily using the machine that day and this is a threaded run, so it is expected that it was slower. But since this is one of our current targets, I want to re-run only this action when the machine is empty today or over the weekend. |
Improves the implementation of wheres to do better with elemental functions. Its a relatively small change that should work out the box....