feat(canner): implement new data source canner
#823
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
grieve54706
requested review from
goldmedal
and removed request for
goldmedal
grieve54706
force-pushed
the
feature/canner-enterprise
branch
from
61f40b2 to
83629e2
Compare
grieve54706
force-pushed
the
feature/canner-enterprise
branch
from
71159d0 to
04faf51
Compare
github-actions
bot
added
ibis
dependencies
goldmedal
approved these changes
Oct 22, 2024
There was a problem hiding this comment.
Thanks @grieve54706 overall looks good to me. Only some minor comments.
Comment on lines
17
to
18
| - The table `orders` must be with a comment `This is a table comment` | ||
| - The table `orders` must have a column `o_comment` with a comment `This is a comment` |
There was a problem hiding this comment.
Suggested change
| - The table `orders` must be with a comment `This is a table comment` | |
| - The table `orders` must have a column `o_comment` with a comment `This is a comment` | |
| - The table `orders` must be with a description `This is a table comment` | |
| - The table `orders` must have a column `o_comment` with a description `This is a comment` |
In the canner side, I think it's called description.
|
|
||
| pytestmark = pytest.mark.canner | ||
|
|
||
| client = TestClient(app) |
There was a problem hiding this comment.
I think we can use the with client clause here, too.
Comment on lines
49
to
52
| "rls": { | ||
| "name": "SESSION_STATUS", | ||
| "operator": "EQUALS", | ||
| }, |
There was a problem hiding this comment.
Unused field.
Suggested change
| "rls": { | |
| "name": "SESSION_STATUS", | |
| "operator": "EQUALS", | |
| }, |
grieve54706
added a commit
that referenced
this pull request
Dec 13, 2024
* feat(canner): implement new data source `canner` * feat(canner): get type name via query and cache the result * test(canner): add test case and fix dry_run * feat(canner): implement metadata * chore(ci): not test canner * chore(poetry): update * test(canner): adjust test * chore: remove unused * chore(test): move test order * chore(canner): move comment * chore(canner): adjust method _oid_to_ibis_type to _to_ibis_type * chore(canner): improve readability * chore(comment): use description instead of comment to fit canner enterprise design * test(canner): adjust the pattern * test(canner): remove unused
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Support new data source Canner Enterprise. It provides the PG wire protocol, so we can connect it by following Postgres.
Connection info
Additional information
CREATE TEMPORARY VIEWto get the schema for ibis. We need to build schema bydry-run.dry-run, so we have to query with a limit of zeroGraphQL.