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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: distinct doesn't work as a column wrapper #275

Merged

Conversation

@jimfulton
Copy link
Contributor

@jimfulton jimfulton commented Aug 18, 2021

Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:

  • Make sure to open an issue as a bug/issue before writing your code! That way we can discuss the change, evaluate designs, and agree on the general idea
  • Ensure the tests and linter pass
  • Code coverage does not decrease (if any source code was changed)
  • Appropriate docs were updated (if necessary)

Fixes #188 馃

@jimfulton
Copy link
Contributor Author

@jimfulton jimfulton commented Aug 20, 2021

Note to reviewers:

This bug occurred because the dialect copies visit_column from SQLAlchemy. See: #274.

The fix was to sync up with the version from sqlalchemy, to the extent possible given that it differs across different versions of SQLAlchemy.

5e4074d#diff-efe1987a73ae5b54aa817dc8c70541025df89d9d08601c8ab33fa84ad8c09de7R266-R271 in particular. This is needed so SQLAlchemy knows how to map subexpressions in queries (e.g. distinct) to columns.

The long term fix will be to stop copying this method.

@jimfulton jimfulton marked this pull request as ready for review Aug 20, 2021
@jimfulton jimfulton requested a review from as a code owner Aug 20, 2021
Copy link
Contributor

@tseaver tseaver left a comment

@jimfulton wrote:

The long term fix will be to stop copying this method.

Is there an issue open to track the long-term fix?


if is_literal:
# note we are not currently accommodating for
# literal_column(quoted_name('ident', True)) here
Copy link
Contributor

@tseaver tseaver Aug 20, 2021

Is there an issue open for addressing that gap?

Copy link
Contributor Author

@jimfulton jimfulton Aug 20, 2021

That comment is part of the copy from sqlalchemy. I'll delete it. :/

Copy link
Contributor Author

@jimfulton jimfulton Aug 20, 2021

done

@@ -691,3 +691,37 @@ def test_has_table(engine, engine_using_test_dataset, bigquery_dataset):
assert engine_using_test_dataset.has_table(f"{bigquery_dataset}.sample") is True

assert engine_using_test_dataset.has_table("sample_alt") is False


def test_distinct_188(engine, bigquery_dataset, metadata):
Copy link
Contributor

@tseaver tseaver Aug 20, 2021

The metadata fixture doesn't look to be used here.

Copy link
Contributor Author

@jimfulton jimfulton Aug 20, 2021

Right you are! I'll remove it.

Copy link
Contributor Author

@jimfulton jimfulton Aug 20, 2021

done

@jimfulton
Copy link
Contributor Author

@jimfulton jimfulton commented Aug 20, 2021

@jimfulton wrote:

The long term fix will be to stop copying this method.

Is there an issue open to track the long-term fix?

#274

@jimfulton
Copy link
Contributor Author

@jimfulton jimfulton commented Aug 23, 2021

@tseaver are you cool with this?

@jimfulton jimfulton merged commit ad5baf8 into googleapis:master Aug 23, 2021
9 checks passed
@jimfulton jimfulton deleted the fix-distinct-column-wrapper-188 branch Aug 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

2 participants