-
Notifications
You must be signed in to change notification settings - Fork 245
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
lance dataset that overrides Dataset.scanner and Dataset.head #158
Conversation
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.
Minor nits
python/lance/_lib.pyx
Outdated
Dataset, | ||
FileFormat, | ||
FileWriteOptions, | ||
CDataset, |
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.
this indent is weird.
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.
fixed. not sure why pycharm is doing that
scanner = self.scanner(limit=n, offset=offset) | ||
return scanner.to_table() | ||
|
||
def scanner(self, *args, **kwargs): |
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.
Are we going to deprecate lance.scanner()
? Could be just remove it from this PR.
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.
makes sense. Can just do lance.dataset(...).scanner(...)
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.
fixed
@@ -36,6 +36,16 @@ def test_simple_round_trips(tmp_path: Path): | |||
assert table == actual | |||
|
|||
|
|||
def test_head(tmp_path: Path): |
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.
can we add a test to make sure duckdb works with this dataset
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.
there is an existing unit test that runs a duckdb query (how i discovered the problem originally)
closes #141