PR#2301: avoid redundant clauses and joins in query_buildroots() - koji

koji

#2301 avoid redundant clauses and joins in query_buildroots()

Merged 3 years ago by tkopecek. Opened 3 years ago by mikem.

mikem/koji redundant-joins into master

avoid redundant clauses and joins

Mike McLean • 3 years ago

88a9454

hub/kojihub.py

file modified

-6

		`@@ -5373,9 +5373,6 @@`
		`# run separate queries for picking smallest candidate set`
		`candidate_buildroot_ids = set()`
		`if rpmID is not None:`
		`- joins.insert(0, 'buildroot_listing ON buildroot.id = buildroot_listing.buildroot_id')`
		`- fields.append(('buildroot_listing.is_update', 'is_update'))`
tkopecek commented 3 years ago Do we want to drop this field? It is (probably) not used anywhere, but it still an API change.
		`- clauses.append('buildroot_listing.rpm_id = %(rpmID)i')`
		`query = QueryProcessor(columns=['buildroot_id'], tables=['buildroot_listing'],`
		`clauses=['rpm_id = %(rpmID)i'], opts={'asList': True},`
		`values=locals())`
		`@@ -5385,8 +5382,6 @@`
		`return []`

		`if archiveID is not None:`
		`- joins.insert(0, 'buildroot_archives ON buildroot.id = buildroot_archives.buildroot_id')`
		`- clauses.append('buildroot_archives.archive_id = %(archiveID)i')`
		`query = QueryProcessor(columns=['buildroot_id'], tables=['buildroot_archives'],`
		`clauses=['archive_id = %(archiveID)i'], opts={'asList': True},`
		`values=locals())`
		`@@ -5399,7 +5394,6 @@`
		`return []`

		`if taskID is not None:`
		`- clauses.append('standard_buildroot.task_id = %(taskID)i')`
		`query = QueryProcessor(columns=['buildroot_id'], tables=['standard_buildroot'],`
		`clauses=['task_id = %(taskID)i'], opts={'asList': True},`
		`values=locals())`

mikem commented 3 years ago

Fixes https://pagure.io/koji/issue/2300

Metadata Update from @tkopecek:
- Pull-request tagged with: testing-ready

3 years ago

tkopecek commented on line 6 of hub/kojihub.py 3 years ago

Do we want to drop this field? It is (probably) not used anywhere, but it still an API change.

Metadata Update from @mfilip:
- Pull-request tagged with: testing-done

3 years ago

mikem commented 3 years ago

Do we want to drop this field? It is (probably) not used anywhere, but it still an API change.

I talked about that a little in #2300. It is technically an api change. We don't rely on the field anywhere, and the polymorphism of the result is a little odd (the field is only present when querying by rpmID).

Also, I think most (all?) places in the code where we query buildroots by rpm_id we use listRPMs instead.

mikem commented 3 years ago

It seemed strange to have the changes from #2274 yet still have those joins. Seems to almost defeat the point. Am I thinking about it wrong?

tkopecek commented 3 years ago

Yes, I agree it should be there. I was a bit afraid just about dropping this field. :thumbsup:

tkopecek commented 3 years ago

Commit 6e50120 fixes this pull-request

Pull-Request has been merged by tkopecek

3 years ago