-
Notifications
You must be signed in to change notification settings - Fork 427
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: Add support for creation of streams on external tables #999
feat: Add support for creation of streams on external tables #999
Conversation
oops sorry! Thanks for the reminder! |
/ok-to-test sha=c4214d2 |
Integration tests failure for c4214d2 |
c4214d2
to
c1a2d10
Compare
/ok-to-test sha=c1a2d10 |
Integration tests failure for c1a2d10 |
c1a2d10
to
fc91cab
Compare
/ok-to-test sha=ce964ea |
Integration tests failure for ce964ea |
/ok-to-test sha=b1fc73b |
Integration tests failure for b1fc73b |
@adamantike I really like this, but request a change: When you do 'show tables like 'TABLE_NAME'; there is an is_external column which indicates if it is external or not. So instead of introducing a new boolean variable, just read the table and determine whether it is external or no. |
b1fc73b
to
9b4d2bb
Compare
@sfc-gh-swinkler, I really liked your recommendation, the implementation is cleaner and simpler in comparison! I have refactored the PR to go with that approach, and would appreciate a review to know if the table information retrieval is using the function you expected. |
9b4d2bb
to
6b46356
Compare
/ok-to-test sha=9cc539d |
Integration tests failure for 9cc539d |
Hi @adamantike can you please fix? One of acceptance tests is failing |
Allow the creation of `STREAMS` on `EXTERNAL TABLE`s.
9cc539d
to
b6a786b
Compare
I have added a fix for that syntax error, but it seems there's no way to test external tables within acceptance tests at the moment. Fixing that, and disabling
I found that the other acceptance tests for External tables are behind a flag, so I duplicated that same behavior here. (reference: #426) |
/ok-to-test sha=b6a786b |
Integration tests success for b6a786b |
Now that acceptance tests are also fixed, I'm open to any feedback on this PR, so it can be merged! |
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.
LGTM
@adamantike thanks for your contribution! This is great 👍 |
Allow the creation of
STREAMS
onEXTERNAL TABLE
s.Test Plan
References