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.

Sign up for GitHub

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

Jump to bottom

fix(snowflake): ensure that quoting matches snowflake behavior, not sqlalchemy #5741

Merged
merged 1 commit into from
Mar 15, 2023
Merged

fix(snowflake): ensure that quoting matches snowflake behavior, not sqlalchemy #5741

merged 1 commit into from
Mar 15, 2023

Conversation

cpcloud
Copy link
Member

@cpcloud cpcloud commented Mar 14, 2023 •
edited
Loading

This PR addresses broken snowflake-sqlalchemy behavior that appears to have been stuck in open source limbo
for over three years. Numerous issues related to the snowflake-sqlalchemy dialect's broken quoting behavior exist:

with no obvious traction or clear effort towards addressing them.

So, in this PR I just disable snowflake-sqlalchemy's broken case-guessing and
let snowflake do what it will, and return what it will.

@cpcloud cpcloud added this to the 5.0 milestone Mar 14, 2023
@cpcloud cpcloud added bug Incorrect behavior inside of ibis snowflake The Snowflake backend labels Mar 14, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Mar 14, 2023 •
edited
Loading

Test Results

       51 files         51 suites   2h 42m 27s ⏱️
16 016 tests 12 009 ✔️   4 007 💤 0 ❌
60 394 runs  44 673 ✔️ 15 721 💤 0 ❌

Results for commit 11972be.

♻️ This comment has been updated with latest results.

@cpcloud cpcloud mentioned this pull request Mar 14, 2023
Merged
@gforsyth
Copy link
Member

Hmm, should we retarget this at #5736 to avoid the mismatch in the (nearly) standardized keyword arguments?

@cpcloud
Copy link
Member Author

cpcloud commented Mar 14, 2023

I'm okay rebasing once this PR is merged.

gforsyth
gforsyth approved these changes Mar 14, 2023
View reviewed changes
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.

:shipit:

@cpcloud cpcloud force-pushed the fix-snowflake-quoting branch 2 times, most recently from dd89275 to 2e2e1b2 Compare March 15, 2023 14:12
@cpcloud cpcloud mentioned this pull request Mar 15, 2023
Open
@cpcloud
Copy link
Member Author

cpcloud commented Mar 15, 2023

Cloud backends:

❯ pytest -m 'snowflake or bigquery' -n auto -q --dist loadgroup --snapshot-update
bringing up nodes...
.xxxxx..x...x....xxxxxxxx.xx....x..xx.x..x.xx.x..x.x...x....x.x.x.x.x..x....x.......x...........x...x.......x.. [  5%]
.x.......xx.......xx.xx..x...................x.............xx.........x...........x.........................x.. [ 10%]
.........x.....x.....x.........x..x..x..xx..x..x.x....x..x.x..xxx...x.....x.......xx......x...........x....x..x [ 16%]
.......x........x....x....x...xx......x.xx.x.x.x..x..xx.x.x.x.xxx.....x......x..x....x...xx..x.xxxx...x....x... [ 21%]
............x...........xx..xxxxx.......x..x..xx.....x..x........xx......x...........x.........x..x........x... [ 27%]
..x.x..........xx..........x.........................x......................................................... [ 32%]
.................x.........................x..xxxxx..x...xx.x.x.xx.x.xx.x.....xxx.x...x..x..x.........x......x. [ 38%]
....x..x...x..xx....x.......xx...................x.x.x....x.xxxx......x...x..x..................x.............. [ 43%]
.....................x...s....x.....x.........x...s...xx.........s.x..........xx.............x...........x..... [ 49%]
................x....................x............sx.............................x...............x......xx..... [ 54%]
...x...............................xx..x.....x.............xx.....x.....x.......x.........x..x...........x..x.. [ 60%]
.......x...x.....x...xx....x...x....xx.x....x.x......x.x.xx.x....x.xx.xxxx..x.x..........x..x..xx....x.......x. [ 65%]
xx.xx.......x..............x..x..x..........xx...x.x.x..xx...x.x.........x......x.........x.................... [ 71%]
.....x.........................x............................x...x..............x.....xx........................ [ 76%]
...x......x..xxx..xxx.xxx.x..x.xx.xx.xxxxxxx..x.x..x...x...............x..x................x..x................ [ 82%]
........................x.x....x........................................................x.....x......x...x..... [ 87%]
............x............s..ss..s...x...........x............................................x................. [ 93%]
.......................................................................xxx.x..xx........................ss.xs.. [ 98%]
..x.......x...x.....x..                                                                                         [100%]
1679 passed, 11 skipped, 331 xfailed in 208.56s (0:03:28)

This was referenced Mar 15, 2023
Open
Open
jgehrcke
jgehrcke reviewed Mar 15, 2023
View reviewed changes
ibis/backends/base/sql/alchemy/__init__.py
@@ -413,12 +416,14 @@ def _log(self, sql):
util.log(query_str)

def _get_sqla_table(
self, name: str, schema: str | None = None, autoload: bool = True, **kwargs: Any
self, name: str, schema: str | None = None, autoload: bool = True, **_: Any
Copy link

Choose a reason for hiding this comment

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

I have always loved this naming convention ❤️

@cpcloud
Copy link
Member Author

cpcloud commented Mar 15, 2023

Merging.

@cpcloud cpcloud merged commit b6b67f9 into ibis-project:master Mar 15, 2023
@cpcloud cpcloud deleted the fix-snowflake-quoting branch March 15, 2023 15:22
@kklein kklein mentioned this pull request May 31, 2023
Open
@binste binste mentioned this pull request Aug 5, 2023
Closed
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jgehrcke jgehrcke jgehrcke left review comments

@gforsyth gforsyth gforsyth approved these changes

Assignees
No one assigned
Labels
bug Incorrect behavior inside of ibis snowflake The Snowflake backend
Projects
None yet
Milestone
5.0
Development

Successfully merging this pull request may close these issues.
3 participants
@cpcloud @gforsyth @jgehrcke