search

finos / legend-engine

Legend Engine module
https://legend.finos.org
Apache License 2.0
83 stars 235 forks source link

update dataspace analytics result to contain executable info #3080

Closed YannanGao-gs closed 1 month ago

YannanGao-gs commented 2 months ago

What type of PR is this?

Improvement

What does this PR do / why is it needed ?

update dataspace analytics result to contain function info and executable info

Which issue(s) this PR fixes:

Fixes #

Other notes for reviewers:

Does this PR introduce a user-facing change?

github-actions[bot] commented 2 months ago

Test Results

  1 033 files  ±0    1 033 suites  ±0   1h 26m 35s :stopwatch: +8s 12 270 tests ±0  12 180 :heavy_check_mark: ±0  90 :zzz: ±0  0 :x: ±0  17 961 runs  ±0  17 871 :heavy_check_mark: ±0  90 :zzz: ±0  0 :x: ±0 

Results for commit 5b741e57. ± Comparison against base commit b3d9d972.

:recycle: This comment has been updated with latest results.

MauricioUyaguari commented 1 month ago

I think we are in the right direction, but we need to be mindful of not restoring the same exact information. We should store only things we need. I think we can keep the template info inside execution context based on your filtering. But for functions, it should be stored globally in the DataSpaceAnalysisResult class.

Furthermore, one thing I would like you to improve (separate PR is fine). We should not store mapped entities for the same mapping twice. So we should move that also to the global class. Major use of execution context is to have the same mapping but different runtimes. So we should only store one graph per mapping.

YannanGao-gs commented 1 month ago

I think we are in the right direction, but we need to be mindful of not restoring the same exact information. We should store only things we need. I think we can keep the template info inside execution context based on your filtering. But for functions, it should be stored globally in the DataSpaceAnalysisResult class.

Furthermore, one thing I would like you to improve (separate PR is fine). We should not store mapped entities for the same mapping twice. So we should move that also to the global class. Major use of execution context is to have the same mapping but different runtimes. So we should only store one graph per mapping.

For moving mapped entities, I am concerned that it will not be backward compatible if I move it to the root as this field has been there since the beginning