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(bigquery): escape table names with spaces for bigquery backend #9589

Merged
merged 4 commits into from
Jul 16, 2024

Conversation

akanz1
Copy link
Contributor

@akanz1 akanz1 commented Jul 15, 2024

Description of changes

bigquery docs "[Identifiers] Must be enclosed by backtick (`) characters."

  • enclose table names with backticks to avoid failures when spaces are included
  • remove L.586 as it appears to be redundant (see L.551)
  • fix typo

Issues closed

Copy link
Member

@gforsyth gforsyth left a comment

Choose a reason for hiding this comment

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

Hey @akanz1, thanks for the bug report and the PR!!

Two questions for you:

  1. Can you add a simple test for this? I think creating a table with a space in the name, and then demonstrating that we can grab a reference to it is sufficient.
  2. Should we include a check that name isn't already quoted with backticks? I don't think double-backticks are valid (or at least sqlglot doesn't like them)

ibis/backends/bigquery/__init__.py Outdated Show resolved Hide resolved
@cpcloud cpcloud dismissed their stale review July 15, 2024 15:43

Invalid reason for requesting changes

@cpcloud
Copy link
Member

cpcloud commented Jul 15, 2024

I don't think we should include a check for already quoted things. We don't allow backend specific quoting for name in any backends AFAIK. If we do, we should fix that :)

@akanz1
Copy link
Contributor Author

akanz1 commented Jul 15, 2024

Hey @akanz1, thanks for the bug report and the PR!!

Two questions for you:

  1. Can you add a simple test for this? I think creating a table with a space in the name, and then demonstrating that we can grab a reference to it is sufficient.
  2. Should we include a check that name isn't already quoted with backticks? I don't think double-backticks are valid (or at least sqlglot doesn't like them)

I'll probably need to go through the projects dev setup and have a look at how you test similar cases. If you can point me to an example that would be great, I'll likely get to it later this week.

@gforsyth
Copy link
Member

Hi @akanz1 -- we have a test for Bigquery for creating temp tables -- you can look at ibis/backends/bigquery/tests/system/test_client.py::test_create_temp_table_from_scratch

You can do something similar there, but with the name being something like f"spacetest {gen_name("bigquery_space_table")}"

That will confirm that we can create tables that have spaces in the names (might need to add handling for that to create_table), and then you can add an additional check of grabbing the table object with t = con.table(f"spacetest {gen_name("bigquery_space_table")}"

You will also want to set the env-var $PROJECT_ID_ENV_VAR to a bigquery project id that you have credentials for, otherwise it will default to the one used by our test suite, which will throw an auth error.

@cpcloud cpcloud added bug Incorrect behavior inside of ibis bigquery The BigQuery backend labels Jul 16, 2024
@cpcloud cpcloud added this to the 9.2 milestone Jul 16, 2024
@cpcloud
Copy link
Member

cpcloud commented Jul 16, 2024

Pushed up a test so we can get this out in 9.2

@cpcloud
Copy link
Member

cpcloud commented Jul 16, 2024

BigQuery tests are passing:

…/ibis on  akanz1/main:main is 📦 v9.1.0 via 🐍 v3.10.14 via ❄️   impure (ibis-3.10.14-env)
❯ pytest -m bigquery -n auto --dist loadgroup -q
bringing up nodes...
...........................................................x.....x.....sssssssssssssssssssssssssssx......................x...............sssssssssssssssssssss..............................xx [  9%]
xxx...x..x.xx.xx.xxx.xxxxxxxxxxxxxxxx.xxxxxxxxx.xxx.xxx.xxxxxx.xxxxxxxxxxxxxxxxxxxx.xxxxxxxxxxxxx.x.xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx.xx..xx......xx......x..x..xx.......x..... [ 19%]
....................x.......x.xxx.x..xx..xx.xx..x...xx....x.......x.......x.....xx.x...x.x..x.x.......xx.xx.xxxx.....xx.xx..x..xx..x.x.x.x..x.x........x.x...........xxx..............x....... [ 28%]
x....x.....x...x.x.......x........x...x...........................x.x....................x.......x..........xx...............x.x..........x...x.....x......xx...xx.....x...........x.......x.. [ 38%]
.................x......x..........................x...............xx.............x..x...x.......x.x....x.....x...........xx.......................x..........x......x.x....x.x..x......x.x.x. [ 47%]
....x.........x.................x...x..........x.x.....x.....xx.......x...x...x.......x.............x.......x...x..............x....xx..xx............x....x.x.x............x.............x.x. [ 57%]
.x.x...x.......xx..x........xx.x.x..........x........x......x...x..x.....x.......x..x..........x.......x............x.xx...x..x.....xx......x.x.........xx............x..x.x.........x..x..x.. [ 66%]
......x....x.......s.x...x.x.....x.......x..x...........x..s..x..s........xxs..........x..........x..x..s......................x..............ss...................s......x................... [ 76%]
..............................................x...................x........................................................................................................................... [ 85%]
............................................................................................................x................................................................................. [ 95%]
...................................................x.....................................                                                                                                      [100%]
1597 passed, 56 skipped, 336 xfailed in 362.23s (0:06:02)

@cpcloud cpcloud enabled auto-merge (squash) July 16, 2024 15:22
@cpcloud
Copy link
Member

cpcloud commented Jul 16, 2024

This only touches the BigQuery backend, and the bigquery compiler tests pass, so merging!

@cpcloud cpcloud disabled auto-merge July 16, 2024 15:30
@cpcloud cpcloud merged commit ca21dbb into ibis-project:main Jul 16, 2024
84 of 85 checks passed
@cpcloud
Copy link
Member

cpcloud commented Jul 16, 2024

@akanz1 Thanks for the PR!

@akanz1
Copy link
Contributor Author

akanz1 commented Jul 17, 2024

Awesome, thanks a lot guys!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bigquery The BigQuery backend bug Incorrect behavior inside of ibis
Projects
None yet
Development

Successfully merging this pull request may close these issues.

bug: bigquery backend fails for tables with spaces in table name
3 participants