Skip to content
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

Merged

Conversation

siddiquebagwan
Copy link
Contributor

  • update GlossaryRelatedTerms.pdl
  • update source datahub-business-glossary
  • update config business_glossary.yml

@github-actions github-actions bot added the ingestion PR or Issue related to the ingestion of metadata label Oct 7, 2022
@siddiquebagwan siddiquebagwan changed the title feat(ingestion): business-glossary - Add hasValue and isRelatedTerm support feat(ingestion): business-glossary - Add hasValue and isRelated support Oct 7, 2022
@github-actions
Copy link

github-actions bot commented Oct 7, 2022

Unit Test Results (metadata ingestion)

       8 files  ±  0         8 suites  ±0   1h 0m 25s ⏱️ + 1m 54s
   756 tests +  8     753 ✔️ +  8  3 💤 ±0  0 ±0 
1 514 runs  +16  1 508 ✔️ +16  6 💤 ±0  0 ±0 

Results for commit 3c404ee. ± Comparison against base commit d986a04.

♻️ This comment has been updated with latest results.

@github-actions
Copy link

github-actions bot commented Oct 7, 2022

Unit Test Results (build & test)

602 tests  +3   598 ✔️ +3   11m 59s ⏱️ +18s
149 suites ±0       4 💤 ±0 
149 files   ±0       0 ±0 

Results for commit 3c404ee. ± Comparison against base commit d986a04.

♻️ This comment has been updated with latest results.

"boostScore": 2.0
}
}
hasRelatedTermValues: optional array[GlossaryTermUrn]
Copy link
Contributor

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]
Copy link
Contributor

Choose a reason for hiding this comment

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

call this relatedTo

Copy link
Contributor

Choose a reason for hiding this comment

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

or relatedTerms

Copy link
Contributor Author

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:
Copy link
Contributor

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

Copy link
Contributor Author

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:
Copy link
Contributor

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

Copy link
Contributor

@shirshanka shirshanka Oct 31, 2022

Choose a reason for hiding this comment

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

or just related_terms

Copy link
Contributor Author

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]]
Copy link
Contributor

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.

Copy link
Contributor Author

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(
Copy link
Contributor

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.

Copy link
Contributor Author

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:
Copy link
Contributor

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.

Copy link
Contributor Author

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]
Copy link
Contributor

Choose a reason for hiding this comment

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

simplify to values

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@siddiquebagwan siddiquebagwan changed the title feat(ingestion): business-glossary - Add hasValue and isRelated support feat(ingestion): business-glossary - Add values and relatedTerms support Oct 31, 2022
@gabe-lyons
Copy link
Contributor

Cannot reproduce the datahub-ingestion docker issue locally- I believe it is spurious. Merging through.

@gabe-lyons gabe-lyons merged commit fbb1b72 into datahub-project:master Nov 1, 2022
cccs-tom pushed a commit to CybercentreCanada/datahub that referenced this pull request Nov 18, 2022
…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]>
cccs-tom pushed a commit to CybercentreCanada/datahub that referenced this pull request Nov 18, 2022
…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]>
gabe-lyons added a commit to gabe-lyons/datahub that referenced this pull request Jun 13, 2023
…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]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ingestion PR or Issue related to the ingestion of metadata
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants