fix: Improve IO Abstractions and cleanup #755
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- add abstract Reader/Writer contracts - add IOStats and base write_batches implementation - add unit tests for contracts and batching Refs: #718
📜 Docstring Coverage ReportRESULT: PASSED (minimum: 30.0%, actual: 76.6%) Detailed Coverage Report |
📦 Trivy Vulnerability Scan Results
Report Summary
Scan Result Details✅ No vulnerabilities found during the scan for |
📦 Trivy Secret Scan Results
Report Summary
Scan Result Details✅ No secrets found during the scan for |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
🛠 Docs available at: https://k.atlan.dev/application-sdk/refactor/io-too |
inishchith
commented
Oct 6, 2025
| parquet_output: Optional[ParquetOutput], | ||
| parquet_output: Optional[ParquetWriter], | ||
| ) -> Optional[Union[ActivityStatistics, "pd.DataFrame"]]: | ||
| """Execute multi-database flow with proper error handling and result finalization.""" |
There was a problem hiding this comment.
@abhishekagrawal-atlan to review
There was a problem hiding this comment.
what does effective_sql_client mean here? removing it
☂️ Python Coverage
Overall Coverage
New FilesNo new covered files... Modified FilesNo covered modified files...
|
|
🛠 Full Test Coverage Report: https://k.atlan.dev/coverage/application-sdk/pr/755 |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
inishchith
force-pushed
the
refactor/io-too
branch
from
e9f2113 to
8ba41cc
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
📦 Example workflows test results
|
inishchith
force-pushed
the
refactor/io-too
branch
from
8ba41cc to
7bb655a
Compare
📦 Example workflows test results
|
📦 Example workflows test results
|
inishchith
force-pushed
the
refactor/io-too
branch
from
7bb655a to
448c5e1
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
OnkarVO7
reviewed
Oct 14, 2025
| ) | ||
| return ParquetOutput( | ||
| return ParquetWriter( | ||
| output_path=output_path, |
There was a problem hiding this comment.
Can we try to simplify or rename it better this output_path and prefix?
Just by looking at the names it is not self explanatory as to what is required to be passed into these values
There was a problem hiding this comment.
removed output_suffix, better let the developers pass output_path directly.
will remove other such patterns across
| if hasattr(dataframe, "empty") and getattr(dataframe, "empty"): | ||
| continue | ||
| valid_dataframes.append(dataframe) | ||
| # Handle both async and sync iterators |
There was a problem hiding this comment.
These multidb related all funtcions and logic should be in it's separate utility module
| with self.engine.connect() as conn: | ||
| return self._execute_pandas_query(conn, query, chunksize) | ||
|
|
||
| async def get_batched_results( |
There was a problem hiding this comment.
We can combine a lot of code for get_batched_results and get_results
| from sqlalchemy.orm import Session | ||
|
|
||
|
|
||
| class BaseSQLClient(ClientInterface): |
There was a problem hiding this comment.
Thinking out loud here
Should we keep the BaseSQLClient as a normal query executor using sqlalchemy
We create a PandasSQLClient that inherits from BaseSQLClient which uses pandas to execute queries
Similarly DaftSQLClient
This will make the all the code very modular since each executor will have it's own module and devs can choose which query executor they want to use
Also let's say there comes a source which is not supported for pandas or daft normal query executor will have the logic that will work
Dependency management for pandas and daft will also get simplified I feel
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
5 tasks
inishchith
changed the title
inishchith
force-pushed
the
refactor/io-too
branch
from
5ec5f53 to
2f94c84
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
inishchith
commented
Oct 29, 2025
| # Use _write_daft_dataframe with the DataFrame we have | ||
| daft_df = daft.from_pandas(df) | ||
| await parquet_output.write_daft_dataframe( | ||
| await parquet_writer._write_daft_dataframe( |
There was a problem hiding this comment.
we must use write() method with dataframeType as daft
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Changelog
Note
Replaces legacy inputs/outputs and SQLQueryInput with a new io Reader/Writer layer and SQL client get_results APIs, updates activities/handlers/tests accordingly, and removes old docgen/docs and related deps.
application_sdk/iowith genericReader/Writerand concreteJsonFileReader/Writer,ParquetFileReader/Writer,IcebergTableReader/Writer.application_sdk/io/_utilsand addsDataframeTypeenum.application_sdk/inputs/*andapplication_sdk/outputs/*in favor of new IO layer.BaseSQLClient.get_results()andget_batched_results(); deprecatesSQLQueryInputusage.events.modelstointerceptors.modelsin server/worker/interceptors.Written by Cursor Bugbot for commit 07c97ad. This will update automatically on new commits. Configure here.
address: #718
Additional context (e.g. screenshots, logs, links)
Checklist
Copyleft License Compliance