Fix/4196 speed up search bundles #4273

AndrewJGaut · 2022-10-07T00:40:21Z

Reasons for making this change

The search_bundles function was slow primarily because it relied on nested subqueries. We updated the function to use inner joins instead, which should be much faster.

Related issues

Fixes #4196. Also addresses #3787 (see example in comment below for that function).

…4196-speed-up-search-bundles

…ix/4196-speed-up-search-bundles

percyliang · 2022-10-07T04:15:05Z

Do you have any before/after timings that you can share?

AndrewJGaut · 2022-10-07T21:25:04Z

Sure thing!

Examples of speedup (run on prod):

Command: cl search .shared data_size=.sum

Original query:
SELECT sum(q1.num) AS sum_1 FROM (SELECT DISTINCT bundle.uuid AS uuid, bundle_metadata_1.metadata_value * 1 AS num FROM bundle_metadata AS bundle_metadata_1, bundle INNER JOIN group_bundle_permission ON bundle.uuid = group_bundle_permission.object_uuid INNER JOIN user_group ON group_bundle_permission.group_uuid = user_group.group_uuid WHERE user_group.user_id='0' AND group_bundle_permission.permission >= 1 AND bundle.uuid = bundle_metadata_1.bundle_uuid AND bundle_metadata_1.metadata_key='data_size') AS q1;
7.52 seconds

Updated query:
SELECT sum(q1.num) AS sum_1 FROM (SELECT DISTINCT bundle.uuid AS uuid, bundle_metadata_1.metadata_value * 1 AS num FROM bundle_metadata AS bundle_metadata_1, bundle INNER JOIN group_bundle_permission ON bundle.uuid = group_bundle_permission.object_uuid INNER JOIN user_group ON group_bundle_permission.group_uuid = user_group.group_uuid WHERE user_group.user_id=’0’ AND group_bundle_permission.permission >= 1 AND bundle.uuid = bundle_metadata_1.bundle_uuid AND bundle_metadata_1.metadata_key='data_size') AS q1;
0.0 seconds

Command: cl search .after=2021-08-20 .count

Original query:
SELECT count(q2.uuid) AS tbl_row_count FROM (SELECT DISTINCT bundle.uuid AS uuid FROM bundle WHERE bundle.uuid IN (SELECT bundle_metadata.bundle_uuid FROM bundle_metadata WHERE bundle_metadata.metadata_key = ‘created’ AND bundle_metadata.metadata_value >= 1629417600) LIMIT 0, 18446744073709551615) AS q2;
27.74 seconds.

Updated query:
SELECT count(q1.uuid) AS tbl_row_count FROM (SELECT DISTINCT bundle.uuid AS uuid FROM bundle INNER JOIN bundle_metadata AS bundle_metadata_1 ON bundle.uuid = bundle_metadata_1.bundle_uuid WHERE bundle_metadata_1.metadata_key = ‘created’ AND bundle_metadata_1.metadata_value >= 1629417600 LIMIT 0, 18446744073709551615) AS q1
1.95 sec

wwwjn · 2022-10-07T22:52:40Z

docker_config/compose_files/.docker/canary.json

