Skip to content

Issue #317 - Composite Search#499

Merged
prb112 merged 12 commits intomasterfrom
issue-317
Dec 17, 2019
Merged

Issue #317 - Composite Search#499
prb112 merged 12 commits intomasterfrom
issue-317

Conversation

@lmsurpre
Copy link
Copy Markdown
Member

@lmsurpre lmsurpre commented Dec 11, 2019

  1. Converted JDBC DTO Parameter into a "proper" visitor pattern. This
    makes adding a CompositeParameter a lot easier and generally cleans
    things up a bit.

  2. Introduced CompositParameter which contains a list of component
    parameters of any type.

  3. Stubbed out Composite search tests, including:

    • extension-search-parameters.json with a single composite search parm
    • BasicComposite.json with a repeating element for testing composite
    • AbstractSearchCompositeTest stub for laying out the test logic
    • JDBCSearchCompositeTest for executing the tests on Derby
  4. SearchUtil and ParametersUtil updates to include SearchParameter
    canonical uris as an alternative key to the SearchParameter code

    • Also updated fhir-search tests to double the expected search
      parameter counts accordingly
  5. Implemented initial CompositeParameter extraction logic in
    FHIRPersistenceJDBCImpl; needs further cleanup

  6. Introduce support for db-generated identity columns in
    fhir-database-utils. Instead of adding to columndefinition, I added it
    to Table...similar to how primaryKey was implemented.

  7. Added ROW_ID columns for all X_Y_VALUES tables and set to GENERATED BY DEFAULT and made primary key to ensure uniqueness.

  8. Created X_COMPOSITES table with foreign keys to the X_Y_VALUES
    ROW_IDs. Also renamed FhirResourceGroup to FhirResourceTableGroup to
    better reflect its purpose.

  9. Added visit(CompositeParmVal) variant to
    ExtractedParameterValueVisitor and implemented initial DTO logic in
    ParameterVisitorBatchDAO; now the components are saved to the X_Y_VALUES
    tables and the composite row is written using their generated ROW_IDs
    (obtained from preparedStatement.getGeneratedKeys())

  10. Updated SearchUtil to parse composite parameters, both for regular and chained searches

  11. Added processComposite method to AbstractQueryBuilder and JDBCQueryBuilder:

    • where clause gets generated in processCompositeParm
    • for "normal" requests, the query segment gets manipulated by QuerySegmentAggregator
    • for "chained" requests, the query segment is manipulated from JDBCQueryBuilder.appendInnerSelect
  12. Expanded Composite search tests (and fixed related issues):

  • at least one test for each search parameter table type we support (except Location)
  • chained searches (where the composite parameter is at the end of a reference chain)
  • support for the missing modifier on composite parameters
  • 'or' behavior (e.g. val1$val2,val1$val3)
  1. Updated Db2 stored proc to remove composite parameters. Also removed the outdated addParameterArrayTypes.sql stored proc and the associated array types which were still in our schema but no longer used.

  2. Addressed issue Db2 deadlock while executing com.ibm.fhir.schema.app.Main --update-schema #508 by adding a dependency from the resource tables to the global parameter value tables...now the global parameter value tables should always be added first and this should hopefully avoid the deadlock.

Signed-off-by: Lee Surprenant lmsurpre@us.ibm.com

@lmsurpre lmsurpre requested review from JohnTimm and prb112 December 11, 2019 15:27
@lmsurpre lmsurpre changed the title issue #317 - refactor jdbc dto parameters and stub out composite tests WIP: issue #317 - refactor jdbc dto parameters and stub out composite tests Dec 11, 2019
Copy link
Copy Markdown
Collaborator

@JohnTimm JohnTimm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@lmsurpre lmsurpre force-pushed the issue-317 branch 2 times, most recently from e58640e to 35f4933 Compare December 12, 2019 19:57
@lmsurpre lmsurpre changed the title WIP: issue #317 - refactor jdbc dto parameters and stub out composite tests WIP: issue #317 - Composite Search Dec 12, 2019
1. Converted JDBC DTO Parameter into a "proper" visitor pattern. This
makes adding a CompositeParameter a lot easier and generally cleans
things up a bit.

2. Introduced CompositParameter which contains a list of component
parameters of any type.

3. Stubbed out Composite search tests, including:
  * extension-search-parameters.json with a single composite search parm
  * BasicComposite.json with a repeating element for testing composite
  * AbstractSearchCompositeTest stub for laying out the test logic
  * JDBCSearchCompositeTest for executing the tests on Derby

4. SearchUtil and ParametersUtil updates to include SearchParameter
canonical uris as an alternative key to the SearchParameter code
  * Also updated fhir-search tests to double the expected search
parameter counts accordingly

5. Implemented initial CompositeParameter extraction logic in
FHIRPersistenceJDBCImpl; needs further cleanup

Next steps:
* add the BASIC_COMPOSITES table
* update ParamterVisitorBatchDAO (and ParameterDAOImpl?) to
* update Db2 stored proc to do similar
* update SearchUtil to properly parse parameters for composite search
parms
* update JDBCQueryBuilder to query on the Composite table

Signed-off-by: Lee Surprenant <lmsurpre@us.ibm.com>
1. renamed fhir-search Parameter and ParameterValue classes to
`QueryParameter` and `QueryParameterValue` respectively to disambiguate
from fhir-jdbc Parameter classes

2. renamed fhir-persistence-jdbc IParameter and IParameterVisitor
classes to ExtractedParameterValue and ExtractedParameterValueVisitor to
further disambiguate. also renamed subtypes from XParameter to XParmVal
to align with the db table names (X_Y_VALUES)

