Note about these updates:

1: I adjusted the hash calculation to strip out names/descriptions of inclusion rules so that simple renames of those fields won't generate an entirely new design hash and result. It does mean that the values that show up in the UI will be based on the last-generated design. In addition, this resolves a SQL Injection vulnerability.

2: we now key our cache results on a design hash, and not a result identifier.

3: I would like to add additional tests to verify some of the design hash logic. I will make those updates soon.

4: I removed the functionality of 'get next result ID' because the implementation would not work: if two threads call select MAX() on the same table they will each get the same max ID and then do the wrong thing with it. These queries can't depend on atomic transactions because usually that is not supported on MPP systems.

5: Because of 4, I removed tests that tested the 'nextReslut' value, but I left the test checkCachingWithPrefilledResults. This test fails because at the end there is a assertion that the next result ID must be sequential to the current resultId (which won't happen when we use design hashes). But there may be another aspet of the test that I am not clear about, so I am leaving it for now.

6: We always 'generate with inclusion statistics', even tho generating a cohort characterization or treatment pathways doesn't use that information. The beauty of it is that if someone does want to look at the inclusion summary, they can go into the cohort, generate (from the cache) and see the output.

7: I specifically avoided making any changes to the WebAPI tables. The approach proposed here does make some columns redundant (such as the result_id = design_hash) but I wanted to limit the scope to changes. If requested, we can make additional migrations to adjust the WebAPI tables.

@chrisknoll - let me know if you want to test the SQL Server migration before we merge this into master.

@anthonysena : I found an issue on sql migration where you can't modify a column that is used in a constraint. For consistency, I did this on PG and oracle migrations as well, but I can't test them.

You already probably have a PG migration complete on your local env, so I can either revert the changes in the PR related to PG or you can do a 'soft rollback' on the tables to add the result_id back to the table (so that the migration can drop it again). Your call, I'll make any change you require.

@chrisknoll - thanks for reviewing & testing those out. I re-tested the PG migration on my side and it worked well. I think we are set now. Thanks!

@chrisknoll hi, your PR overrides PR made by @pavgra and currently estimation generation does not work.
for example field org.ohdsi.webapi.Constants.Params.COHORT_ID_FIELD_NAM is not used now CohortGenerationUtils, but it is needed in circe.

@ssuvorov-fls sorry for this regression. To confirm, it is due to the changes in this function signature: https://github.com/OHDSI/WebAPI/pull/1371/files#diff-378ca22851bb7e5375071dc6718e5f9fR14 since the target* params were removed?

@anthonysena I can't say exactly, I've just found that cohort_id field name is not passed to sql script and estimation analysis execution can not be created now

@ssuvorov-fls: can you find out where the issue is? This was tested on CC and pathway generation (where a local cohort table is defined) as well as regular cohort generation (where there is no local cohort temp table). In those cases, it generates the cohort into the cache, and the result is red from the cache into the local cohort. How is estimation working differently from that?

In addition, the test cases related to cohort generation passes, so if there is a failure we have a gap in our test cases.

@chrisknoll it fails for estimation generation. There is a field '@cohort_id_field_name' in sql scripts from circe and currently it is not replaced with value. You can see how it is replaced in #1318

String renderedSql = SqlRender.renderSql(
      sqlBuilder.toString(),
      new String[] {TARGET_DATABASE_SCHEMA, COHORT_ID_FIELD_NAME, TARGET_COHORT_ID},
      new String[]{request.getTargetSchema(), request.getTargetIdFieldName(), request.getTargetId().toString()}
    );

@ssuvorov-fls ,
I can't find where the problem would be. As I said in the origional PR (and issue), Pavel's PR introduced breaking changes to the result schema, and this PR isolated those changes to just introduce cache tables in a backwards-compatible way where 2.7.4 will write directly to a result table (whether it be a local cohort table or a result schema cohort table) while 2.8 writes to a cache table (if necessary) and then copies from the cache to the target table.

It is still unclear as to why you are getting the failure. We don't need to specify a cohort_id_field_name because the generation util writes to the cache, and we don't need to paramaterize that.

The cohrot_id_field_name is replaced with the default (backwards compatible field name) in circe here . Pavel's code was doing it wrong: the cohort_id_field_name should be replaced as a result of the value in the 'options' param, not by a call to sqlRender. If no options are supplied, it results in the origional, backwards compatable query of 'cohort_definition_id'.

This is where it's set in GenerationUtils.buildGenerationSql:

    CohortExpressionQueryBuilder.BuildExpressionQueryOptions options = new CohortExpressionQueryBuilder.BuildExpressionQueryOptions();
    options.cohortIdFieldName = DESIGN_HASH;
    options.cohortId = request.getTargetId();
    options.cdmSchema = cdmSchema;
    options.vocabularySchema = vocabSchema;
    options.generateStats = true; // always generate with stats

Is it possible you don't have the correct results schema tables? Could you provide the exact error you are getting?

@ssuvorov-fls I'd suggest working with @olga-ganina to open a new issue for 2.8 once you've collected more details around this problem. It is unclear to me how Arachne EE is using WebAPI to perform the estimation generation.