@@ -0,0 +1,9 @@
+{


Do we need docker_config files? Or should we remove them?

Just removed them. Sorry about that!

percyliang · 2022-10-08T16:44:04Z

alembic/versions/2022100403_remove_is_superuser_from_user__22b6c41cd29c.py

@@ -0,0 +1,26 @@
+"""Remove is_superuser from User table


This is not part of speeding up search bundles, right? Ideally, would split the superuser thing into a different PR.

Shoot. It is in a different PR that I submitted Thursday (still hasn't been merged b/c needs review). I must have branched off from my local branch I used for that PR when I created the branch for this PR. Sorry about that! Perhaps I should revert those changes in this branch?

Maybe you can get that PR in first, since it's pretty straightforward, and then you don't have to undo anything.

OK, I'll ping the chat for a review. Thanks!

I think it would be easiest to revert those changes in this branch, so it doesn't have to depend on that PR. Up to you though.

percyliang · 2022-10-08T16:44:25Z

codalab/model/bundle_model.py

        shortcuts = {'type': 'bundle_type', 'size': 'data_size', 'worksheet': 'host_worksheet'}

+        join_tables = []


Could you add type annotations?

Added for the two arrays I'm using (joins and where_clause_conjuncts).

When I try to add type annotations for some of the other variables (like limit), I get a strange error from mypy:
Deferral trace:
codalab.model.bundle_model:409
codalab.model.bundle_model:412
codalab.model.bundle_model:414
codalab.model.bundle_model:417
codalab.model.bundle_model:419
codalab.model.bundle_model:433
codalab.model.bundle_model:438
codalab.model.bundle_model:438
codalab.model.bundle_model:438
codalab/model/bundle_model.py: error: INTERNAL ERROR: maximum semantic analysis iteration count reached

So, I've left those un-typed for now, but I'll look into this.

percyliang · 2022-10-08T16:47:43Z

codalab/model/bundle_model.py

-                            cl_group_bundle_permission.c.permission >= GROUP_OBJECT_PERMISSION_READ,
-                        )
-                    )
+                add_join(


I remember there were some subtleties with joins when the joined tables (e.g., permissions) were empty, whereas for subqueries, it was much more straightforward to reason about, so would be good to double check the edge cases.

Yes, good point. So, the queries I converted to inner joins above are all OK in this regard; I verified each of them individually. Basically, the subqueries (aside from the one you highlighted in a comment above and one other which I didn’t convert) are used as conjuncts in the final WHERE clause, aren’t negated, and require the existence of something in the table in the subquery to be true. In other words, each of those subqueries is automatically false if the table is empty anyway.

This is not true for the two other queries I didn't convert earlier (the one used for .floating and one used when the querying user is not root). For those, an inner join would not work for the reason you mentioned. For instance, for the negated query, it checks that the bundle_uuid is NOT in the Worksheet_Item table, and so using an INNER JOIN would be incorrect since the inner join would be empty even though the result we really want is for every bundle_uuid to be in the result since none belong to a worksheet (since worksheet_item is empty). For this query (and for the other I mentioned), we remedy this by using a LEFT OUTER JOIN instead of an INNER JOIN.

I verified that my changes work by adding test cases. I added some for .floating, and I added some cases for cl search queries used by the non-root user. I verified these work and that, as expected, they all fail when I switch the LEFT OUTER JOIN to an INNER JOIN.

Let me know if I didn't address your comment sufficiently and/or if I missed something. Thanks!

percyliang · 2022-10-08T16:49:25Z

codalab/model/bundle_model.py

+                    cl_bundle.c.uuid == aliased_bundle_metadata.c.bundle_uuid,
+                )
+
+                where_clause = []


To be more accurate, I'd call this where_clause_disjuncts

Thanks! (I think they're actually conjuncts in this case since we use and_ to connect them in line 623, but please correct me if I'm wrong!)

percyliang · 2022-10-08T16:50:36Z

codalab/model/bundle_model.py

                )
-                clause = or_(*clause)
+
+                where_clause = []


seems like you can just write or_(cl_bundle..., ..., ...) directly instead of creating an array?

percyliang · 2022-10-08T16:52:07Z

codalab/model/bundle_model.py

+            if where_clause is not None:
+                where_clauses.append(where_clause)
+
+        where_clause = and_(*where_clauses)


Might be confusing to reuse where_clause for both the top-level clause and the conjuncts. One might consider creating helper function(s) to break up this big function.

percyliang · 2022-10-08T16:52:41Z

codalab/model/bundle_model.py

+                or_(  # Join constraint (group)
+                    cl_group_bundle_permission.c.group_uuid
+                    == self.public_group_uuid,  # Public group
+                    cl_group_bundle_permission.c.group_uuid.in_(


We can't replace this subquery with a join?

Done. But see the comment above about INNER JOIN vs LEFT OUTER JOIN. I left this and the other subquery in the .floating case because they were harder to convert and subsequently forgot to convert them. Sorry about that!

percyliang · 2022-10-08T16:53:45Z

codalab/model/bundle_model.py

+            where_clause = and_(where_clause, or_(access_via_owner, access_via_group))
+
+        table = cl_bundle
+        for join_table, join_condition in zip(join_tables, join_conditions):


Instead of using parallel arrays, which might be error prone, consider making a new dataclass with two fields (the table and the join).

percyliang

At a high-level, logic looks good, and it's great that we get such dramatic speedups! I'd just be careful about the edge cases, and we should probably add some test cases for this. Consider also splitting up the function into more modular pieces (something we've wanted to do for a while) if you think it makes sense.

…d clean up variable names

…ating to be joins as well. Add tests for both cases.

AndrewJGaut · 2022-10-09T06:32:17Z

Thanks so much for the thorough review! I added more test cases to address the edge cases you mentioned (see my comment about INNER JOIN and LEFT OUTER JOIN above).

In terms of splitting the function, I do think that makes sense and gave it some thought throughout the day. However, I'm still not sure how to do it. I'll keep thinking, but in the meantime I addressed your other comments. Thanks!

epicfaace · 2022-10-09T20:57:43Z

0.0 seconds

Amazing!!

epicfaace · 2022-10-09T21:00:38Z

codalab/model/bundle_model.py

        subquery_index = [0]

+        @dataclass
+        class JoinTable:


Can we move this class declaration outside the search_bundles function? Eventually (in a later PR probably!) we'll need to do the same thing as this PR for the search_worksheets and will end up using JoinTable for that, too (#4277).

On that point, it might be worth thinking about whether it makes sense to abstract this code out into something more general, so that it could be implemented in search_worksheets (without having to copy and paste everything again). But not necessary for this PR, but if you do think of a good way to do it that works well in this PR, do add it in!

Done. I have a potential idea for abstracting the logic, but I think it might be better to do it in a separate PR. The issue is that the there aren't many shared keys between search_worksheets and search_bundles (aside from the "special" keywords at the top of the function, which can be abstracted quite easily) and even some shared keys are dealt with a little differently (e.g. the key ''), which makes it hard to abstract.

epicfaace · 2022-10-09T21:04:11Z

codalab/model/bundle_model.py

+            table: Table
+            condition: Any
+            left_outer_join: bool = False
+


Great work! However, I think it's a bit difficult to understand how the code is working by just reading through it. Can you add a few sentences at the beginning of the method that describes the general approach this code is taking (and the role played by where_class_conjuncts, add_join, etc.)?

epicfaace · 2022-10-09T21:26:36Z

tests/cli/test_cli.py

+    check_equals('', _run_command([cl, 'search', '.shared']))
+    # Check search with non-root user.
+    if not os.getenv('CODALAB_PROTECTED_MODE'):
+        # This test does not work when protected_mode is True.


Running cl search with non-root user yields a 401 Unauthorized when in protected mode. I could solve this also by using 'cl uedit --grant-access' with the root user before switching to the non-root user instead, though. Perhaps that's better.

…/codalab-worksheets into fix/4196-speed-up-search-bundles

percyliang

Looks great!

AndrewJGaut added 4 commits October 3, 2022 20:47

Remove unused column (is_superuser) from the User table.

cd21e01

Speed up cl search by converting subqueries to inner joins

a1e0980

Merge branch 'fix/3523-remove-is_superuser-from-user-table' into fix/…

26d1b71

…4196-speed-up-search-bundles

Merge branch 'master' of github.com:codalab/codalab-worksheets into f…

8e49820

…ix/4196-speed-up-search-bundles

AndrewJGaut added 3 commits October 6, 2022 21:23

Fix bug with certain keywords

7b3202a

added in aliasing

08e80b3

Run pre-commit

20073eb

AndrewJGaut added 2 commits October 7, 2022 14:36

Remove code used for testing speedups

801c1be

fix issue with mypy typing

d1a649f

wwwjn reviewed Oct 7, 2022

View reviewed changes

Remove docker_config files

af43e96

AndrewJGaut requested a review from teetone October 7, 2022 23:01

teetone requested a review from epicfaace October 7, 2022 23:09

AndrewJGaut added 2 commits October 7, 2022 16:09

fix minor flake8 formatting issues

c5e888b

Fix another formatting issue

fa702d3

percyliang reviewed Oct 8, 2022

View reviewed changes

AndrewJGaut added 4 commits October 8, 2022 14:01

Use just one list of tuples for inner joins, add type annotations, an…

a9510c1

…d clean up variable names

Fix the disjunct subquery for non-root user and the subquery for .flo…

adc63af

…ating to be joins as well. Add tests for both cases.

minor formatting fixes

b91baca

Remove files added by pre-commit

fc66615

AndrewJGaut added 4 commits October 8, 2022 21:06

Remove package files added by pre-commit

b2f129c

fix test that doesn't work in protected mode

2c113a8

minor formatting change

7595b01

add type annotations

8002843

epicfaace mentioned this pull request Oct 9, 2022

Speed up worksheet search with JOINs instead of subqueries #4277

Open

epicfaace reviewed Oct 9, 2022

View reviewed changes

AndrewJGaut and others added 4 commits October 9, 2022 22:09

revert changes made in other PR to remove the superuser column

7c439f0

Merge branch 'master' into fix/4196-speed-up-search-bundles

1b75dfb

add some comments to make the function more understandable

5b62589

Merge branch 'fix/4196-speed-up-search-bundles' of github.com:codalab…

af1a585

…/codalab-worksheets into fix/4196-speed-up-search-bundles

percyliang approved these changes Oct 12, 2022

View reviewed changes

AndrewJGaut added 2 commits October 13, 2022 21:48

Merge branch 'master' into fix/4196-speed-up-search-bundles

c770d7c

Merge branch 'master' into fix/4196-speed-up-search-bundles

93c781c

AndrewJGaut merged commit efd99ac into master Oct 15, 2022

AndrewJGaut deleted the fix/4196-speed-up-search-bundles branch October 15, 2022 21:12

wwwjn mentioned this pull request Oct 25, 2022

Bump version to 1.5.11 #4287

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix/4196 speed up search bundles #4273

Fix/4196 speed up search bundles #4273

AndrewJGaut commented Oct 7, 2022 •

edited

Loading

percyliang commented Oct 7, 2022

AndrewJGaut commented Oct 7, 2022 •

edited

Loading

wwwjn Oct 7, 2022

AndrewJGaut Oct 7, 2022

percyliang Oct 8, 2022 •

edited

Loading

AndrewJGaut Oct 9, 2022

percyliang Oct 9, 2022

AndrewJGaut Oct 9, 2022

epicfaace Oct 9, 2022

percyliang Oct 8, 2022

AndrewJGaut Oct 9, 2022

percyliang Oct 8, 2022

AndrewJGaut Oct 9, 2022 •

edited

Loading

percyliang Oct 8, 2022

AndrewJGaut Oct 9, 2022

percyliang Oct 8, 2022

AndrewJGaut Oct 9, 2022

percyliang Oct 8, 2022

AndrewJGaut Oct 9, 2022

percyliang Oct 8, 2022

AndrewJGaut Oct 9, 2022

percyliang Oct 8, 2022

AndrewJGaut Oct 9, 2022 •

edited

Loading

percyliang left a comment

AndrewJGaut commented Oct 9, 2022 •

edited

Loading

epicfaace commented Oct 9, 2022

epicfaace Oct 9, 2022

epicfaace Oct 9, 2022

AndrewJGaut Oct 10, 2022 •

edited

Loading

epicfaace Oct 9, 2022

AndrewJGaut Oct 10, 2022

epicfaace Oct 9, 2022

AndrewJGaut Oct 10, 2022

percyliang left a comment

		shortcuts = {'type': 'bundle_type', 'size': 'data_size', 'worksheet': 'host_worksheet'}

		join_tables = []

Fix/4196 speed up search bundles #4273

Fix/4196 speed up search bundles #4273

Conversation

AndrewJGaut commented Oct 7, 2022 • edited Loading

Reasons for making this change

Related issues

percyliang commented Oct 7, 2022

AndrewJGaut commented Oct 7, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

percyliang Oct 8, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndrewJGaut Oct 9, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndrewJGaut Oct 9, 2022 • edited Loading

Choose a reason for hiding this comment

percyliang left a comment

Choose a reason for hiding this comment

AndrewJGaut commented Oct 9, 2022 • edited Loading

epicfaace commented Oct 9, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndrewJGaut Oct 10, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

percyliang left a comment

Choose a reason for hiding this comment

AndrewJGaut commented Oct 7, 2022 •

edited

Loading

AndrewJGaut commented Oct 7, 2022 •

edited

Loading

percyliang Oct 8, 2022 •

edited

Loading

AndrewJGaut Oct 9, 2022 •

edited

Loading

AndrewJGaut Oct 9, 2022 •

edited

Loading

AndrewJGaut commented Oct 9, 2022 •

edited

Loading

AndrewJGaut Oct 10, 2022 •

edited

Loading