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

Multischema endpoint #158

Merged
merged 8 commits into from
Mar 13, 2024
Merged

Multischema endpoint #158

merged 8 commits into from
Mar 13, 2024

Conversation

ceesem
Copy link
Collaborator

@ceesem ceesem commented Mar 12, 2024

Adding the multischema download endpoints and using them in the tables/views interface. Wound up refactoring the materialization initialization to use multithreading for pinging the server for various information, but on the plus side the .tables and .views interfaces are populated at the same time with minor overhead cost. Also, added small tweaks that seem to improve tab-autocompletion for these services as well.

I think this is working, but there are some changes to client.materialize that could be significant if they don't work as expected, so a bit of caution is warranted.

@ceesem ceesem requested a review from fcollman March 12, 2024 08:59
@fcollman
Copy link
Collaborator

If some caution is needed perhaps it’s time we added some tests for this

@ceesem
Copy link
Collaborator Author

ceesem commented Mar 12, 2024

Agreed, will add tests before this gets merged. Although what I was concerned about should be caught by the materialization client tests, if I understand them correctly.

@ceesem
Copy link
Collaborator Author

ceesem commented Mar 12, 2024

Tests added for the table manager. Not perfect because they have to mock the metadata one gets from the server, but at least there's an example there to compare against.

assert 'allen_column_mtypes_v2' == qry.joins_kwargs.get('joins')[0][0]

def test_view_manager():
views = ViewManager(None, view_metadata=views_list, view_schema=views_schema)
Copy link
Collaborator

Choose a reason for hiding this comment

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

this should be done with responses so it tests that portion of the code

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fair, changed now.

@fcollman fcollman merged commit d80dde9 into master Mar 13, 2024
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants