Skip to content
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

Add .value() method to the typeCast field wrapper #2607

Closed
wants to merge 3 commits into from

Conversation

Parsonswy
Copy link
Contributor

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 using wrap. 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.

query raw: AVG 0.0762ms
query typecast.string(): AVG 0.0705ms
query typecast.value(): AVG 0.0677ms

@wellwelwel
Copy link
Collaborator

wellwelwel commented Apr 20, 2024

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 🙋🏻‍♂️

@wellwelwel
Copy link
Collaborator

wellwelwel commented Apr 20, 2024

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 #1446 (comment)?

For context:

@sidorares
Copy link
Owner

sidorares commented Apr 21, 2024

I'm a bit confused. What's the difference between added readCodeFor and existing readValueFor ?
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 function

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;
  },

@Parsonswy
Copy link
Contributor Author

Regardless of benchmark, do you think it's possible to make these modifications without "keeping" the breaking changes you mentioned in #1446 (comment)

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 typeCastwill 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?

@sidorares
Copy link
Owner

let's close .value() for now and potentially focus on re-packaging it via documentation changes

typeCast benchmarks would be definitely valuable

@sidorares sidorares closed this Apr 22, 2024
@max-zu
Copy link

max-zu commented Jun 27, 2024

Any progress on this ? We need to update mysql2 package due to this security update. But we can't due to this bug sequelize/sequelize#17141

@wellwelwel
Copy link
Collaborator

wellwelwel commented Jun 27, 2024

@max-zu

Any progress on this ? We need to update mysql2 package due to this security update. But we can't due to this bug sequelize/sequelize#17141

Although the main issue was opened in Sequelize, the issue seems closer to this #1446 (comment) than to Sequelize.

Unfortunately I haven't been able to reproduce this behavior using MySQL2 only. Any help on this would be appreciated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants