-
-
Notifications
You must be signed in to change notification settings - Fork 4
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
FEATURE: Deterministic charset and collation #14
FEATURE: Deterministic charset and collation #14
Conversation
for greater compatibility
Added some more optimizations after talking to @kitsunet (Thanks again for rubber ducking!) I intentionally left out the binary optimization for |
@@ -254,7 +284,7 @@ private function commitEvent(StreamName $streamName, Event $event, Version $vers | |||
'recordedat' => $this->clock->now(), | |||
], | |||
[ | |||
'version' => Types::INTEGER, | |||
'version' => Types::BIGINT, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is technically wrong in sqlite I guess? But it shouldn't matter much, so no need to change IMHO given that it works.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
eagle-eye Christian :)
The type comes in relevant in Connection:getBindingInfo()
For the value itself, it does not make a difference, because neither IntegerType
nor BigIntType
implement an overload for convertToDatabaseValue()
But I realized, that the returned $bindingType
is different (INTEGER
for IntegerType and STRING
for BigIntType).
I'm not 100% sure whether it makes a difference in our case...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks nice, thank you!
I did some benchmarks to estimate the effect of this PR and unfortunately all numbers are the same or worse with the patch applied.. Performance is equally bad (there are some differences, but I didn't test in fully isolated conditions): To write 1mio events
To read 1mio events from the
|
Where is the benchmark code again? Soemthing must be off, or we missed something, I don#t expect much speed gain here given that we do not do any actual queries that could benefit from anything we do, but the size should not be bigger. |
@kitsunet Ah right, I forgot to add the benchmark script itself. Added it now to the gist
Yes, I didn't either – but I wanted to verify that too
Yes, that puzzled me as well.. But there is not so much that could go wrong really. Here's the created table structures without and with this change applied: Without patch (branch
|
I'm going ahead and merge this anyways. The numbers are not better, but they are not really much worse either and the changes still make sense to me. |
Yep fine by me, I can test this myself over hte next days, I think the direction is definitely right. |
Resolves: #12