[MAINTENANCE] Remove fluent partitioner methods from DataAssets #9517
Conversation
…75/asset_uses_partitioner_from_batch_request
for more information, see https://pre-commit.ci
| @@ -304,11 +284,13 @@ def add_batch_config( | |||
| batch_config = BatchConfig(name=name, partitioner=partitioner) | |||
| batch_config.set_data_asset(self) | |||
| self.batch_configs.append(batch_config) | |||
| self.update_batch_config_field_set() | |||
There was a problem hiding this comment.
We now need to always remember to call this after each update to self.batch_configs? Is there a better way to do this without putting the burden on future devs?
There was a problem hiding this comment.
cc @tyler-hoffman - this was a bugfix he snuck in, since the context was in this PR
There was a problem hiding this comment.
i've added a ticket, and will have a solution as a followup 🙇
| @override | ||
| def get_batch_request_options_keys(self, partitioner): | ||
| def get_batch_request_options_keys(self, partitioner: Optional[Partitioner] = None): |
There was a problem hiding this comment.
I was under the impression that we got this typing from the parent class, so adding types here is redundant. But if i'm wrong about that, I'm happy to update!
There was a problem hiding this comment.
i gave this some more thought, and i think you're right, even though the type is provided by the parent class, it's easiest to read if its also here. will update, thanks! 👍
| @@ -1056,23 +890,6 @@ def test_connection(self) -> None: | |||
| f"failed. Ensure the table exists and the user has access to select data from the table: {query_error}" | |||
| ) from query_error | |||
|
|
|||
| @override | |||
| def test_partitioner_connection(self) -> None: | |||
There was a problem hiding this comment.
Do we want to move this to the batch config (but probably not call it automatically)?
There was a problem hiding this comment.
this is only implemented for SQL partitioners, so to me it felt like a half baked feature. It would make more sense to me if we just had a BatchConfig.test_connection() method which checks the configuration, including the partitioner.
There was a problem hiding this comment.
Yeah, makes sense to me.
| @@ -176,36 +174,10 @@ def get_partitioner_implementation( | |||
| ) | |||
| return PartitionerClass(**abstract_partitioner.dict()) | |||
|
|
|||
| @property | |||
| @override | |||
| def batch_request_options( | |||
There was a problem hiding this comment.
Do we have a ticket to add this property to BatchConfigs and have them call this method on their asset?
invoke lint(usesblack+ruff)For more information about contributing, see Contribute.
After you submit your PR, keep the page open and monitor the statuses of the various checks made by our continuous integration process at the bottom of the page. Please fix any issues that come up and reach out on Slack if you need help. Thanks for contributing!