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

Move export to HTTP #3221

Merged
merged 12 commits into from
Jan 26, 2025
Merged

Move export to HTTP #3221

merged 12 commits into from
Jan 26, 2025

Conversation

aicam
Copy link
Collaborator

@aicam aicam commented Jan 18, 2025

This PR is part of the effort to move download functionality from front end to backend.

New endpoint: /export/result

Previously, ResultExportService was called from WorkflowWebsocketResource, with this PR, it is now called from a new endpoint defined in ResultExportResource. To unify export to dataset and export to local (download), we need to have an endpoint to remove front end from the process. Currently front end fetch all the result and then send them to the user but the goal is to use this new endpoint to stream result directly from backend to user.

Current behavior

The new endpoint only provides export to dataset currently. In code, it supports multiple formats but in front end, we only allow two formats: CSV and Arrow (for binary files). This limitation is based on the previous implementation and might be revised.

Future work

There are four main TODOs left in this PR:

  • Use rowIndex and columnIndex in frontend because already available in backend
  • Request multiple operators result in one HTTP request
  • Adjust endpoint to return the file itself if the export destination is local
  • Adjust endpoint to return a zip file if the export destination is local and multiple operators are selected
2025-01-18.10-34-04.mp4

@aicam aicam requested a review from bobbai00 January 18, 2025 18:38
@aicam aicam self-assigned this Jan 18, 2025
Copy link
Collaborator

@bobbai00 bobbai00 left a comment

Choose a reason for hiding this comment

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

Left some comments.

I have one question: how about the downloading to the local? Is the download done in the frontend by concatenating pages?

@aicam
Copy link
Collaborator Author

aicam commented Jan 22, 2025

Left some comments.

I have one question: how about the downloading to the local? Is the download done in the frontend by concatenating pages?

Download to local is currently disabled (although its logic is still on frontend), however, professor asked me to make small contributions at a time. So in this PR, I just moved export to HTTP.

@aicam aicam requested a review from bobbai00 January 22, 2025 02:30
Copy link
Collaborator

@bobbai00 bobbai00 left a comment

Choose a reason for hiding this comment

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

Left some comments.

Make sure you keep the PR description updated.

@aicam aicam requested a review from bobbai00 January 26, 2025 23:39
Copy link
Collaborator

@bobbai00 bobbai00 left a comment

Choose a reason for hiding this comment

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

LGTM! Left a minor

@aicam aicam merged commit 5e7568e into master Jan 26, 2025
8 checks passed
@aicam aicam deleted the move-export-to-http branch January 26, 2025 23:55
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