feat(python): expose DatasetDeltaBuilder and relevant apis#5091
feat(python): expose DatasetDeltaBuilder and relevant apis#5091jackye1995 merged 8 commits intolance-format:mainfrom
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
yanghua
force-pushed
the
feat-py-delta
branch
from
82c9aef to
05e7dfa
Compare
| """ | ||
| return SqlQueryBuilder(self._ds.sql(sql)) | ||
|
|
||
| def delta(self) -> "DatasetDeltaBuilder": |
There was a problem hiding this comment.
I think python don't really have builders, what about we just have all the builder inputs as function arguments?
There was a problem hiding this comment.
After thinking about this suggestion. There are two opinions:
- There are some config options that have some check logic in
buildfn:
lance/rust/lance/src/dataset/delta.rs
Line 91 in b218725
Reusing the DatasetDeltaBuilder so that we can reuse the check logic?
- The build pattern could keep the scalarity for the future.
WDYT?
There was a problem hiding this comment.
what about having the inputs in the function, so it's functional style, and then we internally call into the binded builder within the function? So the DatasetDeltaBuilder is just internal usage and can be _DatasetDeltaBuilder
| builder = builder.with_begin_version(begin_version) | ||
| builder = builder.with_end_version(end_version) | ||
|
|
||
| return builder.build() |
There was a problem hiding this comment.
Agree DatasetDeltaBuilder could be _DatasetDeltaBuilder. Can we reuse the arg check in the builder.build() function?
There was a problem hiding this comment.
What do you mean? It should already use it?
There was a problem hiding this comment.
oh I see what you mean, you mean we don't need to duplicate the checks, agree
jackye1995
force-pushed
the
feat-py-delta
branch
from
df41d9d to
7447ab0
Compare
| has_compared_against = compared_against is not None | ||
| has_range = begin_version is not None or end_version is not None | ||
|
|
||
| if has_compared_against and has_range: | ||
| raise ValueError( | ||
| "Cannot specify both 'compared_against' and " | ||
| "'begin_version'/'end_version'. Use one or the other." | ||
| ) | ||
|
|
||
| if not has_compared_against and not has_range: | ||
| raise ValueError( | ||
| "Must specify either 'compared_against' or both " | ||
| "'begin_version' and 'end_version'." | ||
| ) | ||
|
|
||
| if has_range: | ||
| if begin_version is None or end_version is None: | ||
| raise ValueError( | ||
| "Both 'begin_version' and 'end_version' must be specified together." | ||
| ) |
There was a problem hiding this comment.
I mean, this block is not necessary, because it has been checked in Rust:
lance/rust/lance/src/dataset/delta.rs
Line 105 in b218725
And it could be reused for Python and Java?
…mat#5091) Co-authored-by: Jack Ye <yezhaoqin@gmail.com>
3 participants
No description provided.