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

Fix null cmp codegen #856

Merged
merged 2 commits into from
Feb 2, 2025
Merged

Conversation

sivukhin
Copy link
Contributor

@sivukhin sivukhin commented Feb 1, 2025

This PR remove manual null-comparison optimization (introduced in #847) which replace Eq/Ne instructions with explicit IsNull/NotNull. There are few factors for this change:

  1. Before, manual optimization were incorrect because it ignored jump_if_condition_is_true flag which is important to properly build logical condition evaluation
  2. Manual optimization covered all scenarios in test cases and scenarios when both sides are non trivial expressions were not covered by tests
  3. Limbo already mark literals in the initial emitted bytecode as constants and will evaluate and store them only once - so performance difference from manual optimization seems very minor to me (but I am wrong with high probability)
  4. I think (but again, I am wrong with high probability) that such replacement can be done in the generic optimizator layer instead of manually encode them in the first emit phase

Fixes #850

…true flag

- limbo already store constants only once and more clever optimizations
  better to do with generic optimizator and not manually
@jussisaurio
Copy link
Collaborator

jussisaurio commented Feb 2, 2025

possible to add a test case that would capture 1. current main failing it 2. this branch succeeding?

diff --git a/testing/where.test b/testing/where.test
index d34cc1b..3259cc5 100755
--- a/testing/where.test
+++ b/testing/where.test
@@ -35,6 +35,11 @@ do_execsql_test_small where-not-equals-a-with-nulls {
     select count(*) from demo where value != 'A';
 } {2}
 
+do_execsql_test_small where-is-null-or-id-2 {
+    select id from demo where value is null or id == 2;
+} {2
+4}
+
 do_execsql_test where-clause-eq {
     select last_name from users where id = 2000;
 } {Rodriguez}

i verified this fails on main and succeeds here

EDIT: hmm I guess you added some in #857 ?

@penberg penberg merged commit 650b56e into tursodatabase:main Feb 2, 2025
27 checks passed
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.

OR behavior seems broken
4 participants