-
-
Notifications
You must be signed in to change notification settings - Fork 628
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
feat(disableEval
): add static parsers
#3365
base: master
Are you sure you want to change the base?
feat(disableEval
): add static parsers
#3365
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3365 +/- ##
==========================================
+ Coverage 88.45% 88.70% +0.25%
==========================================
Files 83 85 +2
Lines 13067 13446 +379
Branches 1393 1531 +138
==========================================
+ Hits 11558 11927 +369
- Misses 1509 1519 +10
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Thanks, @thomasgauvin! And sorry, january was a hectic month and I didn't really pay attention to your comment in #2289 (comment). I'll add the label Perhaps a simple approach with parameterized tests can make things easier to avoid duplicate tests. |
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.
@thomasgauvin, I added some security points that were resolved after PR #2099 was created.
If it's not a problem, can I commit directly to your branch (including the typeCast / nestTables
question and to add the tests I mentioned yesterday)?
Also, in #2289 it was decided to use |
Yes, please feel free to commit anything to the branch directly. I've added you as collaborator on the fork so you should have commit access. |
Missed that, will adjust |
Co-authored-by: Weslley Araújo <[email protected]>
Co-authored-by: Weslley Araújo <[email protected]>
This comment was marked as resolved.
This comment was marked as resolved.
@sidorares, as the @thomasgauvin, I won't be committing until Saturday. Feel free to continue or wait 🙋🏻♂️
|
This comment was marked as resolved.
This comment was marked as resolved.
disableEval
): add static parser
Static binary parser implementationI've implemented the new binary static parser preserving the code as faithful as possible to the original, except for two things: Unified/simplified the
A minimal memory improvement, by pre-allocating the length of null bitmaps instead of
TestingAs previously mentioned, there are no new tests due to new parsers. Instead, every single test, including the matrixes (CI), platforms and their respective versions (Node.js, Bun, Deno), as well as the entire test suite are tested against the new parsers globally. Note Although this is a rather intensive approach, it ensures that a modification in one parser will cause failures if the other parser is not updated accordingly, as well as being easier to maintain in the long term by allowing the same test to be run dynamically for each variation. CoverageAs coverage is tested on different matrixes (CI), naturally the percentage when testing only standard or static parsers will reduce the total coverage count. For this reason, I adapted the limits to support this difference. As far as I'm concerned, it's ready to merge, so please feel free to review 🙋🏻♂️ |
disableEval
): add static parserdisableEval
): add static parsers
I'm hoping to help push for getting the interpreting parser available (this makes it possible to use mysql2 as an alternative to mysql on Cloudflare Workers).
I opened this as a new PR because I have some bandwidth to address further comments in the PR if they come up. This code merely applies the recommended fixes as noted by @shiyuhang0 to the original PR by @sidorares #2099 (the original commit history is maintained and the branch was rebased onto master).
This could also be created as a PR to
deoptimise-large-rows
branch, however the maintainers recommend moving forward.