feat: Speed up problem results by combining responses into one CTE #35
Conversation
|
@SoryRawyer what do you think of this? If it's good I think we can do the same here: https://github.com/openedx/tutor-contrib-aspects/blob/4ea65d69307b53e5da96cedb46296c785fe3a259/tutoraspects/templates/openedx-assets/queries/fact_learner_problem_summary.sql#L1 |
|
@SoryRawyer this is out of date, but I think we should still look at making these changes anywhere we're doing multiple CTEs and can combine them. It was a pretty marked performance difference. |
|
Ah sorry for letting this get past me, I must have thought this was shelved for a while. At first pass it looks good but I'll take a deeper look. |
|
@bmtcril did Altinity say anything about using window functions in views? I might be missing something, but I'm not sure that was ever resolved. That said, I think this is a good strategy for some of the more complicated Superset datasets like the one you linked to. |
| if( | ||
| was_successful, | ||
| minIf(emission_time, was_successful), | ||
| maxIf(emission_time, not was_successful) |
There was a problem hiding this comment.
Is this double if necessary?
| if( | |
| was_successful, | |
| minIf(emission_time, was_successful), | |
| maxIf(emission_time, not was_successful) | |
| if( | |
| was_successful, | |
| min(emission_time), | |
| max(emission_time) |
This change was based on a recommendation from Altinity, tests showed about a 30% speedup but I'm not 100% sure it's correct on a larger dataset so putting it up for comment while I test.