Skip to content
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(reader): handle header-only results #63

Merged
merged 4 commits into from
Feb 10, 2025

Conversation

mharrisb1
Copy link
Contributor

Closes #62

Issue was that single cell results are the same as header-only results. For header-only results we returned bind data too early without setting column name and types so when bind function was called by DuckDB it failed due to empty schema error.

This makes the following changes to avoid this:

  • Catch empty results and throw custom exception to tell user the exact cause of error
  • Avoid segfault when trying to access first row at index 1 when header-only results

The following tests were added to catch these issues in the future

  • Read sheet with single cell range (w/ headers)
  • Read sheet with single cell range (w/o headers)
  • Read header-only sheet
  • Read empty sheet

@mharrisb1 mharrisb1 marked this pull request as ready for review February 7, 2025 15:31
@archiewood
Copy link
Member

archiewood commented Feb 10, 2025

Played around with this, big improvement.

The only error that could be clearer is this one

from read_gsheet('https://docs.google.com/spreadsheets/d/11QdEasMWbETbFVxry-SsD8jVcdYIT1zBQszcF84MdE8/edit?gid=0#gid=0', range='d4');

Invalid Input Error:
Sheet Sheet1 is empty

In this case, Sheet1 is not empty, but the selected range is empty. I think users will be smart enough to work this out though, and feels like a low % edge case.

@archiewood archiewood merged commit cb2f5ca into evidence-dev:main Feb 10, 2025
22 checks passed
@mharrisb1
Copy link
Contributor Author

I'm about to work on ranged writes and can make this change then. Do you have a better alternative?

Maybe something about values being empty instead of sheet? That's really what this would be, the API fetch results values field is empty

@mharrisb1 mharrisb1 deleted the fix/62-single-cell-ranges branch February 10, 2025 17:28
@archiewood
Copy link
Member

archiewood commented Feb 10, 2025

How about something like this if the range is empty?

Error:
Range Sheet1!D1:D2 is empty

Else - we could do something less specific, which we could throw irrespective of whether the user has specified a range or not

Error:
The specified range in Sheet1 is empty

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Uncaught exception when using single cell ranges
2 participants