fix(tesseract): Fix rolling window with few time dimensions, filter_group in segments and member expressions #9673
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #9673 +/- ##
=======================================
Coverage 84.17% 84.17%
=======================================
Files 230 230
Lines 85112 85112
=======================================
Hits 71647 71647
Misses 13465 13465
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| let query = SimpleQuery { | ||
| schema, | ||
| filter: logical_filter, | ||
| offset: self.query_properties.offset(), |
There was a problem hiding this comment.
Nice catch there. I noticed the offset is still present here and wanted to share some thoughts (though I'm still familiarizing myself with the codebase, so please correct me if I'm missing something).
From what I understand, there might be a few considerations regarding offset in subqueries:
- Database compatibility: Different databases have varying requirements for offset usage:
MSSQL requires an ORDER BY clause when using OFFSET
MySQL requires OFFSET to be paired with a LIMIT clause - Pagination concerns: I'm wondering if pagination logic should be restricted to the outer query only? My understanding is that applying pagination to inner queries could potentially lead to data inconsistencies, especially when the API request includes result pagination.
Would it make sense to remove the offset from this subquery as well? I'd appreciate your thoughts on this, as you likely have more context about the intended behavior here.
There was a problem hiding this comment.
@UsmanYasin Thank you so much for catching that! The offset really isn’t needed here — that was my mistake, and I’ve removed it.
More broadly, I think this part could use a small refactor to prevent both limit and offset from being set at all in this context. I’ve been thinking about that, and as I find time, I plan to gradually refactor things to (hopefully) improve the architecture and overall clarity.
| if (getEnv('nativeSqlPlanner')) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
Can you move this to the upper scope and mark test(s) as skipped instead of making it pass while it does nothing?
| } else { | ||
| false | ||
| } | ||
| pub fn can_used_as_addictive_in_multplied(&self) -> bool { |
There was a problem hiding this comment.
| pub fn can_used_as_addictive_in_multplied(&self) -> bool { | |
| pub fn can_be_used_as_additive_in_multplied(&self) -> bool { |
There was a problem hiding this comment.
Measures are not about drugs :)
| } | ||
| pub fn can_used_as_addictive_in_multplied(&self) -> bool { | ||
| if let Ok(measure_symbol) = self.member_evaluator.as_measure() { | ||
| measure_symbol.can_used_as_addictive_in_multplied() |
There was a problem hiding this comment.
Would be nice to rename this too:
| measure_symbol.can_used_as_addictive_in_multplied() | |
| measure_symbol.can_be_used_as_additive_in_multplied() |
waralexrom
changed the title
waralexrom
changed the title
…roup in segments and member expressions (cube-js#9673)
Check List