Closed Parsonswy closed 3 weeks ago
Thanks, @Parsonswy 🚀
There are some Sequelize traces on typeCast
+ execute
that I believe the issues come from MySQL2:
Maybe these changes can clarify this issue. As soon as possible I intend to check it out in detail 🙋🏻♂️
A question: this PR would work as a feat
, right?
Regardless of benchmark, do you think it's possible to make these modifications without "keeping" the breaking changes you mentioned in https://github.com/sidorares/node-mysql2/issues/1446#issuecomment-2067516735?
For context:
I'm a bit confused. What's the difference between added
nevermind, I was blind :) I can see the difference now. So what you have in your PR is typeCast field.value is a reference to "non-jit" parser functionreadCodeFor
and existing readValueFor
?
Also looks like what we currently pass as a second parameter ( "next()" function ) to a typeCast call is what we actually need from value()
? Which means that we only need to update documentation
instead of
typeCast: function (field, next) {
if (field.type === 'LONGLONG') {
return field.string() === '1';
}
return next();
},
we can have an example
typeCast: function (field, getValue) {
const value = getValue();
if (field.type === 'LONGLONG') {
return value === '1';
}
return value;
},
And later we can deprecate typeCast
and introduce mapValue
which would get value as a parameter:
mapValue: function (field, value) {
if (field.type === 'LONGLONG') {
return value === '1';
}
return value;
},
Regardless of benchmark, do you think it's possible to make these modifications without "keeping" the breaking changes you mentioned in https://github.com/sidorares/node-mysql2/issues/1446#issuecomment-2067516735
I don't think I understand the question. By "breaking change" I meant invoking typeCast
on binary results at all was a break as there are results which when handled by text protocol with a given implementation of typeCast
will work, but the same implementation would then break if the results are binary parsed. I don't think this PR would have introduced a revert of a breaking change.
Going forward, I don't think it is safe to use the read*
methods on the field wrapper though because of that text/bin discrepancy. I think the safe/protocol agnostic way to cast is what @sidorares pointed out.
Also looks like what we currently pass as a second parameter ( "next()" function ) to a typeCast call is what we actually need from value()? Which means that we only need to update documentation.
Hmm. Yes. I think this is totally possible with the current API and I just zeroed in on .value()
. Updating the docs to to codify that it is ok to invoke next();
as a way to get a "default" deserialization of the MySQL value and then perform any casting off of that makes sense to me.
I think I can close this PR, but will wait for further comment. Maybe the benchmarks are useful?
let's close .value()
for now and potentially focus on re-packaging it via documentation changes
typeCast
benchmarks would be definitely valuable
I was a bit crude here and most or less copy pasted the
readCodeFor
methods that compile the parser and am open to other approaches.Another option I considered was modifying the compiled parser so that it could be invoked recursively, but I felt that was going to be a bit hacky since
wrap()
would need to be able to invoke the parser, but the parser is defined usingwrap
. It would avoid duplicating the switch statement though.I also included a simple benchmark that compares querying with default the default cast behavior, custom type cast with the current wrapper methods, and type cast using the added
.value()
method.These are the results on my machine against a local docker container with mysql 8.3.0.