Some confusion related to cohort table schema

[chrisknoll]

I'm finding a few places where the cohort table schema is being confused between the result schema and temp schema:

Line 122

return getSqlQueriesToRun(createFeJsonObject(options, options.resultSchema + "." + cohortTable), cohortDefinitionId);

Line 127

String cohortTable = Optional.ofNullable(strata).map(this::getStrataCohortTable).orElse(tempSchema + "." + this.cohortTable);

This might work if the resultsSchema is the same as the tempSchema, but in the case they are different, I believe this would break. However, looking elsewhere in the code, I see something strange when using createDefaultOptions:

Line 246

		final CohortExpressionQueryBuilder.BuildExpressionQueryOptions options = new CohortExpressionQueryBuilder.BuildExpressionQueryOptions();
		options.cdmSchema = cdmSchema;
		// Target schema
		options.resultSchema = tempSchema;
		options.cohortId = id;
		options.generateStats = false;
		return options;

One element of confusion is re-suing the CohortExpressionQueryBuilder.BuildExpressionQueryOptions: this object encapsulates options for cohort definition generation, but the re-use of it in characterization seems strange here. But the main point of confusion is assigning the options.resultSchema = tempSchema. Again, this works if resultSchema == tempSchema, but doing this sort of overwrite here is very confusing to me.

For background: the purpose of defining the cohortTable in the other analysis methods (or even as the targetTable in cohort definitions) was that we don't assume anything about the location of the cohort table: it could be in a completely different schema in the database (something other than result or temp schema). It purpose was that the user would have to specify the fully-qualified table name of the cohort table. Therefore, I believe the solution to the confusion here is to make the callers of the CCQueryBuilder() constructor pass the fully qualified name of the cohort table, and don't attempt to do any tricks with concatenating 'tempSchema' or 'resultsSchema': the cohort table is simply the cohort table (including schema).

It looks like when dealing with strata, a strata-cohort table is created:

return String.format("%s.sc_%s_%d", tempSchema, sessionId, strata.getId());

Which, in this case: the cohort table generated is the fully qualified name, so it does feel like it would be consistent to pass in the 'cohortTable' as the fully qualified name instead of trying to figure out which schema the cohort table belongs to.