-
Notifications
You must be signed in to change notification settings - Fork 541
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
fix: disallow empty struct #8876
base: main
Are you sure you want to change the base?
Conversation
0bc84c5
to
2d56523
Compare
It seems to me like IDK, maybe we could make ibis lenient, but it feels like a slippery slope. If we allow this, then it feels like we should make it so that |
6bef829
to
1a4d1d0
Compare
1a4d1d0
to
bbfea30
Compare
if isinstance(df, dd.DataFrame): | ||
df = df.compute() | ||
self.schemas[table_name] = schema = PandasData.infer_table(df) |
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.
The original code was breaking a bunch of tests because in one of the datasets we're using there is a struct column that looks like [{}, None, {1: "a", 2: "b"}]
. We need all of the data in that column in order to infer the struct type there...
working towards ibis-project#8289 I'm not sure how useful empty structs are, since it seems like not many backends actually support them. But still, if you stay in ibis-land, perhaps it is useful. Not that hard for us to support it, so why not. I'm not sure of the history of the specific disallowment that I am removing from the type inference. Relevant context: - ibis-project#8876 - https://github.com/ibis-project/ibis/issues?q=empty+struct
working towards ibis-project#8289 I'm not sure how useful empty structs are, since it seems like only bigquery, dask, and pandas actually support them. But still, if you stay in ibis-land, perhaps it is useful. ie for doing type manipulations, or maybe you only use them for intermediate calculations? Not that hard for us to support it, so why not. I'm not sure of the history of the specific disallowment that I am removing from the type inference. Relevant context: - ibis-project#8876 - https://github.com/ibis-project/ibis/issues?q=empty+struct
working towards ibis-project#8289 I'm not sure how useful empty structs are, since it seems like only bigquery, dask, and pandas actually support them. But still, if you stay in ibis-land, perhaps it is useful. ie for doing type manipulations, or maybe you only use them for intermediate calculations? Not that hard for us to support it, so why not. I'm not sure of the history of the specific disallowment that I am removing from the type inference. Relevant context: - ibis-project#8876 - https://github.com/ibis-project/ibis/issues?q=empty+struct
Description of changes
Disallow empty struct in type inference. Pyarrow allows the construction of empty structs but duckdb doesn't.
I think what the user in #8460 was attempting to create was an empty value in a struct column that has defined fields. You can accomplish this with the following:
I do want to note here that ibis interprets
{}
as a struct whose keys point to empty values rather than a struct that is empty:Issues closed
#8460