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

Several performance optimizations #1822

Conversation

paulrutter
Copy link
Contributor

@paulrutter paulrutter commented Oct 25, 2023

We have been using AlaSQL for a couple of years, and had some patches applied to it locally.
These patches were created after Node.js CPU profiling AlaSQL queries. This PR contains these patches.

Performance improvements for AlaSQL

  • Add AST cache
  • Add regex pattern cache when using LIKE (building RegEx is expensive)
  • Optimize when having lots of values in a (NOT) IN statement, using JS Set
  • Remove RegEx checking for DATE function (what is it used for? It slows DATE functions down)
  • When using NOW, just return the date (not sure why a date is translated to a string?)

Please let me know if these changes make sense and/or require changes.
Especially the last two changes might be there for a reason we don't know about.

I also skipped some test cases around string dates and adjusted one for WHERE IN, using plain string literals.
The PR is lacking new test cases, but if you're interested in these changes, we can work on these.

I couldn't get the formatter to work on Windows though, so that still has to be done.

Thank you for the time you are putting into AlaSQL!

- Add pattern cache when using like (building RegEx is expensive)
- Optimize when having lots of values in a (NOT) IN statement, using JS Set
- Remove RegEx checking for DATE function
- When using NOW, just return the date (not sure why a date is translated to a string and then to a date again)
- Adjust or skip unit tests
- Remove cache property
@mathiasrw
Copy link
Member

I love it!

Let me have a look at the code and merge as soon as possible

@paulrutter
Copy link
Contributor Author

paulrutter commented Oct 30, 2023

I was under the impression that the failing unit tests where due to running on Windows (newlines), but apparantly these also fail in CI. Are these new failures? Then we need to look into these.

- Fix LIKE unit tests by not using toUpperCase() anymore; use same code as before, but now with a patternCache
@paulrutter
Copy link
Contributor Author

paulrutter commented Oct 30, 2023

@mathiasrw I fixed the unit tests for the LIKE, all unit tests succeed now locally:

  ............................................................................................................................................................
  ....,,,......................................,,,................,...........................................................................................
  .............................................................................................................................,,..,................,.........
  ...............................,.........................................................................................................................,,,
  ,,,,............................,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,..........................................,,,...................,,,,......,,,,........
  ..,,,,,,,,,,,,,,,...,,,...,,,,,,,,,,,,,,,,,,.........,,,,,,,,..................................,,,,,,,,,,,,........,,,..............................,,,,,,,,
  ,,.................................,,,,....,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,....,,,,,,................,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,
  ,,,,,,,,,,,,,,,,,,,,,,,,,,,....,,,,,,,,.......,,,,,,..........................................,,,,,,,,,,,,,,,,,,,.........,,,,.,............................
  ...........................................................,........,,.,.,..,,,,,,,,,,......................................................................
  .......,.................................,,,,,,,,,,,,,,,,,,,,,,,,,..........................................................................................
  .....................................................,........................................................,.......,,..,,..,,..,,.........,..............
  ..,.......,..,......,,...................,..,........,,,.....,,,,,,,,,,,,,,,,...............,.,.............................................................
  ...................................................,.................

  1561 passing (4s)
  380 pending

- Always use SET, remove old code
- Made returning Date objects configurable as option, to remain backwards compatible
- Added unit test for Date objects in relevant test cases
@paulrutter
Copy link
Contributor Author

@mathiasrw I adjusted the skipped tests and re-enabled them again, as i made it configurable as option instead.
I extended the relevant unit tests.

Can you run the workflow again?

@mathiasrw
Copy link
Member

I sure can. Sorry for slow response. Lots of things going on at the moment.

I love the performance optimizations, but we cant "just" redo tests that beforehand was working. If we will be doing changes that by some can be considered breaking (= tests are not verifying the same thing as before) than I want to pool those together and add them at the next major version bump (we got a few other tings that is also in the queue for that)

Are you OK to split this PR into so we seperate what demands a change of an existing test?

test/test811.js Show resolved Hide resolved
- Fix using new String in IN statement
- Add new test cases for using plain string literals in both IN and NOT IN
@paulrutter
Copy link
Contributor Author

@mathiasrw I fixed the implementation so it supports both string literals and new String.
Added new unit tests for using string literals in IN, and also added tests using NOT IN.

Can you check and run the workflow again? This way, we don't need to split of the PR.

@paulrutter paulrutter requested a review from mathiasrw November 9, 2023 13:00
src/50expression.js Show resolved Hide resolved
src/50expression.js Show resolved Hide resolved
src/50expression.js Show resolved Hide resolved
src/20database.js Show resolved Hide resolved
src/17alasql.js Show resolved Hide resolved
src/17alasql.js Show resolved Hide resolved
src/15utility.js Show resolved Hide resolved
@mathiasrw mathiasrw merged commit 11e4fe7 into AlaSQL:develop Nov 9, 2023
10 checks passed
@paulrutter paulrutter deleted the maintenance/performance-optimizations-alasql branch November 9, 2023 14:34
@paulrutter
Copy link
Contributor Author

Thanks for merging the PR, will this be part of next release?

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

Successfully merging this pull request may close these issues.

2 participants