-
Notifications
You must be signed in to change notification settings - Fork 83
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Support for multiple FHIR profile extensions #986
Support for multiple FHIR profile extensions #986
Conversation
2105ecb
to
41c1f5d
Compare
41c1f5d
to
329bbbf
Compare
@bashir2 The PR is ready for the first round of review, can you please have a look and let me know if the approach taken is good. In the meanwhile, I will add unit tests. |
329bbbf
to
8c05e77
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @chandrashekar-s for the changes. I think this is a nice solution for the multiple extension issue and I like the merge()
idea. I am sharing my first batch of comments. I have not yet finished my first pass and may share another batch later today, if I can (otherwise tomorrow).
pipelines/batch/src/main/java/com/google/fhir/analytics/FetchSearchPageFn.java
Outdated
Show resolved
Hide resolved
bunsen/bunsen-core/src/test/java/com/cerner/bunsen/ProfileMapperProviderTest.java
Outdated
Show resolved
Hide resolved
pipelines/common/src/test/java/com/google/fhir/analytics/AvroConversionUtilTest.java
Outdated
Show resolved
Hide resolved
bunsen/bunsen-core/src/main/java/com/cerner/bunsen/definitions/StructureDefinitions.java
Show resolved
Hide resolved
bunsen/bunsen-avro/src/main/java/com/cerner/bunsen/avro/AvroConverter.java
Outdated
Show resolved
Hide resolved
bunsen/bunsen-avro/src/main/java/com/cerner/bunsen/avro/AvroConverter.java
Show resolved
Hide resolved
bunsen/bunsen-avro/src/main/java/com/cerner/bunsen/avro/converters/DefinitionToAvroVisitor.java
Outdated
Show resolved
Hide resolved
bunsen/bunsen-avro/src/main/java/com/cerner/bunsen/avro/converters/DefinitionToAvroVisitor.java
Show resolved
Hide resolved
bunsen/bunsen-avro/src/main/java/com/cerner/bunsen/avro/converters/DefinitionToAvroVisitor.java
Outdated
Show resolved
Hide resolved
bunsen/bunsen-avro/src/main/java/com/cerner/bunsen/avro/converters/DefinitionToAvroVisitor.java
Show resolved
Hide resolved
02d314b
to
ba495b9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the review @bashir2. In the latest commits, I have added more unit tests and have addressed your comments.
bunsen/bunsen-avro/src/main/java/com/cerner/bunsen/avro/AvroConverter.java
Outdated
Show resolved
Hide resolved
bunsen/bunsen-avro/src/main/java/com/cerner/bunsen/avro/AvroConverter.java
Show resolved
Hide resolved
bunsen/bunsen-avro/src/main/java/com/cerner/bunsen/avro/converters/DefinitionToAvroVisitor.java
Outdated
Show resolved
Hide resolved
bunsen/bunsen-avro/src/main/java/com/cerner/bunsen/avro/converters/DefinitionToAvroVisitor.java
Outdated
Show resolved
Hide resolved
bunsen/bunsen-avro/src/main/java/com/cerner/bunsen/avro/converters/DefinitionToAvroVisitor.java
Show resolved
Hide resolved
bunsen/bunsen-core/src/main/java/com/cerner/bunsen/definitions/StructureDefinitions.java
Show resolved
Hide resolved
pipelines/batch/src/main/java/com/google/fhir/analytics/FetchSearchPageFn.java
Outdated
Show resolved
Hide resolved
pipelines/batch/src/main/java/com/google/fhir/analytics/ParquetMerger.java
Outdated
Show resolved
Hide resolved
bunsen/bunsen-avro/src/main/java/com/cerner/bunsen/avro/AvroConverter.java
Show resolved
Hide resolved
bunsen/bunsen-avro/src/main/java/com/cerner/bunsen/avro/AvroConverter.java
Show resolved
Hide resolved
0695519
to
af96f19
Compare
@bashir2 Please have a look at the latest commits containing more unit tests and some minor fixes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @chandrashekar-s for the changes. Most of my comments are minor and I think we can close this PR this week. Beside the comments below, there is also one unresolved comment about ParquetMerger
which I cannot reply to anymore (I guess because you have force pushed over an old commit); it is about the experiments you did with compatible but different schemas (e.g., when a new extension is added). Can you please document your finding somewhere? Even if it is in a javadoc that is fine, but it would be nice to have a record of that.
bunsen/bunsen-avro/src/main/java/com/cerner/bunsen/avro/converters/DefinitionToAvroVisitor.java
Show resolved
Hide resolved
bunsen/bunsen-avro/src/main/java/com/cerner/bunsen/avro/converters/DefinitionToAvroVisitor.java
Show resolved
Hide resolved
bunsen/bunsen-avro/src/test/java/com/cerner/bunsen/avro/AvroConverterMergeTest.java
Outdated
Show resolved
Hide resolved
bunsen/bunsen-avro/src/main/java/com/cerner/bunsen/avro/AvroConverter.java
Show resolved
Hide resolved
bunsen/bunsen-avro/src/test/java/com/cerner/bunsen/avro/AvroConverterMergeTest.java
Show resolved
Hide resolved
bunsen/bunsen-core/src/main/java/com/cerner/bunsen/definitions/HapiConverter.java
Outdated
Show resolved
Hide resolved
bunsen/bunsen-core/src/main/java/com/cerner/bunsen/definitions/HapiConverter.java
Outdated
Show resolved
Hide resolved
bunsen/bunsen-core/src/main/java/com/cerner/bunsen/definitions/StructureDefinitions.java
Show resolved
Hide resolved
pipelines/common/src/test/java/com/google/fhir/analytics/AvroConversionUtilTest.java
Outdated
Show resolved
Hide resolved
Thanks @bashir2 for the detailed review. I have addressed most of the comments, there are only 2 comments left which we can discuss in our tomorrow's meeting. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @chandrashekar-s for the changes; all remaining comments are minor and/or we discussed them. So please feel free to merge this once they are resolved.
bunsen/bunsen-avro/src/main/java/com/cerner/bunsen/avro/converters/DefinitionToAvroVisitor.java
Outdated
Show resolved
Hide resolved
bunsen/bunsen-core/src/main/java/com/cerner/bunsen/definitions/EnumConverter.java
Outdated
Show resolved
Hide resolved
pipelines/batch/src/main/java/com/google/fhir/analytics/ParquetMerger.java
Show resolved
Hide resolved
bunsen/bunsen-avro/src/test/java/com/cerner/bunsen/avro/AvroConverterMergeTest.java
Outdated
Show resolved
Hide resolved
bunsen/bunsen-avro/src/test/java/com/cerner/bunsen/avro/AvroConverterMergeTest.java
Show resolved
Hide resolved
bunsen/bunsen-core/src/main/java/com/cerner/bunsen/definitions/StructureDefinitions.java
Show resolved
Hide resolved
d1b3297
to
178366b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @bashir2 for the review. I have addressed the comments in the latest commits. I will be merging the changes. Please let me know if there are any concerns.
Description of what I changed
Fixes #980
This is an extension to the PR #924. In this PR we provide support for multiple FHIR profile extension for a given resource type. This is achieved by creating a Master AvroConverter for the given resource type by merging all the individual AvroConverters against the configured extensions profiles including the base profile. The merging includes a deep merge of the nested elements under the AvroConverter.
E2E test
Tested the changes with the default US Core FHIR profile extensions and verified that the extension fields are properly populated in the parquet files.
TESTED:
Please replace this with a description of how you tested your PR beyond the
automated e2e/unit tests.
Checklist: I completed these to help reviewers :)
I have read and will follow the review process.
I am familiar with Google Style Guides for the language I have coded in.
No? Please take some time and review Java and Python style guides.
My IDE is configured to follow the Google code styles.
No? Unsure? -> configure your IDE.
I have added tests to cover my changes. (If you refactored existing code that was well tested you do not have to add tests)
I ran
mvn clean package
right before creating this pull request and added all formatting changes to my commit.All new and existing tests passed.
My pull request is based on the latest changes of the master branch.
No? Unsure? -> execute command
git pull --rebase upstream master