-
-
Notifications
You must be signed in to change notification settings - Fork 73
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
Export Data working #342
base: main
Are you sure you want to change the base?
Export Data working #342
Conversation
anthony2261
commented
Oct 5, 2024
•
edited
Loading
edited
- Stream results BE -> FE
- Stream results SQL -> BE
- Validate that we can export the data for the given result_id
def custom_run_sql_stream(self, query: str) -> Generator[Sequence[Row[Any]], Any, None]: | ||
# https://docs.sqlalchemy.org/en/20/core/connections.html#streaming-with-a-fixed-buffer-via-yield-per | ||
yield_per = 1000 | ||
if self.dialect == "mssql": |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is becoming a little annoying seeing all these if-statements everywhere. Idk if it's time for this yet, cause it's still acceptable, but we should at least start thinking about using some better design pattern here, ex. Command pattern or something where based on the Dialect, the functionality is housed in one place.
result_service: Annotated[ResultService, Depends(ResultService)], | ||
background_tasks: BackgroundTasks, | ||
) -> StreamingResponse: | ||
background_tasks.add_task(posthog_capture, "results_exported") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd be a bit more specific here - export results as csv maybe. Will probs have diff kinds of exports in the future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same with function/route name maybe
|
||
|
||
@router.get("/result/{result_id}/export-url") | ||
async def export_csv_url(request: Request, result_id: UUID) -> SuccessResponse[str]: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't understand why we need this
@@ -103,6 +107,17 @@ export const DynamicTable: React.FC<{ | |||
</button> | |||
</CustomTooltip> | |||
))} | |||
{data.rows.length > 1 && linked_id && ( | |||
<CustomTooltip hoverText="Export"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Export as CSV"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Some tiny comments left all over, nothing major.
🥳 🥳