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(graphql): enabling graphql for data platform instance aspects #7177

Conversation

sgomezvillamor
Copy link
Contributor

@sgomezvillamor sgomezvillamor commented Jan 30, 2023

This continues the work started here #5728 and so it enables fetching Data Platform Instance aspects via GraphQL.

Not in the scope of this PR and so still pending:

  • graphql:
    • update endpoints
    • enable search and autocompletion
  • UI:
    • search result cards and a profile page for data platform instances

Checklist

  • The PR conforms to DataHub's Contributing Guideline (particularly Commit Message Format)
  • Links to related issues (if applicable)
  • Tests for the changes have been added/updated (if applicable)
  • Docs related to the changes have been added/updated (if applicable). If a new feature has been added a Usage Guide has been added for the same.
  • For any breaking change/potential downtime/deprecation/big changes an entry has been made in Updating DataHub

@github-actions github-actions bot added the product PR or Issue related to the DataHub UI/UX label Jan 30, 2023
@sgomezvillamor
Copy link
Contributor Author

I've been following instructions here https://datahubproject.io/docs/datahub-graphql-core/#extending-the-graph

Although this PR is about adding the aspects to the GraphQL interface, I would like to check the Data Platform Instance entity is searchable. While the instructions mention this related to that:

In order to enable searching an Entity, you'll need to modify the SearchAcrossEntities.java resolver, which enables unified search across all DataHub entities.

I couldn't find where to add the DataPlatformInstance, so I cannot confirm in that code whether it is searchable or not. Likely the code has evolved and those instructions are not up-to-date.

@sgomezvillamor sgomezvillamor marked this pull request as ready for review January 30, 2023 19:29
@chriscollins3456 chriscollins3456 self-assigned this Jan 30, 2023
@chriscollins3456
Copy link
Collaborator

hey @sgomezvillamor thanks for the PR! gonna give this a look through now :)

Copy link
Collaborator

@chriscollins3456 chriscollins3456 left a comment

Choose a reason for hiding this comment

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

Nice! this is looking great. I have a few suggestions / changes but once that's settled I think this is ready to rumble!

@anshbansal anshbansal added the community-contribution PR or Issue raised by member(s) of DataHub Community label Jan 31, 2023
@sgomezvillamor
Copy link
Contributor Author

Thanks @chriscollins3456 for the review.

Here my update:

  • I addressed most of the comments, the easy ones :-)
  • The one about using the mapper helper is still pending; I will have a look at some moment during this week.
  • I also shared some feedback about why I skipped adding domains and glossary terms.

@chriscollins3456
Copy link
Collaborator

  • The one about using the mapper helper is still pending; I will have a look at some moment during this week.

@sgomezvillamor sounds good! let me know if you have any questions - I'm happy to provide some more examples or collaborate some more on this.

@sgomezvillamor
Copy link
Contributor Author

Hi @chriscollins3456 I have addressed the comment about the code style with the mapperHelper. May you have a look? Let me know if there is any other update that you would like to include in this PR.

From my side, the question raised here #7177 (comment) is still open to me. With the updates done here in this PR, is the Data Platform Instance searchable by its aspects?

Thanks!

@chriscollins3456
Copy link
Collaborator

From my side, the question raised here #7177 (comment) is still open to me. With the updates done here in this PR, is the Data Platform Instance searchable by its aspects?

Sorry I didn't even notice that question above! yeah so what you need to do is update SEARCHABLE_ENTITY_TYPES which you will see used in the resolver you mention above. You can also add data platform instance type to AUTO_COMPLETE_ENTITY_TYPES right below it. You'll know what pieces of the entity's aspects are actually searchable by seeing what has a @Searchable annotation above it. so it looks like the name and description from dataPlatformInstanceProperties are searchable already

Copy link
Collaborator

@chriscollins3456 chriscollins3456 left a comment

Choose a reason for hiding this comment

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

@sgomezvillamor This is looking great for what you have! I know you might want to make one last change and add to SEARCHABLE_ENTITY_TYPES and AUTO_COMPLETE_ENTITY_TYPES as well, just let me know when you push that up and we can get this guy in there

@sgomezvillamor
Copy link
Contributor Author

Hi @chriscollins3456 ... here it is, last but not least f975307 :-)

Copy link
Collaborator

@chriscollins3456 chriscollins3456 left a comment

Choose a reason for hiding this comment

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

@sgomezvillamor actually this last commit does cause concern..

If we add it to SEARCHABLE_ENTITY_TYPES and AUTO_COMPLETE_ENTITY_TYPES then data platform instances will start showing up in search and auto-complete results.

however, we don't have search result cards or a profile page for data platform instances built on the frontend yet, so I think this will definitely cause problems.

so before we add those two things in SearchUtils.java we need to build out the frontend pieces for data platform instances!

@sgomezvillamor
Copy link
Contributor Author

Good point on search and autocompletion @chriscollins3456 ! I have reverted that commit and explicitly mentioned in the PR description the features not covered in the scope of this PR (updates, search+autocomplete, UI elements).

Copy link
Collaborator

@chriscollins3456 chriscollins3456 left a comment

Choose a reason for hiding this comment

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

alright nice! thanks so much for this. Excited for this feature development to keep on rolling!

Also sorry about the confusion around adding to seachable entity types and auto-complete - I wasn't thinking it fully through until you had made the change

@sgomezvillamor
Copy link
Contributor Author

Thanks to you for your speedy and responsive support!

@sgomezvillamor
Copy link
Contributor Author

By the way, I cannot merge, so all yours, folks!

@chriscollins3456 chriscollins3456 merged commit 2f2eda0 into datahub-project:master Feb 8, 2023
@sgomezvillamor sgomezvillamor deleted the feat-dataplatforminstance-graphql branch February 9, 2023 09:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
community-contribution PR or Issue raised by member(s) of DataHub Community product PR or Issue related to the DataHub UI/UX
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants