-
Notifications
You must be signed in to change notification settings - Fork 21
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
[O2B-1365] Implement GAQ periods views #1808
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1808 +/- ##
==========================================
+ Coverage 43.78% 43.81% +0.03%
==========================================
Files 893 893
Lines 15954 15968 +14
Branches 3003 3015 +12
==========================================
+ Hits 6985 6997 +12
- Misses 8969 8971 +2 ☔ View full report in Codecov by Sentry. |
( | ||
SELECT gaqd.data_pass_id, | ||
gaqd.run_number, | ||
COALESCE(UNIX_TIMESTAMP(first_tf_timestamp), UNIX_TIMESTAMP(time_start), 0) AS timestamp |
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.
From what I see as usage, you almost always re-convert the 0 and NOW to null.
Moreover, I am not sure when using the view that NOW(3)
will always return the same value as .to
that has been coalesced to NOW(3)
inside the view, because of timing issue.
Don't you think we can do something like select COALESCE(UNIX_TIMESTAMP(first_tf_timestamp), UNIX_TIMESTAMP(time_start)) AS timestamp,
COALESCE(UNIX_TIMESTAMP(first_tf_timestamp), UNIX_TIMESTAMP(time_start), 0) AS order` and sort by order?
Then you can directly use timestamp without further if to make it null again
COALESCE(gaq_periods.\`to\` , 0) | ||
+ COALESCE(gaq_periods.\`from\`, 0) |
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.
I created a PR that aims to simplify a bit the readability of these, if you can have a look and bring here what can be (for example the changes in this if): https://github.com/AliceO2Group/Bookkeeping/pull/1802/files#diff-a63ab748af8aa9d059b804a24f2e444657dbaf1848679c1a5eb11f4bccac752aR269
effectiveRunCoverage, | ||
mcReproducible, | ||
flagsList, | ||
verifiedFlagsList, | ||
}) => ({ | ||
runNumber, | ||
bad, | ||
bad: mcReproducibleAsNotBad ? badWhenMcReproducibleAsNotBad : bad, |
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.
Why is that change here?
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.
in the stored view the particular column cannot be parametrised with mcReproducibleAsNotBad
flag (I would need to use sth. else than stored views), so I calculate two separate columns bad
and badWhenMcReproducibleAsNotBad
effectiveRunCoverage: parseFloat(effectiveRunCoverage) || null, | ||
mcReproducible: Boolean(mcReproducible), | ||
flagsIds: [...new Set(flagsList.split(','))], | ||
flagsIds: flagsList ? [...new Set(flagsList.split(','))] : [], |
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.
Why is that change here?
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.
now the view my return GAQ eff. periods
which doesn't have any flags in, if so, this property is null
, so need to handle it like that
I have a JIRA ticket
Notable changes for users:
Notable changes for developers:
Changes made to the database: