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

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 馃

@product-auto-label product-auto-label bot added the api: bigquery Issues related to the googleapis/python-bigquery-sqlalchemy API. label Aug 18, 2021
@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Aug 18, 2021
@jimfulton
Copy link
Contributor Author

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 August 20, 2021 14:03
@jimfulton jimfulton requested a review from a team as a code owner August 20, 2021 14:03
Copy link
Contributor

@tseaver tseaver left a comment

Choose a reason for hiding this comment

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

@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

Choose a reason for hiding this comment

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

Is there an issue open for addressing that gap?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right you are! I'll remove it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@jimfulton
Copy link
Contributor Author

@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

@tseaver are you cool with this?

@jimfulton jimfulton merged commit ad5baf8 into googleapis:master Aug 23, 2021
@jimfulton jimfulton deleted the fix-distinct-column-wrapper-188 branch August 23, 2021 19:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: bigquery Issues related to the googleapis/python-bigquery-sqlalchemy API. cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

distinct doesn't work as a column wrapper - no label is given
2 participants