Conversation
vbarua
left a comment
vbarua
left a comment
There was a problem hiding this comment.
Left some minor comments. Some of them are related to ambiguities in the spec around the DdlRel definition, which I don't consider blockers for this work.
|
|
||
| public abstract DdlOp getOperation(); | ||
|
|
||
| public abstract Optional<Rel> getViewDefinition(); |
There was a problem hiding this comment.
If I understand the core spec, this is intended as the definition for a view for CREATE VIEW style statements.
There's nothing indicating that it is optional, or that it should only be set when object DDL_OBJECT_VIEW, but I think that's consistent with the current definition.
| /** | ||
| * @return the table schema for the associated {@link ExtensionWrite} relation | ||
| */ | ||
| NamedStruct deriveSchema(); |
There was a problem hiding this comment.
Thinking about this, I don't think we actually need a deriveSchema method here, because the schema is given directly as the table_schema.
The same applies to the WriteExtensionObject as well.
What do you think about removing these?
| .tableDefaults(defaults) | ||
| .detail(detail) | ||
| .operation(ExtensionDdl.DdlOp.ALTER) | ||
| .object(ExtensionDdl.DdlObject.VIEW) |
There was a problem hiding this comment.
When we use VIEW do we need to set the viewDefinition as well?
There was a problem hiding this comment.
According to the docs we do (https://substrait.io/relations/logical_relations/#ddl-properties).
I set it on line 64.
Add the POJOs for the DdlRel proto message Similar pattern to the previous WriteRel bindings. Signed-off-by: Andrew Coleman <andrew_coleman@uk.ibm.com>
2 participants
Add the POJOs for the DdlRel proto message
Similar pattern to the previous WriteRel bindings.