-
Notifications
You must be signed in to change notification settings - Fork 138
feat: Feature/list tables page size #174
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
feat: Feature/list tables page size #174
Conversation
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed (or fixed any issues), please reply here with What to do if you already signed the CLAIndividual signers
Corporate signers
ℹ️ Googlers: Go here for more info. |
@googlebot I signed it! |
5a97fa7
to
085df0f
Compare
…peration as list_tables_page_size fix: arraysize default always takes priority over querystring
085df0f
to
5331bcd
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a great start. In addition to adding a feature, you found two bugs!
This needs test of the logic that fetches lists of tables. A unit test would be fine and should be straightforward to write.
hey, regarding this can you explain this logic to me? why shouldn't it always take the "full_table_id" property? gives us inconsistencies |
That's a good question. If you look at: https://github.com/sqlalchemy/sqlalchemy/blob/b20b6f8fe7ea0198f819a0fd68ca076b6c760054/lib/sqlalchemy/testing/suite/test_reflection.py#L537-L579 SQLAlchemy expects the results to be unqualified, which we satisfy if we have a schema. Looking at other dialects: MS SQL requires a schema: https://github.com/sqlalchemy/sqlalchemy/blob/b20b6f8fe7ea0198f819a0fd68ca076b6c760054/lib/sqlalchemy/dialects/mssql/base.py#L2877 Oracle assumes there's a default: https://github.com/sqlalchemy/sqlalchemy/blob/b20b6f8fe7ea0198f819a0fd68ca076b6c760054/lib/sqlalchemy/dialects/oracle/base.py#L1759-L1760 as does Postgres: https://github.com/sqlalchemy/sqlalchemy/blob/b20b6f8fe7ea0198f819a0fd68ca076b6c760054/lib/sqlalchemy/dialects/postgresql/base.py#L3488-L3490 As does sqlite, sort of: https://github.com/sqlalchemy/sqlalchemy/blob/b20b6f8fe7ea0198f819a0fd68ca076b6c760054/lib/sqlalchemy/dialects/sqlite/base.py#L1977-L1981 and so on. So BQ is a little different because there might not be a default. Amongst the options:
Playing with this some more, you can list schemas in other projects by specifying |
…peration as list_tables_page_size fix: arraysize default always takes priority over querystring
…riBromberg/python-bigquery-sqlalchemy into feature/list_tables_page_size
The bigquery client now accepts a When your;re ready for me to review, please merge master to get rid of many spurious changes. :) |
Thanks! |
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:
Fixes #173 🦕
Hey guys, I explained in the issue a bit but I wanted to implement it so users would be able to change max_results @ dataset.list_tables like the arraysize parameter so I copied how it is treated in the parse_url area and updated the tests and docs for it, the new parameter for it is called
list_tables_page_size
also I saw that on line 606
there is this expression
self.arraysize = self.arraysize or arraysize
which always takes the default arraysize instead of the parsed querystring arraysize because self.arraysize is always initialized to a default value, so I also reversed it to be as soself.arraysize = arraysize or self.arraysize
, now the querystring arraysize will take precedence over the default value