3. removed unused transformXParameters methods from ParameterDAO
interface and impl

4. other miscelaneous updates

Signed-off-by: Lee Surprenant <lmsurpre@us.ibm.com>
1. Introduce support for db-generated identity columns in
fhir-database-utils. Instead of adding to columndefinition, I added it
to Table...similar to how primaryKey was implemented.

2. Added ROW_ID columns for all X_Y_VALUES tables and set to `GENERATED
BY DEFAULT` and made primary key to ensure uniqueness.

3. Created X_COMPOSITES table with foreign keys to the X_Y_VALUES
ROW_IDs. Also renamed FhirResourceGroup to FhirResourceTableGroup to
better reflect its purpose.

4. Added visit(CompositeParmVal) variant to
ExtractedParameterValueVisitor and implemented initial DTO logic in
ParameterVisitorBatchDAO; now the components are saved to the X_Y_VALUES
tables and the composite row is written using their generated ROW_IDs
(obtained from `preparedStatement.getGeneratedKeys()`).

Also removed composite-parameters.json that I had accidentally checked
in from a previous commit.

Next steps:
* update SearchUtil to properly parse parameters for composite search
parms
* update JDBCQueryBuilder to query on the Composite table
* update the Db2 stored proc to work similar (possibly using 'SELECT
from INSERT' as documented at
https://www.ibm.com/support/knowledgecenter/en/SSEPEK_12.0.0/apsg/src/tpc/db2z_identitycols.html#d7934e239
)

Signed-off-by: Lee Surprenant <lmsurpre@us.ibm.com>
Also updated BasicReference.json so that Reference-id has an identifier
and no reference value. Previously I thought that a reference value
could be just a resource id with the type prefix, but it turns out this
"infer the type" behavior is only for the query and not the resource
value.

Signed-off-by: Lee Surprenant <lmsurpre@us.ibm.com>
Signed-off-by: Lee Surprenant <lmsurpre@us.ibm.com>
it turns out we don't pass the parameters into the stored proc any more,
so the only update here was to delete the parameters from the composites
when we get a new version of the resource (which covers both `delete`
and `update` cases).

also, i deleted the stale `addParameterArrayTypes.sql` stored procedure
template, and the corresponding methods that were defining custom array
types that were only used from this one procedure.

finally, i added a dependency from the resource tables to the "global"
search parameter values tables in fhir-persistence-schema...this to
avoid a deadlock I was hitting very frequently while attempting to
deploy the schema to db2 in a local container

Signed-off-by: Lee Surprenant <lmsurpre@us.ibm.com>
Technically this is an optional dependency, but including it as a
runtime dependency makes it slightly more convenient to:
* run the Main from eclipse / other IDEs
* package the jar with "batteries included" so it can work with db2 out
of the box

I also added the following command to `create-database.sh` to hopefully
avoid a "transaction log full" Db2 SQL Exception I hit once:
* `db2 update database cfg for LOGFILSIZE using 4000`

Finally, I updated some of the README documentation and delivered a
sample `db2.properties` file...hopefully making it easier for others to
follow.

Signed-off-by: Lee Surprenant <lmsurpre@us.ibm.com>
@prb112 prb112 added enhancement New feature or request schema labels Dec 16, 2019
Added info about Composite, Location, and Number/Quantity precision
searches.

Signed-off-by: Lee Surprenant <lmsurpre@us.ibm.com>
I think I added this code originally when first doing the `:missing`
implementation, but its not clearly labeled and I'm not sure why we'd
want to keep it.

Signed-off-by: Lee Surprenant <lmsurpre@us.ibm.com>
Comment thread fhir-examples/src/main/resources/json/ibm/basic/BasicComposite.json
assertNotNull(result);
printSearchParameters("testGetApplicableSearchParameters2", result);
assertEquals(23, result.size());
assertEquals(23 * 2, result.size());
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

* 2, has a particular purpose. Can we throw this in a Constant that explains it?
SearchTest.NUMBER_2_DOES_X?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because we're now indexin the list by both the code and the canonical uri, the expected size is now 2 times the number of search parameters.
I don't even love these tests which just happen to know the number of expected parameters in the file...seems like it would be better to just compute it from the file...otherwise its for sure gonna break each time we add or remove a search parameter.

Comment thread fhir-search/src/main/java/com/ibm/fhir/search/util/SearchUtil.java Outdated
Comment thread fhir-search/src/main/java/com/ibm/fhir/search/parameters/ParametersUtil.java Outdated
Comment thread fhir-search/src/main/java/com/ibm/fhir/search/SearchConstants.java
Comment thread fhir-persistence-schema/pom.xml
Comment thread fhir-persistence-schema/README.md
Comment thread fhir-search/src/main/java/com/ibm/fhir/search/parameters/QueryParameterValue.java Outdated
Copy link
Copy Markdown
Contributor

@prb112 prb112 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, needs to be updated master (obvious as it is in progress)

Signed-off-by: Lee Surprenant <lmsurpre@us.ibm.com>
@lmsurpre lmsurpre changed the title WIP: issue #317 - Composite Search Issue #317 - Composite Search Dec 17, 2019
@lmsurpre lmsurpre requested review from JohnTimm and prb112 December 17, 2019 03:44
Signed-off-by: Lee Surprenant <lmsurpre@us.ibm.com>
Copy link
Copy Markdown
Contributor

@prb112 prb112 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Signed-off-by: Lee Surprenant <lmsurpre@us.ibm.com>
@prb112 prb112 merged commit 6cf8f4c into master Dec 17, 2019
@prb112 prb112 deleted the issue-317 branch December 17, 2019 15:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request schema

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants