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.

Sign up for GitHub

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

Jump to bottom

leverage minimal graph to build query builder with datsapaces #3488

Merged
merged 9 commits into from
Oct 30, 2024
Merged

leverage minimal graph to build query builder with datsapaces #3488

merged 9 commits into from
Oct 30, 2024

Conversation

YannanGao-gs
Copy link
Contributor

@YannanGao-gs YannanGao-gs commented Aug 30, 2024 •
edited
Loading

Summary

Use PMCD returned by mappinganalysis to build minimal graph for query

clean/update: #2523

How did you test this change?

  • Test(s) added
  • Manual testing (please provide screenshots/recordings)
  • No testing (please provide an explanation)
Screen.Recording.2024-09-26.at.10.28.48.PM.mov

enable switch to load full graph

Screen.Recording.2024-09-26.at.10.27.01.PM.mov

@YannanGao-gs YannanGao-gs requested a review from a team as a code owner August 30, 2024 21:14
@YannanGao-gs YannanGao-gs marked this pull request as draft August 30, 2024 21:14
@changeset-bot changeset-bot
Copy link

changeset-bot bot commented Aug 30, 2024 •
edited
Loading

🦋 Changeset detected

Latest commit: 7d4c264

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 29 packages
Name Type
@finos/legend-extension-dsl-data-space Minor
@finos/legend-application-query Minor
@finos/legend-extension-dsl-data-space-studio Patch
@finos/legend-application-studio Patch
@finos/legend-query-builder Patch
@finos/legend-server-depot Patch
@finos/legend-graph Patch
@finos/legend-application-query-bootstrap Patch
@finos/legend-application-studio-bootstrap Patch
@finos/legend-extension-dsl-data-quality Patch
@finos/legend-extension-dsl-service Patch
@finos/legend-extension-assortment Patch
@finos/legend-extension-dsl-diagram Patch
@finos/legend-extension-dsl-persistence Patch
@finos/legend-extension-dsl-text Patch
@finos/legend-extension-store-flat-data Patch
@finos/legend-extension-store-relational Patch
@finos/legend-extension-store-service-store Patch
@finos/legend-vscode-extension-dependencies Patch
@finos/legend-application-pure-ide Patch
@finos/legend-application-repl Patch
@finos/legend-code-editor Patch
@finos/legend-data-cube Patch
@finos/legend-lego Patch
@finos/legend-application-query-deployment Patch
@finos/legend-application-studio-deployment Patch
@finos/legend-application-pure-ide-deployment Patch
@finos/legend-application-repl-deployment Patch
@finos/legend-server-showcase-deployment Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@YannanGao-gs YannanGao-gs self-assigned this Aug 30, 2024
@codecov Codecov
Copy link

codecov bot commented Aug 30, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 32.14286% with 950 lines in your changes missing coverage. Please review.

Project coverage is 45.49%. Comparing base (c1f82a0) to head (7d4c264).
Report is 12 commits behind head on master.

Files with missing lines Patch % Lines
...e/v1/V1_DSL_DataSpace_PureGraphManagerExtension.ts 4.73% 342 Missing ⚠️
...c/components/Core_LegendQueryApplicationPlugin.tsx 14.59% 240 Missing ⚠️
...s/data-space/DataSpaceTemplateQueryCreatorStore.ts 9.58% 66 Missing ⚠️
...d-application-query/src/stores/QueryEditorStore.ts 82.67% 40 Missing and 4 partials ⚠️
...components/query-builder/DataSpaceQueryBuilder.tsx 0.00% 40 Missing ⚠️
...stores/query-builder/DataSpaceQueryBuilderState.ts 0.00% 32 Missing ⚠️
...ph-manager/protocol/pure/v1/V1_PureGraphManager.ts 8.57% 32 Missing ⚠️
...r/action/analytics/MappingModelCoverageAnalysis.ts 10.34% 26 Missing ⚠️
...r/src/stores/workflows/ServiceQueryBuilderState.ts 0.00% 19 Missing ⚠️
...gend-query-builder/src/stores/QueryBuilderState.ts 18.18% 18 Missing ⚠️
... and 19 more
Additional details and impacted files 
@@            Coverage Diff             @@
##           master    #3488      +/-   ##
==========================================
- Coverage   45.55%   45.49%   -0.06%     
==========================================
  Files        2168     2172       +4     
  Lines      374935   376107    +1172     
  Branches    16118    16117       -1     
==========================================
+ Hits       170792   171104     +312     
- Misses     203170   203990     +820     
- Partials      973     1013      +40
Files with missing lines Coverage Δ
...ry/src/application/LegendQueryApplicationConfig.ts 98.41% <100.00%> (+0.02%) ⬆️
...s/__test-utils__/QueryEditorComponentTestUtils.tsx 100.00% <100.00%> (ø)
...-query/src/stores/LegendQueryApplicationPlugin.tsx 100.00% <100.00%> (ø)
...r-group/mapping-editor/MappingExecutionBuilder.tsx 58.41% <100.00%> (ø)
...ng-editor/legacy/DEPRECATED__MappingTestEditor.tsx 69.47% <100.00%> (ø)
...itor/editor-group/uml-editor/ClassQueryBuilder.tsx 42.11% <100.00%> (ø)
...dio/src/stores/editor/EmbeddedQueryBuilderState.ts 79.66% <100.00%> (+0.17%) ⬆️
...ol/pure/DSL_DataSpace_PureGraphManagerExtension.ts 92.22% <100.00%> (+1.94%) ⬆️
...-graph/src/graph-manager/GraphManagerStatistics.ts 52.00% <100.00%> (+0.48%) ⬆️
...gend-graph/src/graph-manager/action/query/Query.ts 41.66% <100.00%> (+0.70%) ⬆️
... and 29 more

... and 74 files with indirect coverage changes

@YannanGao-gs YannanGao-gs force-pushed the performanceEnhance branch 3 times, most recently from 2375853 to b8520b0 Compare September 3, 2024 20:27
@YannanGao-gs YannanGao-gs marked this pull request as ready for review September 3, 2024 20:27
@YannanGao-gs YannanGao-gs mentioned this pull request Sep 3, 2024
Closed
3 tasks
@YannanGao-gs YannanGao-gs marked this pull request as draft September 3, 2024 20:29
@YannanGao-gs YannanGao-gs force-pushed the performanceEnhance branch 3 times, most recently from aab4beb to e84388e Compare September 4, 2024 13:26
@YannanGao-gs
Copy link
Contributor Author

YannanGao-gs commented Sep 4, 2024 •
edited
Loading

HEADS-UP:

To fix (regressions)

  • about dataspace will not work due to missing connection (modify result json to have connection path)
  • dataspace template query is not working due to empty executables
  • either add diagram analyze coverage in engine or bypass diagram check in secondpass in studio

