-
Notifications
You must be signed in to change notification settings - Fork 3k
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
feat(ingestion): business-glossary - Add values and relatedTerms support #6148
feat(ingestion): business-glossary - Add values and relatedTerms support #6148
Conversation
…ue/datahub into master+acr-4365-model-changes
…datahub-fork into master+acr-4365-model-changes
…s get set to None
…datahub-fork into master+acr-4365-model-changes
…datahub-fork into master+acr-4365-model-changes
…datahub-fork into master+acr-4365-model-changes
"boostScore": 2.0 | ||
} | ||
} | ||
hasRelatedTermValues: optional array[GlossaryTermUrn] |
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.
this should just be called values
"boostScore": 2.0 | ||
} | ||
} | ||
isRelatedToTerms: optional array[GlossaryTermUrn] |
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.
call this relatedTo
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.
or relatedTerms
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.
Done
term_source: "EXTERNAL" | ||
source_ref: FIBO | ||
source_url: "https://spec.edmcouncil.org/fibo/ontology/FBC/ProductsAndServices/ClientsAndAccounts/Account" | ||
has_value: |
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.
lets call this key values
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.
Done
term_source: "EXTERNAL" | ||
source_ref: FIBO | ||
source_url: "https://spec.edmcouncil.org/fibo/ontology/FBC/ProductsAndServices/ClientsAndAccounts/Account" | ||
is_related_to: |
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.
call this key related_to
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.
or just related_terms
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.
Done
@@ -41,6 +45,8 @@ class GlossaryTermConfig(ConfigModel): | |||
owners: Optional[Owners] | |||
inherits: Optional[List[str]] | |||
contains: Optional[List[str]] |
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.
let's add an additional Optional str field called id
, which if present will be used as the id for the term. The id can be provided in urn:li:..
form or in bare form.
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.
Done
@@ -66,6 +72,8 @@ class DefaultConfig(ConfigModel): | |||
|
|||
class BusinessGlossarySourceConfig(ConfigModel): | |||
file: str = Field(description="Path to business glossary file to ingest.") | |||
enable_datahub_guid: bool = Field( |
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.
Suggest renaming this field to enable_auto_id
to line up with the suggested id
field for terms and term groups.
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.
Done
return v | ||
|
||
|
||
def create_id(path: List[str], enable_datahub_guid: bool) -> str: |
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.
this should additionally take in the Optional id
.
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.
Done
"boostScore": 2.0 | ||
} | ||
} | ||
hasRelatedTermValues: optional array[GlossaryTermUrn] |
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.
simplify to values
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.
Done
…datahub-fork into master+acr-4365-model-changes
…datahub-fork into master+acr-4365-model-changes
Cannot reproduce the datahub-ingestion docker issue locally- I believe it is spurious. Merging through. |
…ort (datahub-project#6148) * WIP * updated snapshot * merge * while creating BusinessGlossaryConfig object, the version field always get set to None * WIP * lint fix * remove field * doc update * rename the fields * review comments * lintFix * Fix the business glossary example link * small fixes to examples and docs * some small tweaks to the model annotations Co-authored-by: MohdSiddique Bagwan <[email protected]> Co-authored-by: Shirshanka Das <[email protected]> Co-authored-by: Gabe Lyons <[email protected]>
…ort (datahub-project#6148) * WIP * updated snapshot * merge * while creating BusinessGlossaryConfig object, the version field always get set to None * WIP * lint fix * remove field * doc update * rename the fields * review comments * lintFix * Fix the business glossary example link * small fixes to examples and docs * some small tweaks to the model annotations Co-authored-by: MohdSiddique Bagwan <[email protected]> Co-authored-by: Shirshanka Das <[email protected]> Co-authored-by: Gabe Lyons <[email protected]>
…ort (datahub-project#6148) * WIP * updated snapshot * merge * while creating BusinessGlossaryConfig object, the version field always get set to None * WIP * lint fix * remove field * doc update * rename the fields * review comments * lintFix * Fix the business glossary example link * small fixes to examples and docs * some small tweaks to the model annotations Co-authored-by: MohdSiddique Bagwan <[email protected]> Co-authored-by: Shirshanka Das <[email protected]> Co-authored-by: Gabe Lyons <[email protected]>
datahub-business-glossary
business_glossary.yml