rusackas commented on PR #31485: URL: https://github.com/apache/superset/pull/31485#issuecomment-4042149298
Thanks for working on this — Athena S3 download is a genuinely useful feature. We took a look at the branch with an eye toward rebasing it, but found a few issues that would need to be addressed before it's ready to merge: **Blocking bugs** - `Chart.jsx` `exportFromS3` uses `this.props.slice.slice_id` and `this.props.isCached`, but `Chart` is a functional component — `this` is undefined there and this would crash at runtime. - `query_context.py` references `ChartDataResultLocations` (extra `s`) which won't compile — should be `ChartDataResultLocation`. - `athena.py` `get_remote_download_url` is annotated `-> str` but returns a `pd.DataFrame` (or implicitly `None`). **Design concern** - `superset/utils/aws.py` is imported unconditionally in `models/core.py`, making `boto3` a hard dependency for every Superset install, even those not using Athena. The import (and `boto3` itself) should be guarded so it's only required when the Athena engine is actually in use. **Minor issues** - `aws.py` is missing the Apache license header (will fail the License Check CI job). - `aws.py` `run_query_and_get_s3_url` has an unbounded `while True:` polling loop with no timeout — if Athena never responds, the request hangs indefinitely. - Missing newline at end of `aws.py`. We're going to close this for now to keep the queue tidy. Please feel free to reopen (or open a new PR) once the above are addressed — happy to help get it over the line at that point. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
