fix(time_offset): improved LIMIT-handling in advanced analytics #27934
Conversation
Antonio-RiveroMartnez
requested review from
betodealmeida,
zhaoyongjie and
eschutho
- Remove inner row_limit and row_offset when applying time_offset to prevent data inconsistency
Antonio-RiveroMartnez
force-pushed
the
fix_time_offset
branch
from
59c830e to
dce9d38
Compare
|
/testenv up |
|
@Antonio-RiveroMartnez Ephemeral environment spinning up at http://34.215.85.155:8080. Credentials are |
- Use config value as row_limit - Update test
|
/testenv up |
|
@Antonio-RiveroMartnez Ephemeral environment spinning up at http://54.244.23.72:8080. Credentials are |
|
@Antonio-RiveroMartnez are there any open issues we can link to this? I'm somewhat surprised that no one has reported an issue with this in the past. |
|
Hey @john-bodley I just linked it in the PR description, thanks. |
mistercrunch
changed the title
| @@ -455,6 +455,13 @@ def processing_time_offsets( # pylint: disable=too-many-locals,too-many-stateme | |||
| for metric in metric_names | |||
| } | |||
|
|
|||
| # When the original query has limit or offset we wont apply those | |||
There was a problem hiding this comment.
Wow this method has gotten large! Thinking about refactors here and factoring out a processing_time_offset (a method for each time offset) might help a little in reading this, but probably breaking down further would be good too.
Doing the refactor in this PR as part of this PR would make it harder to spot the diff, so I'll approve this one, but hoping we can refactor some of these into more bite-sized methods. Maybe a goal could be to remove the disable=too-many-locals,too-many-statements above.
|
Ephemeral environment shutdown and build artifacts deleted. |
michael-s-molina
added
the
v4.0
(cherry picked from commit 6844735)
(cherry picked from commit 6844735)
(cherry picked from commit 6844735)
(cherry picked from commit 6844735)
mistercrunch
added
🍒 4.0.1
🍒 4.0.2
🏷️ bot
SUMMARY
When applying a time_offset in Advanced Analytics, if the row_limit is too small, data will not be present when the join occurs, so this PR prevent such behavior by not using limit nor offset in the inner query (the shifted one).
For example this query shown in the following video has all the records for a period of 10+ years:
Screen.Recording.2024-04-08.at.11.04.50.AM.mov
We can see that the result of shifted queries not always include the same periods than the original query, so when the join gets applied, it won't match some records, thus giving you incorrect output.
Original query with limit and sorting:
1 year ago query with limit and sorting:
2 years ago query with limit and sorting:
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Error:
In the following image some datapoints are missing in the shifted columns because they are not part of the result dataframe after sorting and limiting it like the original query.
Fix:
Now we can see that the query is able to find the records that belong to those offsets regardless, because it's joining using the complete data.
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION