-
Notifications
You must be signed in to change notification settings - Fork 1.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[MAINTENANCE] Remove fluent partitioner methods from DataAssets #9517
Changes from 111 commits
c3969dd
c9d892a
3cc0639
36239d9
4854727
19164a2
8bf119b
e695d19
a167db1
50994e7
e20b62e
307263b
5032352
8193139
fa5cda8
9218fc9
f8780c2
c7e9103
29e28af
c0d2c1a
af5e0d9
8795e5f
9d2bdf5
f9b2210
89f7193
4b405a0
7bb9777
1c0c5d7
baff01e
364dfaa
f0d2f52
d626182
a4e24b4
269089e
f00a1fb
dbd95d0
1004b30
8623707
ac3ee51
85f072a
9ed5cee
c2ba4c7
fbf7657
abc76d5
0cf9e3a
de6fc0c
454aef8
7a9fafa
6edf63f
7da1552
aa244ab
d0b3961
ab1149f
e2e4d16
d801d17
f97c441
d88a277
d0ff1ca
b924f97
7b7247f
3362915
7611ecb
46a5c3e
ff5abdf
b38fa01
4953def
5cf5c9e
519867e
6137d92
ec0e7f1
dc32b3b
5b188d9
795c831
3e708f8
2509852
1ae3497
990cb11
d91e36d
6c964ad
e7d28e3
b19afcf
e6e777c
175814b
53d8c89
4d3b6ee
344bedc
4eab300
4b0651e
43888ed
9d42d57
cb0987c
0b63506
3a4b906
52e4f63
dbd18ac
3dac9c9
9c1134e
8ad0efc
17a32b7
c6f3772
f34d8d7
854d523
c7a899b
d79245c
b666761
82ac959
b020097
a3bc3b5
e8954fe
8b237f6
31b6674
b4be3df
ae5bb0d
9198fe6
e9d1fde
18dec3f
212de7e
9d3bfd5
92b7b0f
4720bd0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -208,28 +208,8 @@ def test_connection(self) -> None: | |
"""One needs to implement "test_connection" on a DataAsset subclass.""" | ||
) | ||
|
||
# Abstract Methods | ||
@property | ||
def batch_request_options(self) -> tuple[str, ...]: | ||
"""The potential keys for BatchRequestOptions. | ||
|
||
Example: | ||
```python | ||
>>> print(asset.batch_request_options) | ||
("day", "month", "year") | ||
>>> options = {"year": "2023"} | ||
>>> batch_request = asset.build_batch_request(options=options) | ||
``` | ||
|
||
Returns: | ||
A tuple of keys that can be used in a BatchRequestOptions dictionary. | ||
""" | ||
raise NotImplementedError( | ||
"""One needs to implement "batch_request_options" on a DataAsset subclass.""" | ||
) | ||
|
||
def get_batch_request_options_keys( | ||
self, partitioner: Optional[Partitioner] | ||
self, partitioner: Optional[Partitioner] = None | ||
) -> tuple[str, ...]: | ||
raise NotImplementedError( | ||
"""One needs to implement "get_batch_request_options_keys" on a DataAsset subclass.""" | ||
|
@@ -246,7 +226,7 @@ def build_batch_request( | |
Args: | ||
options: A dict that can be used to filter the batch groups returned from the asset. | ||
The dict structure depends on the asset type. The available keys for dict can be obtained by | ||
calling batch_request_options. | ||
calling get_batch_request_options_keys(...). | ||
batch_slice: A python slice that can be used to limit the sorted batches by index. | ||
e.g. `batch_slice = "[-5:]"` will request only the last 5 batches after the options filter is applied. | ||
partitioner: A Partitioner used to narrow the data returned from the asset. | ||
|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We now need to always remember to call this after each update to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. cc @tyler-hoffman - this was a bugfix he snuck in, since the context was in this PR There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i've added a ticket, and will have a solution as a followup 🙇 |
||
if self.datasource.data_context: | ||
try: | ||
batch_config = self.datasource.add_batch_config(batch_config) | ||
except Exception: | ||
self.batch_configs.remove(batch_config) | ||
self.update_batch_config_field_set() | ||
raise | ||
self.update_batch_config_field_set() | ||
return batch_config | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -114,13 +114,8 @@ def _get_reader_method(self) -> str: | |
def test_connection(self) -> None: | ||
... | ||
|
||
@property | ||
@override | ||
def batch_request_options(self) -> tuple[str, ...]: | ||
return tuple() | ||
|
||
@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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we annotate the return? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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! 👍 |
||
return tuple() | ||
|
||
@override | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we have a ticket to add this property to BatchConfigs and have them call this method on their asset?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
v1-205 👍