@YannanGao-gs YannanGao-gs force-pushed the performanceEnhance branch 4 times, most recently from 987b5c7 to 04837d5 Compare October 18, 2024 17:14
@YannanGao-gs YannanGao-gs force-pushed the performanceEnhance branch 2 times, most recently from 57f997c to f5123a2 Compare October 23, 2024 19:35
MauricioUyaguari
MauricioUyaguari requested changes Oct 29, 2024
View reviewed changes
packages/legend-graph/src/graph-manager/protocol/pure/v1/V1_PureGraphManager.ts Show resolved Hide resolved
...ension-dsl-data-space/src/__lib__/to-delete/DSL_DataSpace_LegendQueryNavigation_to_delete.ts Outdated Show resolved Hide resolved
packages/legend-application-query/src/__lib__/DSL_DataSpace_LegendQueryNavigation.ts Outdated
@@ -39,6 +39,7 @@ export enum DATA_SPACE_TEMPLATE_QUERY_CREATOR_ROUTE_PATTERN_TOKEN {
GAV = 'gav',
DATA_SPACE_PATH = 'dataSpacePath',
TEMPLATE = 'template',
EXECUTION_CONTEXT = 'executionContext',
Copy link
Member

Choose a reason for hiding this comment

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

we need to make this work without requiring the execution context key. we would make the url not backward compatible with this change. can we maybe just resolve the execution key by fetching the dataspace v1 from metadata first ?

MauricioUyaguari
MauricioUyaguari requested changes Oct 30, 2024
View reviewed changes
Copy link
Member

@MauricioUyaguari MauricioUyaguari left a comment •
edited
Loading

Choose a reason for hiding this comment

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

We can merge if you address the last open comments I left (including not requireing exeuction context in URL for template queries).
Anything labeled [Next PR] we can address after testing on integration environment (still need to be done to be considered done with feature).

packages/legend-query-builder/src/components/workflows/ServiceQueryBuilder.tsx
@@ -82,16 +83,21 @@ const ServiceQueryBuilderSetupPanelContent = observer(
queryBuilderState.selectedExecutionContext,
)
: null;
const onExecutionContextOptionChange = (
const onExecutionContextOptionChange = async (
Copy link
Member

Choose a reason for hiding this comment

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

[NEXT PR] could we add a test with opening query builder with service with mutli execution and switching.

...-data-space/src/graph-manager/protocol/pure/v1/V1_DSL_DataSpace_PureGraphManagerExtension.ts Show resolved Hide resolved
packages/legend-application-query/src/components/__tests__/QueryEditor.dataspace.test.tsx
@@ -106,3 +106,70 @@ test(
// await waitFor(() => getByText(dataspaceViewModal, 'this is template with inline query'));
},
);

test(
Copy link
Member

Choose a reason for hiding this comment

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

[NEXT PR] we need to increase test coverage here.

  1. can we test when no light graph is returned in the result.
  2. can we maybe add to the engine roundtrip and test the analysis dataspace endpoint and do unit testing on some of the build graph/ build query builder state you have build.
  3. can we ensure some of the CRUD methods on dataspace queries are still working and saving as expcted.

MauricioUyaguari
MauricioUyaguari approved these changes Oct 30, 2024
View reviewed changes
...-data-space/src/graph-manager/protocol/pure/v1/V1_DSL_DataSpace_PureGraphManagerExtension.ts
templateQueryId: string,
plugins: PureProtocolProcessorPlugin[],
): string | undefined {
const analysisResult = V1_deserializeDataSpaceAnalysisResult(
Copy link
Member

Choose a reason for hiding this comment

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

[Next PR] lets add a test opening a template query with dataspace (with analytics info and without).

@MauricioUyaguari MauricioUyaguari changed the title Use PMCD returned by mappinganalysis to build minimal graph for query leverage minimal graph to build query builder with datsapaces Oct 30, 2024
@MauricioUyaguari MauricioUyaguari merged commit 2fa496b into finos:master Oct 30, 2024
15 checks passed
@github-actions github-actions bot mentioned this pull request Oct 30, 2024
Merged
YannanGao-gs added a commit to YannanGao-gs/legend-studio that referenced this pull request Nov 1, 2024
@YannanGao-gs
Revert leverage minimal graph to build query builder with datsapaces (f…
8aa2a63 
…inos#3488)
@YannanGao-gs YannanGao-gs mentioned this pull request Nov 1, 2024
Merged
3 tasks
YannanGao-gs added a commit to YannanGao-gs/legend-studio that referenced this pull request Nov 12, 2024
@YannanGao-gs
Revert leverage minimal graph to build query builder with datsapaces (f…
e8618bb 
…inos#3488)
MauricioUyaguari pushed a commit that referenced this pull request Nov 12, 2024
@YannanGao-gs
Revert leverage minimal graph to build query builder with datsapaces (#…
12ca35f 
…3488) (#3640)

* Revert leverage minimal graph to build query builder with datsapaces (#3488)

* fix build
YannanGao-gs added a commit to YannanGao-gs/legend-studio that referenced this pull request Nov 18, 2024
@YannanGao-gs
Revert "Revert leverage minimal graph to build query builder with dat…
86dda5f 
…sapaces (finos#3488) (finos#3640)"

This reverts commit 12ca35f.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MauricioUyaguari MauricioUyaguari MauricioUyaguari approved these changes

Assignees

@YannanGao-gs YannanGao-gs

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@YannanGao-gs @MauricioUyaguari