refactor(ingest/s3): enhance readability #12686
Open
+72
−42
Conversation
This file contains 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
github-actions
bot
added
ingestion
eagle-25
force-pushed
the
refactor/ingest/s3/enhance-redability-get_folder_info
branch
from
0fc14e6 to
cade3f4
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Continue to review full report in Codecov by Sentry.
|
eagle-25
commented
Feb 20, 2025
| @@ -847,7 +847,7 @@ def get_folder_info( | |||
| path_spec: PathSpec, | |||
| bucket: "Bucket", | |||
| prefix: str, | |||
| ) -> List[Folder]: | |||
| ) -> Iterable[Folder]: | |||
There was a problem hiding this comment.
Changed for memory efficiency
Comment on lines
+880
to
+882
| if _is_allowed_path( | ||
| path_spec, self.create_s3_path(obj.bucket_name, obj.key) | ||
| ) |
Comment on lines
-901
to
-905
| if modification_time is None: | ||
| logger.warning( | ||
| f"Unable to find any files in the folder {key}. Skipping..." | ||
| ) | ||
| continue |
There was a problem hiding this comment.
Remove this(L901~ 905) because groupby_unsorted never returns an empty group.
Comment on lines
+888
to
+901
| for _, group in grouped_s3_objects_by_dirname: | ||
| max_file = max(group, key=lambda x: x.last_modified) | ||
| max_file_s3_path = self.create_s3_path(max_file.bucket_name, max_file.key) | ||
|
|
||
| # If partition_id is None, it means the folder is not a partition | ||
| partition_id = path_spec.get_partition_from_path(max_file_s3_path) | ||
|
|
||
| yield Folder( | ||
| partition_id=partition_id, | ||
| is_partition=bool(partition_id), | ||
| creation_time=min(obj.last_modified for obj in group), | ||
| modification_time=max_file.last_modified, | ||
| sample_file=max_file_s3_path, | ||
| size=sum(obj.size for obj in group), |
There was a problem hiding this comment.
| Before | After |
|---|---|
| O(n) | O(3n) |
We got some performance loss but it is minimal. The time complexity has only increased from O(n) to O(3n).
The reason of O(3n) is this calls max(), min() and sum() in the each loop.
Comment on lines
-911
to
-916
| # If id is None, it means the folder is not a partition | ||
| partitions.append( | ||
| Folder( | ||
| partition_id=id, | ||
| is_partition=bool(id), | ||
| creation_time=creation_time if creation_time else None, # type: ignore[arg-type] |
There was a problem hiding this comment.
We don't need null handling. The type of creation_time is datetime.
creation_timeis the value ofitem.last_modified. (L896)- The type of
itemis ObjectSummary - The type of
ObjectSummary.last_modifiedisdatetime.
- Refactor S3Source().get_folder_info() to enhance readability - Add a test to ensure that get_folder_info() returns the expected result.
eagle-25
force-pushed
the
refactor/ingest/s3/enhance-redability-get_folder_info
branch
from
cade3f4 to
5952e30
Compare
datahub-cyborg
bot
added
the
needs-review
5 tasks
datahub-cyborg
bot
added
pending-submitter-response
|
Comment to change the label |
datahub-cyborg
bot
added
needs-review
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.
S3Source().get_folder_info()to enhance readabilityget_folder_info()returns the expected result.Checklist