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(dataProcessInstance): Support data process instance entity page and lineage #12499

Merged
merged 8 commits into from
Jan 30, 2025

Conversation

asikowitz
Copy link
Collaborator

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

@asikowitz asikowitz requested a review from shirshanka January 30, 2025 01:54
@github-actions github-actions bot added ingestion PR or Issue related to the ingestion of metadata product PR or Issue related to the DataHub UI/UX devops PR or Issue related to DataHub backend & deployment smoke_test Contains changes related to smoke tests labels Jan 30, 2025
@@ -19,6 +19,30 @@ import {
import { decodeComma } from '../../../utils';

const FILTER = 'filter';
const SEARCH_ENTITY_TYPES = [
EntityType.Dataset,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why not use the backend to do this? Via default search entity types?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Don't want them to appear in main search, only container page / impact analysis for now

@@ -146,6 +170,7 @@ export const EmbeddedListSearchSection = ({

return (
<EmbeddedListSearch
entityTypes={SEARCH_ENTITY_TYPES}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Again, we've been managing this on the backend for some time. Why move to the UI layer?

Copy link

codecov bot commented Jan 30, 2025

Codecov Report

Attention: Patch coverage is 29.72350% with 305 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
...-react/src/app/lineageV2/pruneAllDuplicateEdges.ts 31.85% 92 Missing ⚠️
...t/src/app/lineageV2/useComputeGraph/filterNodes.ts 9.09% 70 Missing ⚠️
.../dataProcessInstance/DataProcessInstanceEntity.tsx 43.18% 25 Missing ⚠️
...pp/lineageV2/LineageFilterNode/computeOrFilters.ts 0.00% 18 Missing ⚠️
datahub-web-react/src/app/lineageV2/common.ts 38.09% 13 Missing ⚠️
...rc/app/lineageV2/controls/LineageSearchFilters.tsx 0.00% 13 Missing ⚠️
.../dataProcessInstance/DataProcessInstanceEntity.tsx 33.33% 12 Missing ⚠️
...b-react/src/app/lineageV2/useColumnHighlighting.ts 0.00% 12 Missing ⚠️
...containers/profile/sidebar/SidebarEntityHeader.tsx 10.00% 9 Missing ⚠️
...p/entityV2/dataProcessInstance/preview/Preview.tsx 20.00% 8 Missing ⚠️
... and 10 more

❌ Your patch status has failed because the patch coverage (29.72%) is below the target coverage (90.00%). You can increase the patch coverage or adjust the target coverage.

Files with missing lines Coverage Δ
.../containers/profile/header/DefaultEntityHeader.tsx 87.84% <100.00%> (ø)
...b-react/src/app/ingest/source/builder/constants.ts 100.00% <100.00%> (ø)
datahub-web-react/src/app/lineageV2/NodeBuilder.ts 28.39% <ø> (ø)
...onents/styled/search/EmbeddedListSearchSection.tsx 40.29% <96.15%> (ø)
...app/lineageV2/useComputeGraph/getDisplayedNodes.ts 19.02% <0.00%> (ø)
...tahub-web-react/src/app/previewV2/EntityHeader.tsx 89.68% <50.00%> (ø)
...eb-react/src/app/lineageV2/useBulkEntityLineage.ts 14.28% <50.00%> (ø)
.../app/lineageV2/useComputeGraph/useComputeGraph.tsx 18.81% <0.00%> (ø)
...-react/src/app/lineageV2/useSearchAcrossLineage.ts 16.80% <25.00%> (ø)
...ub-web-react/src/app/lineageV2/LineageExplorer.tsx 20.71% <0.00%> (ø)
... and 13 more

... and 1147 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c2a4358...ffd5ca4. Read the comment docs.

@datahub-cyborg datahub-cyborg bot added the pending-submitter-response Issue/request has been reviewed but requires a response from the submitter label Jan 30, 2025
@@ -148,6 +149,14 @@ export const DefaultEntityHeader = ({
const displayedEntityType = getDisplayedEntityType(entityData, entityRegistry, entityType);
const { platform, platforms } = getEntityPlatforms(entityType, entityData);

const containerPath =
Copy link
Collaborator

Choose a reason for hiding this comment

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

nice refactor

@@ -75,6 +77,17 @@ export default function LineageSearchFilters() {
/>
</Tooltip>
</ToggleWrapper>
<ToggleWrapper>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks for putting this behind a control. This is really nice!

</CardEntityTitle>
</Tooltip>
) : (
<EntityTitle title={name} onClick={onClick} $titleSizePx={titleSizePx} data-testid="entity-title">
<SearchTextHighlighter field="name" text={name || ''} />
<SearchTextHighlighter field="name" text={name || urn} />
Copy link
Collaborator

Choose a reason for hiding this comment

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

nice change

@@ -80,6 +78,7 @@ export default function useSearchAcrossLineage(
platforms: [DBT_URN],
},
{ entityType: EntityType.DataJob },
{ entityType: EntityType.DataProcessInstance },
Copy link
Collaborator

Choose a reason for hiding this comment

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

Always? I guess so

if (hideDataProcessInstances) {
// Note: Will only pick one query node if there is lineage t1 -> q1 -> dpi1 -> q2 -> t2
// Currently data process instances can't have lineage to queries so this is fine
newNodes = new Map(
Copy link
Collaborator

Choose a reason for hiding this comment

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

I thought we were ignoring dpi as hops.. Do they still come back? So we have to filter them out and connect through them?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I built this like I built the hide transformations toggle, which doesn't alter the query and instead filters entirely on the frontend. If we alter the query, like the show hidden edges (show ghost entities) toggle, then we have to refresh the graph when you toggle, because your graph state might be the result of many search across lineage calls and currently I don't store which ones have been made so we could re-issue them with the new params. I think it's a better experience to get the instant feedback on toggling + not having to re-expand your graph, but there is a concern that a graph could have many data process instances and thus cause slowdown. I think for now, this is ok.

Copy link
Collaborator

@jjoyce0510 jjoyce0510 left a comment

Choose a reason for hiding this comment

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

This looks great. Also relatively low risk...

The main question i had was around moving search entity types to the UI side instead of keeping it at the server.. Is there a reason I am missing? (I'm sure there is)

@datahub-cyborg datahub-cyborg bot added pending-submitter-merge and removed pending-submitter-response Issue/request has been reviewed but requires a response from the submitter labels Jan 30, 2025
Copy link
Contributor

@shirshanka shirshanka left a comment

Choose a reason for hiding this comment

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

LGTM!

@asikowitz asikowitz merged commit aaaa655 into datahub-project:master Jan 30, 2025
193 of 197 checks passed
@asikowitz asikowitz deleted the as--ai-changes-on-top branch January 30, 2025 08:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
devops PR or Issue related to DataHub backend & deployment ingestion PR or Issue related to the ingestion of metadata pending-submitter-merge product PR or Issue related to the DataHub UI/UX smoke_test Contains changes related to smoke tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants