Only load symbols data if pattern requires them. #791
Conversation
|
@gregtatum - not flagging you for review, because you're on PTO, but pinging you so that you can skim through when you're back. |
|
This fixes #603 for 0.3. I'm aware of #380 and I don't think it's in conflict with #380 long term - the Additionally, as we further split symbols, we will use this function to identify keys we want to load. For 0.3, I think this is a nice perf boost with minimal complexity incurred. |
Codecov Report
@@ Coverage Diff @@
## main #791 +/- ##
==========================================
+ Coverage 75.42% 75.53% +0.10%
==========================================
Files 196 192 -4
Lines 12360 12316 -44
==========================================
- Hits 9323 9303 -20
+ Misses 3037 3013 -24 Continue to review full report at Codecov.
|
Pull Request Test Coverage Report for Build f8b7ab38541050e0133151c0610b138542df24e4-PR-791
💛 - Coveralls |
| w.write_str(symbol)? | ||
| } | ||
| field @ FieldSymbol::TimeZone(_) => return Err(Error::UnsupportedField(field)), | ||
| }; | ||
| Ok(()) | ||
| } | ||
|
|
||
| // This function determins whether the struct will load symbols data. | ||
| // Keep it in sync with the `write_field` use of symbols. |
There was a problem hiding this comment.
Issue: "Keep it in sync" is a noble comment, but the reality is that devs don't obey instructions in code comments unless there is a hard error. Is there a way to make this into more of a hard error? I guess test coverage, to make sure we hit your .expect(), but is that all we can do?
There was a problem hiding this comment.
I assume it is. I could try to build some more elaborate enum that has to be true here and in the analyze function together, but I think it's overengineering at this point, and I'd prefer to land it as is.
| if supports_time_zones { | ||
| if requires_symbols { | ||
| // If we require time zones, and symbols, we know all | ||
| // we need to return already. | ||
| break; | ||
| } | ||
| } else if matches!(field.symbol, FieldSymbol::TimeZone(_)) { | ||
| // If we don't support time zones, and encountered a time zone | ||
| // field, error out. | ||
| return Err(field); | ||
| } |
There was a problem hiding this comment.
Suggestion: I don't see the point of this if/elseif. More specifically, I don't think checking supports_time_zones affects any behavior. I think you can remove the supports_time_zones argument and just unconditionally break whenever requires_symbols gets set (or, better, delete that variable and just return true).
There was a problem hiding this comment.
The value is that we also validate the time zone availability here. If I were to apply the change you're suggesting, assuming I understand it, then we wouldn't error out if we established we need symbols but also the pattern used time zone and was called from non-time-zoned DTF.
Am I missing something?
There was a problem hiding this comment.
ok, I didn't catch that you actually wanted to validate something. This is fine then.
sffc
added
discuss-priority
Initial pattern analysis allows us to bail from loading symbols if they're not used by the pattern.
The
analyze_patternsignature will get more advanced when we split keys further to instrument decisions on which keys to load, but I think it's a good milestone to land it already.The common cases where this is useful are 24h times, and numerical dates.
A decent win on the overview benchmark: