-
Notifications
You must be signed in to change notification settings - Fork 34
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(ibis): Implement Snowflake Metadata APIs #895
base: main
Are you sure you want to change the base?
Conversation
Hi @grieve54706 @goldmedal, could you take a look at my PR when you have some free time? I'd really appreciate your feedback, thanks in advance! |
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.
Thanks @ongdisheng. Overall looks good to me 👍 . Let's wait for @grieve54706 to review it.
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.
Well done. Thanks for your contribution. Welcome @ongdisheng.
url=f"{base_url}/metadata/tables", | ||
json={"connectionInfo": connection_info}, | ||
) | ||
assert response.status_code == 200 |
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.
Please add more expected to like test_trino
.
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.
I’ve updated the test. Could you please take a look when you have time and let me know if I’ve done it correctly? Thanks @grieve54706!
ibis-server/tests/routers/v3/connector/snowflake/test_functions.py
Outdated
Show resolved
Hide resolved
@goldmedal @grieve54706, thanks for the quick feedback! I’ll have the fixes ready later today. |
5a9c329
to
377b455
Compare
def test_metadata_list_constraints(): | ||
pass | ||
response = client.post( | ||
url=f"{base_url}/metadata/constraints", | ||
json={"connectionInfo": connection_info}, | ||
) | ||
assert response.status_code == 200 |
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.
Hi @ongdisheng,
Could you add expected like test_metadata_list_tables
too?
You did a good job. Thanks for your help.
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.
Hi @grieve54706, I've noticed that the TPCH_SF1
schema doesn’t seem to enforce foreign key constraints, which causes the SHOW IMPORTED KEYS
command to return an empty list.
Here are the results I found when querying the TABLE_CONSTRAINTS
:
Given this, would it be appropriate to add an assertion to check that the result returns an empty list? I’d be glad to hear any other suggestions you may have. Thank you!
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.
Yes, asserting it is an empty list is a good choice.
39f0af9
to
7113489
Compare
Implement Snowflake Metadata APIs #709
(POST) /v2/connector/snowflake/metadata/tables
Request payload example
Response example
(POST) /v2/connector/snowflake/metadata/constraints
Request payload example
Response example
({constraint_table}_{constraint_column}_{constrainted_table}_{constrainted_column})