-
Notifications
You must be signed in to change notification settings - Fork 152
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: SQL Catalog - Tables #610
feat: SQL Catalog - Tables #610
Conversation
Signed-off-by: callum-ryan <19956159+callum-ryan@users.noreply.github.com>
Signed-off-by: callum-ryan <19956159+callum-ryan@users.noreply.github.com>
Signed-off-by: callum-ryan <19956159+callum-ryan@users.noreply.github.com>
Signed-off-by: callum-ryan <19956159+callum-ryan@users.noreply.github.com>
Signed-off-by: callum-ryan <19956159+callum-ryan@users.noreply.github.com>
3de1771
to
8229ea8
Compare
Signed-off-by: callum-ryan <19956159+callum-ryan@users.noreply.github.com>
Signed-off-by: callum-ryan <19956159+callum-ryan@users.noreply.github.com>
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 @callum-ryan for this great pr! Generally LGTM, I've left some minor points to fix, others are great! One thing I want to ask is that do we have plan for tests against other databases such as postgres, mysql?
Signed-off-by: callum-ryan <19956159+callum-ryan@users.noreply.github.com>
Thanks @liurenjie1024, I have fixed the points raised. I'm happy to take a look at integration tests for PostgreSQL and MySQL, it will take me some extra time as I am unfamiliar with the way these tests run for the repo. The REST catalog tests seem to be doing what I'd need to spin up some containers with the appropriate backends. |
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 @callum-ryan !
this is a WIP. Pulling tests in from @fqaiser94's original recommended test set as I go.