-
Notifications
You must be signed in to change notification settings - Fork 15
Add create table support, venice deployer, and various fixes #182
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
Conversation
71e7c4f to
8311ecd
Compare
ryannedolan
left a comment
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.
Really nice tests!
|
|
||
| // TODO: Add support for populating new tables from a query as a one-time operation. | ||
| if (create.query != null) { | ||
| throw new DdlException(create, "Populating new tables is not currently supported."); |
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.
For us, I think CTAS would be the same as an MV, except without the job. Table triggers would work the same, and would backfill the table asynchronously. Maybe we can add REFRESH TABLE as well, which would make CTAS just a batch-only version of MV.
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.
Yea it is similar but notably CTAS is meant as a one off backfill where as a MV is meant to have a running job to keep it in constant sync.
I guess it could be a trigger refreshed when executed but then not really again?
| source = new Source(temporaryTable.databaseName(), tablePath, Collections.emptyMap()); | ||
| } | ||
| deployers = DeploymentService.deployers(source, connection); | ||
| logger.info("Deleting table {}", tableName); |
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.
Wondering how we prevent a table from getting deleted if it's used in a view. Maybe we should add a Validator to that effect at some point.
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.
Hmm that's interesting. Validating if an existing MV is sinking to the same table intended to be deleted is actually super easy. But if this table is used as the source of an MV, that's quite a bit harder today.
| } | ||
|
|
||
| /** | ||
| * Temporary table implementation used during CREATE TABLE to provide row type information |
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.
Smart
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 still don't love this tbh, this temporary table does become sticky and will likely cause problems if we are reusing connections. We could maybe have something in place that if a table resolves as a temporary table we force resolution of it?
| Object defaultValue = null; | ||
| // For unions containing null, defaults are specified in a specific way | ||
| if (innerField.isUnion() && innerField.isNullable()) { | ||
| defaultValue = Schema.Field.NULL_DEFAULT_VALUE; |
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.
TIL!
|
|
||
| public static <T extends Deployable> Collection<Deployer> deployers(T obj, Connection connection) { | ||
| return providers().stream() | ||
| Collection<DeployerProvider> providers = providers(); |
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.
This is interesting. If downstream plugins are extending from upstream plugins, maybe we should split our dependencies into e.g. venice-core and venice-plugin (or whatever names make sense), s.t. we can depend on venice-core and not get the SPI service file?
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.
Yea maybe, still some pros and cons. That'll get rid of some of the potential weirdness caused by only choosing subclasses (if there are any?) but still won't allow inheritance and overrides. Say we have this VeniceDeployer, we want to expose it internally but drop in a different ControllerClient implementation, we still wouldn't be able to extend this Deployer without pulling in the SPI (with that being said we could have a 3rd BaseClass of sorts existing in a different module). Idk both don't feel great to me.
|
|
||
| @Override | ||
| public void restore() { | ||
| // TODO: Restoration can be complicated |
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.
Maybe we can use connection.warn(msg) here.
d05859a to
0971ac1
Compare
This change implements the following:
CREATE TABLE VENICE."<my-store>".["null", "SqlType"]Left various TODOs for future enhancements