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

Test Improvements & Benchmarks for SqlKata Compiler #738

Open
wants to merge 28 commits into
base: master
Choose a base branch
from

Conversation

faddiv
Copy link

@faddiv faddiv commented Feb 8, 2025

Hi Ahmad,

We've been using SqlKata in our project, and I’ve started exploring its internals.
I thought the compiler part could be improved significantly both for speed and memory usage.
However, to rewrite it safely, I first wanted to improve the tests and add some benchmarks.

This PR shows what I did in this regard. Please review this PR, and let me know if you are interested in any part of my work. (I also have a branch in which I implemented a new compiler, just enough to run the benchmarks.)

Summary of Changes:

  • I broke down the tests that check the same input with multiple engines, with Theory and InlineData attributes. This way it can be seen if anything fails separately, and better visible if any test is missing.
    • At some places, I didn't do this breakdown everywhere since I was unsure if it is really worth it. (Base class functionality was tested.)
    • I replaced TestCompilersContainer completely. I was not sure in which direction you wanted to move these tests, but I saw somewhere that you wished to deprecate them.
  • Created separate tests for the bindings.
  • Created some benchmarks with three example queries.
    • These benchmarks only test the compiler part.
    • In benchmarks, I usually create pre-tests to validate whether they return the correct results, which I did here as well.
  • Also, it was my goal, to make the compiler easily replaceable in the tests.

Looking forward to your feedback!

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.

1 participant