New manifest structure and added support for BQ manifest. #295
Conversation
|
|
||
| def __init__(self, location: Location) -> None: | ||
| super().__init__(Location(location[5:])) | ||
| TABLE_SCHEMA = [ |
There was a problem hiding this comment.
@fredzyda I could use a second pair of eyes on the schema definition.
There was a problem hiding this comment.
@TylerTCF may have some opinions, too.
There was a problem hiding this comment.
Before we land this change, I was thinking Tyler could build a prototype dashboard out of sample data you produce. That should be a good test of the schema.
There was a problem hiding this comment.
I'll check with Tyler one last time. Otherwise, I think we're good to go.
…from BQ Manifest Client.
Pass manifest instance across pipeline steps instead of dict. Improve mock manifest client & make use of it in tests. Remove None from Status Enum & make set its default value to None in DownloadStatus dataclass.
Instead of creating a separate transaction for each stage, now we will just set the stage using set_stage().
…art time of the stage.
| if parsed_gcs_path.scheme != 'gs' or parsed_gcs_path.netloc == '': | ||
| new_status.size = LocalSystemFileSizeStrategy().get_file_size(path) | ||
| else: | ||
| new_status.size = GCSBlobSizeStrategy().get_file_size(parsed_gcs_path) |
There was a problem hiding this comment.
This is much cleaner. However, could we apply something like the strategy pattern on the manifest subclass itself? e.g. the GCS Manifest has a clac_file_size() method that gets overridden? I see no need to create a separate class hierarchy, this one should do fine.
There was a problem hiding this comment.
For example, our _read and _update calls use the strategy pattern in my book.
There was a problem hiding this comment.
I changed the implementation a bit w.r.t to getting file size. WRYT ?
|
|
||
| def __init__(self, location: Location) -> None: | ||
| super().__init__(Location(location[5:])) | ||
| TABLE_SCHEMA = [ |
There was a problem hiding this comment.
Before we land this change, I was thinking Tyler could build a prototype dashboard out of sample data you produce. That should be a good test of the schema.
Moved the 'config_name' property to the top level of the config dataclass. Few minor changes.
|
Thanks Alex for reviewing. |
Fixes #149 and #60.