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

Balance SQLite expression tree for logical operators (AND/OR) to lower the depth #2565

Merged
merged 24 commits into from
Oct 31, 2024

Conversation

LZRS
Copy link
Collaborator

@LZRS LZRS commented Jun 8, 2024

IMPORTANT: All PRs must be linked to an issue (except for extremely trivial and straightforward changes).

Fixes #2561

Description
Recursively bifurcates the conditional params expressions to prevent occurences of SQLite expression tree exceeding depth of 1000, as suggested in this comment

Alternative(s) considered
Chunking large expression list to limit 50 within parantheses to avoid crashing with Expression tree is too large (maximum depth 1000), as described here

Type
Enhancement Feature

Screenshots (if applicable)

Checklist

  • I have read and acknowledged the Code of conduct.
  • I have read the Contributing page.
  • I have signed the Google Individual CLA, or I am covered by my company's Corporate CLA.
  • I have discussed my proposed solution with code owners in the linked issue(s) and we have agreed upon the general approach.
  • I have run ./gradlew spotlessApply and ./gradlew spotlessCheck to check my code follows the style guide of this project.
  • I have run ./gradlew check and ./gradlew connectedCheck to test my changes locally.
  • I have built and run the demo app(s) to verify my change fixes the issue and/or does not break the demo app(s).

@LZRS LZRS force-pushed the 2561-fix-sqlite-crash branch from ae8187f to 6b3d4c4 Compare June 12, 2024 15:11
@LZRS LZRS marked this pull request as ready for review June 21, 2024 17:52
@LZRS LZRS force-pushed the 2561-fix-sqlite-crash branch from 80c50de to 0264816 Compare June 24, 2024 11:17
@LZRS LZRS requested a review from santosh-pingle as a code owner June 26, 2024 14:16
@LZRS LZRS force-pushed the 2561-fix-sqlite-crash branch from d9e6d03 to d764115 Compare June 26, 2024 22:21
@jingtang10
Copy link
Collaborator

jingtang10 commented Jul 22, 2024

judging purely by the error message, my hypothesis is that the expression tree depth is O(n) for nested OR operators because the expression tree is constructed naively by parsing the OR operators sequentially. For example, for this expression

a OR b OR c OR d OR e OR f OR g OR h

if the expression tree is constructed naively you'd get:

  o
 / \
a   o
   / \
  b   o
     / \
    c   o
       / \
      d   o
         / \
        e   o
           / \
          f   o
             / \
            g   h

where each o stands for an OR operator. This has depth 8.

But what you really want is actually this:

        o
      /   \
    o       o
   / \     / \
  o   o   o   o
 / \ / \ / \ / \
 a b c d e f g h

where the tree is more "balanced" and this has depth 4. In other words, this is O(log(n)).

If my hypothesis of what causes the problem is correct above, instead of trying to break the OR statements into chunks (and having to come up with a value), all you actually have to do is keep the tree balanced by splitting the top level OR statment at the middle of the list of params.

Does this make sense?

@LZRS
Copy link
Collaborator Author

LZRS commented Jul 23, 2024

judging purely by the error message, my hypothesis is that the expression tree depth is O(n) for nested OR operators because the expression tree is constructed naively by parsing the OR operators sequentially. For example, for this expression

a OR b OR c OR d OR e OR f OR g OR h

if the expression tree is constructed naively you'd get:

  o
 / \
a   o
   / \
  b   o
     / \
    c   o
       / \
      d   o
         / \
        e   o
           / \
          f   o
             / \
            g   h

where each o stands for an OR operator. This has depth 8.

But what you really want is actually this:

        o
      /   \
    o       o
   / \     / \
  o   o   o   o
 / \ / \ / \ / \
 a b c d e f g h

where the tree is more "balanced" and this has depth 4. In other words, this is O(log(n)).

If my hypothesis of what causes the problem is correct above, instead of trying to break the OR statements into chunks (and having to come up with a value), all you actually have to do is keep the tree balanced by splitting the top level OR statment at the middle of the list of params.

Does this make sense?

Yeah, it makes sense.

@jingtang10
Copy link
Collaborator

there's no guarantee what i said is true @LZRS - i've done no testing or verification and depending on sqlite's implementation what i said could be complete garbage... so pls test and see if this is true :) (for example you could try to see if you'll hit the depth limit a bit later to bifurcate the tree instead of chunking the parameters)

@LZRS
Copy link
Collaborator Author

LZRS commented Jul 23, 2024

Alright, no problem. I'll test it out and get back

@LZRS LZRS requested a review from a team as a code owner August 22, 2024 00:57
@LZRS
Copy link
Collaborator Author

LZRS commented Aug 22, 2024

there's no guarantee what i said is true @LZRS - i've done no testing or verification and depending on sqlite's implementation what i said could be complete garbage... so pls test and see if this is true :) (for example you could try to see if you'll hit the depth limit a bit later to bifurcate the tree instead of chunking the parameters)

@jingtang10 I tested this out for 1000, 2000 and upto 5000 parameters, and it worked perfectly. I also went ahead a drafted an implementation in the PR

@LZRS LZRS force-pushed the 2561-fix-sqlite-crash branch from af4d912 to 821feab Compare August 22, 2024 13:03
@LZRS LZRS changed the title Modify toQueryString to chunk large list of ConditionParam Modify toQueryString to prevent SQLite expression tree from exceeding depth of 1000 Aug 22, 2024
Copy link
Contributor

@aditya-07 aditya-07 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add a DatabaseImplTest with

ndegwamartin added a commit to opensrp/android-fhir that referenced this pull request Sep 10, 2024
FORK
         - With unmerged PR #9
            - WUP  #13

SDK
            - WUP google#2178
            - WUP google#2650
            - WUP google#2663
PERF
- WUP google#2669
- WUP google#2565
- WUP google#2561
- WUP google#2535
ndegwamartin added a commit to opensrp/android-fhir that referenced this pull request Oct 2, 2024
    FORK
             - With unmerged PR #9
                - WUP  #13

    SDK
                - WUP google#2178
                - WUP google#2650
                - WUP google#2663
    PERF
    - WUP google#2669
    - WUP google#2565
    - WUP google#2561
    - WUP google#2535
@LZRS LZRS requested a review from jingtang10 October 8, 2024 09:37
@LZRS LZRS requested review from pld and jingtang10 October 16, 2024 10:11
@jingtang10 jingtang10 changed the title Modify toQueryString to prevent SQLite expression tree from exceeding depth of 1000 Balance SQLite expression tree for logical operators (AND/OR) to lower the depth Oct 29, 2024
@jingtang10 jingtang10 enabled auto-merge (squash) October 29, 2024 10:53
@jingtang10 jingtang10 merged commit 6977691 into google:master Oct 31, 2024
6 checks passed
@jingtang10 jingtang10 deleted the 2561-fix-sqlite-crash branch October 31, 2024 10:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Complete
Development

Successfully merging this pull request may close these issues.

SQLite crashes with 'Expression tree is too large (maximum depth 1000)'
5 participants