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

update dataspace analytics result to contain executable info #3080

Merged
merged 1 commit into from
Sep 18, 2024

Conversation

YannanGao-gs
Copy link
Contributor

@YannanGao-gs YannanGao-gs commented Sep 10, 2024

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?

Copy link

GitHub Actions bot commented Sep 10, 2024

Test Results

  1 033 files  ±0    1 033 suites  ±0   1h 26m 35s ⏱️ +8s
12 270 tests ±0  12 180 ✔️ ±0  90 💤 ±0  0 ±0 
17 961 runs  ±0  17 871 ✔️ ±0  90 💤 ±0  0 ±0 

Results for commit 5b741e5. ± Comparison against base commit b3d9d97.

♻️ This comment has been updated with latest results.

@YannanGao-gs YannanGao-gs force-pushed the functionInfoBackup branch 3 times, most recently from 0eb5617 to 765e3dc Compare September 16, 2024 17:41
@YannanGao-gs YannanGao-gs changed the title update dataspace analytics result to contain function info update dataspace analytics result to contain function info and executable info Sep 16, 2024
@MauricioUyaguari
Copy link
Member

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 YannanGao-gs force-pushed the functionInfoBackup branch 3 times, most recently from aab5385 to 9e43396 Compare September 17, 2024 14:13
@YannanGao-gs
Copy link
Contributor Author

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

@YannanGao-gs YannanGao-gs changed the title update dataspace analytics result to contain function info and executable info update dataspace analytics result to contain executable info Sep 17, 2024
@YannanGao-gs YannanGao-gs force-pushed the functionInfoBackup branch 2 times, most recently from 14443f6 to 72b9b92 Compare September 17, 2024 21:14
@MauricioUyaguari MauricioUyaguari merged commit e9118fa into finos:master Sep 18, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants