Skip to content
This repository was archived by the owner on Mar 6, 2026. It is now read-only.

fix: load_table_from_dataframe method for issue 1692 - #1698

Closed
Gaurang033 wants to merge 3 commits into
googleapis:mainfrom
Gaurang033:feature/fix_1692_load_table_from_dataframe
Closed

fix: load_table_from_dataframe method for issue 1692#1698
Gaurang033 wants to merge 3 commits into
googleapis:mainfrom
Gaurang033:feature/fix_1692_load_table_from_dataframe

Conversation

@Gaurang033

@Gaurang033Gaurang033 commented Oct 23, 2023

Copy link
Copy Markdown
Contributor

Fixes#1692

@Gaurang033
Gaurang033 requested review from a team and mrfaizalOctober 23, 2023 16:17
@product-auto-labelproduct-auto-labelBot added size: s Pull request size is small. api: bigquery Issues related to the googleapis/python-bigquery API. labels Oct 23, 2023
@dandhlee

dandhlee commented Oct 23, 2023

Copy link
Copy Markdown
Contributor

Please keep titles less than 50 characters.

dandhlee
dandhlee suggested changes Oct 23, 2023
View reviewed changes
Comment threadgoogle/cloud/bigquery/_pandas_helpers.py Outdated
Comment threadgoogle/cloud/bigquery/_pandas_helpers.py Outdated
Comment threadtests/unit/test_client.py Outdated
@Gaurang033
Gaurang033force-pushed the feature/fix_1692_load_table_from_dataframe branch 2 times, most recently from ed452fb to 6dd6a79CompareOctober 23, 2023 17:49
@Gaurang033Gaurang033 changed the title fix: load_table_from_dataframe does not error out when nan in a requi…fix: load_table_from_dataframe method for issue 1692Oct 23, 2023
dandhlee
dandhlee reviewed Oct 23, 2023
View reviewed changes
Comment threadtests/unit/test_client.py Outdated
@Gaurang033
Gaurang033force-pushed the feature/fix_1692_load_table_from_dataframe branch from 6dd6a79 to 98e568fCompareOctober 23, 2023 18:14
@Gaurang033

Gaurang033 commented Oct 26, 2023

Copy link
Copy Markdown
ContributorAuthor

@dandhlee could you please review the changes.

@Gaurang033
Gaurang033force-pushed the feature/fix_1692_load_table_from_dataframe branch 2 times, most recently from f123095 to d06a2acCompareOctober 31, 2023 23:50
@LinchinLinchin added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Nov 2, 2023
@yoshi-kokoroyoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Nov 2, 2023
@LinchinLinchin added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Nov 2, 2023
@yoshi-kokoroyoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Nov 2, 2023
@Gaurang033
Gaurang033force-pushed the feature/fix_1692_load_table_from_dataframe branch from afe6a01 to a731061CompareNovember 3, 2023 21:12
@Gaurang033
Gaurang033force-pushed the feature/fix_1692_load_table_from_dataframe branch from a731061 to 3a57815CompareNovember 3, 2023 21:12
@LinchinLinchin added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Nov 6, 2023
@yoshi-kokoroyoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Nov 6, 2023
Linchin
Linchin reviewed Nov 6, 2023
View reviewed changes
Comment threadgoogle/cloud/bigquery/_pandas_helpers.py

def _check_nullability(arrow_fields, dataframe):
"""Throws error if dataframe has null values and column doesn't allow nullable"""
if dataframe.index.name:

@LinchinLinchinNov 6, 2023•
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please help me understand what lines 307-308 are for?

@Gaurang033Gaurang033Nov 11, 2023

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's hard to let you know which exact code as I am on vacation. But when dataframe with index is used, index name is transformed as airow column name. There were two way to fix it. One was to put exception for this case or the create another column with index name. I choose the second option as it's easier. Without this the dataframe unit test case where they use index names will fail.

@LinchinLinchinNov 14, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you, hope you are having a good time in vacation! I played with dataframe's index a little bit, and I think there are several corner cases (which are likely non-comprehensive) that we need to cover:

  • Index doesn't have a name, does it still get converted into arrow?
  • Multiple index
  • Index with the same name as columns, which is possible with dataframes
  • Index columns have the same names (possible too)
  • multiindex

@chalmerlowechalmerloweNov 18, 2023

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Gaurang033 Thanks for offering up this PR.
@Linchin I appreciate this summary of additional edge cases that may not be covered by this solution.

I too worry about the edge cases, but more importantly, I worry about spending too much time and energy trying to create a work around for what we all agree is a problem in pyarrow. This feels like it creates greater complexity in our code, increased fragility, and a higher maintenance burden in the long run. Am I missing something?

@LinchinLinchinNov 20, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am also uncertain if we should add logic in our repo to correct an issue with pyarrow. I have been thinking about this PR as more of a temporary patchwork that maybe reverted later, but for now does help our customers. However if the logic covering the corner cases get too convoluted with the behaviors of pyarrow, I agree that perhaps it's a better idea to open an issue with pyarrow instead.

@chalmerlowechalmerlowe mentioned this pull request Mar 2, 2026
Closed
@LinchinLinchin added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Nov 14, 2023
@yoshi-kokoroyoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Nov 14, 2023
@Linchin
Linchin removed the request for review from mrfaizalNovember 14, 2023 19:57
@chalmerlowechalmerlowe mentioned this pull request Nov 19, 2023
Closed
@tswast

tswast commented Nov 22, 2023

Copy link
Copy Markdown
Contributor

Let's just let the server-side determine if we aren't matching the correct schema. I propose #1735 instead.

@tswasttswast closed this Nov 22, 2023
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@dandhleedandhleeAwaiting requested review from dandhlee
2 more reviewers
@chalmerlowechalmerlowechalmerlowe left review comments
@LinchinLinchinLinchin left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

api: bigqueryIssues related to the googleapis/python-bigquery API.size: sPull request size is small.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

load_table_from_dataframe does not error out when nan in a required column - Million dollar bug

6 participants

@Gaurang033@dandhlee@tswast@chalmerlowe@Linchin@yoshi-kokoro