[FEATURE] DataAsset uses partitioner from BatchConfig #9499
Conversation
…75/asset_uses_partitioner_from_batch_request
✅ Deploy Preview for niobium-lead-7998 canceled.
|
| method_name: Literal[ | ||
| "partition_on_converted_datetime" | ||
| ] = "partition_on_converted_datetime" | ||
| date_format_string: str |
There was a problem hiding this comment.
Should we do any validation on this string?
There was a problem hiding this comment.
current behavior is to not validate the string, so i think it's likely okay to not. I'm not 100% how we would validate it, either, since its correctness depends on the shape of the source data.
| raise ValueError( | ||
| f"Requested Partitioner `{abstract_partitioner.method_name}` is not implemented for this DataAsset. " | ||
| ) | ||
| assert PartitionerClass is not None |
There was a problem hiding this comment.
i originally added this because mypy wasn't correctly catching that the exception above narrows the type to non null, but i'll double check 🙇
There was a problem hiding this comment.
removed in 4b0651e
| @@ -45,5 +45,10 @@ def build_batch_request( | |||
| options=batch_request_options, partitioner=self.partitioner | |||
| ) | |||
|
|
|||
| def batch_request_option_keys(self) -> tuple[str, ...]: | |||
There was a problem hiding this comment.
Nit - should this be prefixed with get_ for consistency?
There was a problem hiding this comment.
i'll remove this method entirely. it was meant as a user-facing replacement for the asset property batch_request_options, but let's evaluate if we actually want this, and then add it as a followup PR.
There was a problem hiding this comment.
removed in 9d42d57
tests/conftest.py
Outdated
| ).add_partitioner_column_value("event_type") | ||
|
|
||
| ) | ||
| # add_partitioner_column_value("event_type") |
There was a problem hiding this comment.
Should this be updated to add an actual partitioner?
There was a problem hiding this comment.
we can no longer add partitioners at the asset level, so it needs to be added where this fixture is used. i can remove this 👍
There was a problem hiding this comment.
removed in 43888ed
joshua-stauffer
deleted the
f/v1-175/asset_uses_partitioner_from_batch_request
branch
This PR updates the DataAsset to use Partitioners defined in the BatchRequest rather than the Partitioner defined on the DataAsset. This change is required since a single asset can now have multiple Partitioners, as defined by multiple BatchConfigs.
changes
deprecates
DataAsset.batch_request_options. Functionality is now availableBatchConfig.get_batch_request_options(), which invokesDataAsset.get_batch_request_options_keys(...). This has been changed from a property to a method because it potentially triggers a query of the data.PartitionOnConvertedDatetimeis moved alongside the rest of sql partitioners to allow it to join theSqlPartitionerunion, with the expectation that it will be implemented for other sql backends in the future. This is a workaround to pydantic not allowing child classes to override types from parent classes. Specifically,SqliteTableAssetandSqliteQueryAssetneed to be able to add their implementation ofPartitionerConvertedDateTimeto the_partitioner_implementation_map.Description of PR changes above includes a link to an existing GitHub issue
PR title is prefixed with one of: [BUGFIX], [FEATURE], [DOCS], [MAINTENANCE], [CONTRIB]
Code is linted - run
invoke lint(usesblack+ruff)Appropriate tests and docs have been updated
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!