-
Notifications
You must be signed in to change notification settings - Fork 124
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
feat: pyspark and duckdb selectors #1853
Conversation
@@ -96,7 +88,7 @@ def test_set_ops( | |||
expected: list[str], | |||
request: pytest.FixtureRequest, | |||
) -> None: | |||
if ("pyspark" in str(constructor)) or "duckdb" in str(constructor): | |||
if "duckdb" in str(constructor) and not expected: |
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.
To figure this out due to:
# TODO(marco): return empty relation with 0 columns?
return self._from_native_frame(self._native_frame.limit(0))
took me some sanity π
I tried in different ways to create an empty table but duckdb doesn't really like that. Maybe we might just raise an error?
dtypes = import_dtypes_module(self._version) | ||
return self.by_dtype( | ||
[ | ||
dtypes.Int64, |
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 need dtypes.Int128 in here too now?
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.
Nice catch!
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.
Should it be added everywhere?
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.
yes, probably
nice, thanks - i'm just in the middle of a big refactor, so i'll finish that, then i'll update these |
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.
thanks @FBruzzesi !
What type of PR is this? (check all applicable)
Checklist
If you have comments or can explain your changes, please do so below