Cache common scalar values #1875
Conversation
| return new ScalarValue(value.ToString() ?? ""); | ||
| var stringified = value.ToString(); | ||
|
|
||
| if (stringified == null) | ||
| return ScalarValue.EmptyString; // ScalarValue.Null; ? |
There was a problem hiding this comment.
🤔 ? Why so returning empty string? I tried to change it to return ScalarValue.Null but some tests failed. I have a suspicion that this was done by design a long time ago and now it can be dangerous to change behavior here. Honestly returning null here seems more reasonable.
There was a problem hiding this comment.
Is this even possible? I suppose some class could override ToString method, but that seems silly. As an outsider, returning null seems more logical.
There was a problem hiding this comment.
I suppose some class could override ToString method, but that seems silly.
Not so silly. There are a lot of different situations. Nevertheless, the question is - why to swap null to empty string here? What is the purpose?
There was a problem hiding this comment.
What I meant was, it would be silly for a class to override ToString to return null, but I'm sure it's being done somewhere for a reason I am not aware of.
|
I just ran: select
sum(length(keys(@Properties))) as total,
count(@Properties[?] = -1) as minus_1,
count(@Properties[?] = 0) as zero,
count(@Properties[?] = 1) as plus_1,
count(@Properties[?] = '') as empty,
count(@Properties[?] = null) as nul,
count(@Properties[?] = true) as tru,
count(@Properties[?] = false) as fals
from stream over our last week's raw prod log data, and got:
The logs are our own app logs (system is ~6 years old?) using ASP.NET Core. Out of the 8.5M property values we've collected, shockingly, we don't have any -1 or
Not sure how representative this is, but given the most frequent Anyone watching along keen to spin that query up over a few hours (or days) data in Seq and see whether the picture is similar? |
Yes, experience may vary. |
|
Rebased on dev. What is verdict here? |
|
I'd be inclined to skip this one - doesn't seem compelling enough to justify including it. |
|
Some logs from our production for As I said - experience may vary. In your case you have no enough logs with booleans, ints, etc., OK. |
| if (stringified == null) | ||
| return ScalarValue.EmptyString; // ScalarValue.Null; ? | ||
|
|
||
| if (stringified == string.Empty) |
There was a problem hiding this comment.
A similar check is already happening at line 138. IMO we should move the string checks to the bottom and consolidate to something like var stringified = value is string ? (string)value : value.ToString().
There was a problem hiding this comment.
Similar but not the same. Check on line 138 serves as fast return before calling value.ToString().
|
rebased on dev |
|
|
||
| if (value is int i) | ||
| { | ||
| if (i == 0) return ScalarValue.Zero; |
There was a problem hiding this comment.
We could use a switch expression here, that would eliminate two comparisons if i == -1.
There was a problem hiding this comment.
Switch expression should return value for each case.
fixes #1777