fix(backend): Fix performance issue within a mysql request

[difince]

One of the requests used for Archiving Experiments is rewritten to use inner join for better performance, instead of using nested select. The fix leverage 'SQLDialect' interface, because the new Mysql request is not supported by SQLite Dialect (used for testing).
There are other MySQL requests that use nested requests in Archive Experiment, but only the most problematic one reported by this comment (bobgy) is fixed in the PR.
The other requests are not optimized, as the resource_reference table will be deprecated soon - once v2beta1 is available.

Issue: #6845

Note: Archive experiments need to be enabled back in test infra. The change done in this PR needs to be reverted once this PR got merged.

Description of your changes:

Checklist:

• The title for your pull request (PR) should follow our title convention. Learn more about the pull request title convention used in this repository.

[difince]

Did another commit - to add unit tests

[gkcalat]

Thank you @difince for the contribution!
I left a few comments.

[gkcalat]

I just realized that this actually should be LEFT JOIN because we are replacing the OR op in the WHERE clause with this.

[difince]

Yep! Thanks for catching this!

[difince]

Re-opening this conversation...
I just realized that the below statement is not correct.

I just realized that this actually should be LEFT JOIN because we are replacing the OR op in the WHERE clause with this.

This is the original request:
UPDATE run_details SET StorageState = 'ARCHIVED' WHERE StorageState <> 'ARCHIVED' AND UUID in (SELECT ResourceUUID FROM resource_references as rf WHERE (rf.ReferenceUUID = "d86e5315-09d7-4c8a-982e-4de004b2e4b6" AND rf.ResourceType = 'Run' AND rf.ReferenceType = 'Experiment')) OR ExperimentUUID = 'd86e5315-09d7-4c8a-982e-4de004b2e4b6';
created by the following code.

	// TODO(gkcalat): deprecate resource_references table once we migration to v2beta1 is available.
	// TODO(jingzhang36): use inner join to replace nested query for better performance.
	filteredRunsSql, filteredRunsArgs, err := sq.Select("ResourceUUID").
		From("resource_references as rf").
		Where(sq.And{
			sq.Eq{"rf.ResourceType": model.RunResourceType},
			sq.Eq{"rf.ReferenceUUID": expId},
			sq.Eq{"rf.ReferenceType": model.ExperimentResourceType},
		}).ToSql()
	if err != nil {
		return util.NewInternalServerError(err,
			"Failed to create query to filter the runs in an experiment %s. error: '%v'", expId, err.Error())
	}
	updateRunsSql, updateRunsArgs, err := sq.
		Update("run_details").
		SetMap(sq.Eq{
			"StorageState": model.StorageStateArchived.ToString(),
		}).
		Where(sq.NotEq{"StorageState": model.StorageStateArchived.ToString()}).
		Where(fmt.Sprintf("UUID in (%s) OR ExperimentUUID = '%s'", filteredRunsSql, expId), filteredRunsArgs...).
		ToSql()
	if err != nil {
		return util.NewInternalServerError(err,
			"Failed to create query to archive the runs in an experiment %s. error: '%v'", expId, err.Error())
	}

This is the new optimized MySQL query:

UPDATE run_details JOIN resource_references ON run_details.UUID = resource_references.ResourceUUID SET StorageState = "STORAGESTATE_ARCHIVED" WHERE resource_references.ResourceType = "RUN" AND resource_references.ReferenceUUID = "<experiment_id>" AND resource_references.ReferenceType = "Experiment"

By adding LEFT JOIN we won't directly achieve OR ExperimentUUID = 'experiment_ID' from the original query, because:

1. In general, LEFT JOIN intends to return the entire LEFT table (we just do not want this) and
2. The WHERE clause of the new query is too specific: WHERE resource_references.ResourceType = "RUN" AND resource_references.ReferenceUUID = "<experiment_id>" AND resource_references.ReferenceType = "Experiment".
   Even if we add an OR ExperimentUUID = 'experiment_ID' statement it won't work because of how we JOIN both Tables. Plus there is already a dedicated query that updates all run_details based just on experimentUUID see (ref A) :

   pipelines/backend/src/apiserver/storage/experiment_store.go

   Lines 338 to 346 in 4f74148

   	updateRunsWithExperimentUUIDSql, updateRunsWithExperimentUUIDArgs, err := sq.
   	Update("run_details").
   	SetMap(sq.Eq{
   	"StorageState": model.StorageStateArchived.ToString(),
   	}).
   	Where(sq.Eq{"ExperimentUUID": expId}).
   	Where(sq.NotEq{"StorageState": model.StorageStateArchived.ToString()}).
   	ToSql()
   	if err != nil {

In short :), After I took a look at how Run and Jobs are created IMO, it would be a bug if we have in the database run_details record without a corresponding resource_reference record with ResourceType= RUN and ReferenceType=Experiment. In other words, Am I right that there shouldn't be existing Runs that don't belong to some Experiment?

• So If this is the case, I suggest keeping the INNER JOIN version of the optimized request and even removing the (ref A) - IMO it has been added for "just in case"

• If I am missing something and that is not the case - I would keep the optimized INNER JOIN request + ref A ( For Runs) and write appropriate ref B for Jobs.

[gkcalat]

Thank you @difince for a thorough analysis. I agree.

[Linchin]

Thank you @difince!
/lgtm
/approve
closes #6845

[google-oss-prow]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Linchin

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• backend/OWNERS [Linchin]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[difince]

Hi @Linchin I haven't removed the "LEFT" from the JOIN as I expect some comments :)
Even with the LEFT JOIN the query is still doing the right thing - the performance is much worse. So, I will open a new PR to remove LEFT JOIN and instead iser INNER!

[gkcalat]

Hi @Linchin I haven't removed the "LEFT" from the JOIN as I expect some comments :) Even with the LEFT JOIN the query is still doing the right thing - the performance is much worse. So, I will open a new PR to remove LEFT JOIN and instead iser INNER!

The PR was merged by accident. In the next PR, could you please replace the join with INNER JOIN and add a TODO with a note to switch to filtering by experiment_id once we deprecate resource_references?

Thank you!