-
Notifications
You must be signed in to change notification settings - Fork 742
[YQ-1997] OR REPLACE grammar support for EXTERNAL DATA SOURCE and EXTERNAL TABLE #1318
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
|
⚪
|
|
⚪
|
ydb/library/yql/sql/v1/SQLv1.g.in
Outdated
| object_type_ref: an_id_or_type; | ||
|
|
||
| create_table_stmt: CREATE (TABLE | TABLESTORE | EXTERNAL TABLE) (IF NOT EXISTS)? simple_table_ref LPAREN create_table_entry (COMMA create_table_entry)* COMMA? RPAREN | ||
| create_table_stmt: CREATE (TABLE | TABLESTORE | EXTERNAL TABLE) (IF NOT EXISTS | OR REPLACE)? simple_table_ref LPAREN create_table_entry (COMMA create_table_entry)* COMMA? RPAREN |
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.
As I remember, we wanted syntax like CREATE [OR REPLACE] EXTERNAL TABLE ..., but here in this rule I see another syntax: CREATE EXTERNAL TABLE [OR REPLACE] .... I think that the first option is more proper from the point of view of the natural language
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.
Fixed
| namespace NObjectOptionsParsing { | ||
| Y_HAS_MEMBER(ExistingOk); // for create | ||
| Y_HAS_MEMBER(MissingOk); // for drop | ||
| Y_HAS_MEMBER(IsReplace); // for create |
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.
ReplaceIfExists. This is creation operation
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.
Fixed
ydb/core/protos/flat_scheme_op.proto
Outdated
| optional string Location = 6; | ||
| repeated TColumnDescription Columns = 7; | ||
| optional bytes Content = 8; | ||
| optional bool IsReplace = 9; |
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.
ReplaceIfExists.
But it has no sense for describe operation. Is it OK? Maybe we need different protos for describe and create operations?
At least comment for this field is required.
ydb/library/yql/sql/v1/node.h
Outdated
| std::map<TString, TDeferredAtom>&& features, const TObjectOperatorContext& context); | ||
| TNodePtr BuildCreateObjectOperation(TPosition pos, const TString& objectId, const TString& typeId, | ||
| bool existingOk, std::map<TString, TDeferredAtom>&& features, const TObjectOperatorContext& context); | ||
| bool existingOk, bool isReplace, std::map<TString, TDeferredAtom>&& features, const TObjectOperatorContext& context); |
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.
replaceIfExists
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.
Fixed
| std::set<TString> FeaturesToReset; | ||
| protected: | ||
| bool ExistingOk = false; | ||
| bool IsReplace = false; |
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.
ReplaceIfExists
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.
Fixed
|
|
||
| TFuture<TGenericResult> CreateExternalTable(const TString& cluster, const TCreateExternalTableSettings& settings, | ||
| bool createDir, bool existingOk) override | ||
| bool createDir, bool existingOk, bool isReplace) override |
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.
replaceIfExists
| } | ||
|
|
||
| TFuture<TGenericResult> CreateTable(NYql::TKikimrTableMetadataPtr metadata, bool createDir, bool existingOk) override { | ||
| TFuture<TGenericResult> CreateTable(NYql::TKikimrTableMetadataPtr metadata, bool createDir, bool existingOk, bool isReplace) override { |
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.
replaceIfExists
| TFuture<TGenericResult> CreateExternalTable(const TString& cluster, | ||
| const NYql::TCreateExternalTableSettings& settings, | ||
| bool createDir, bool existingOk) override { | ||
| bool createDir, bool existingOk, bool isReplace) override { |
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.
replaceIfExists
|
|
||
| void FillCreateExternalTableColumnDesc(NKikimrSchemeOp::TExternalTableDescription& externalTableDesc, | ||
| const TString& name, | ||
| bool isReplace, |
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.
replaceIfExists
|
|
||
| void FillCreateExternalTableColumnDesc(NKikimrSchemeOp::TExternalTableDescription& externalTableDesc, | ||
| const TString& name, | ||
| bool isReplace, |
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.
replaceIfExists
|
⚪ |
|
⚪
|
|
⚪
|
|
⚪
|
ydb/core/protos/flat_scheme_op.proto
Outdated
| optional string Location = 6; | ||
| repeated TColumnDescription Columns = 7; | ||
| optional bytes Content = 8; | ||
| optional bool ReplaceIfExists = 9; |
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 field has no sense for describe operation. It's worth to write comment about it.
| Visit(msg.GetBlock3()); | ||
| Visit(msg.GetRule_simple_table_ref4()); | ||
| Visit(msg.GetToken5()); | ||
| Visit(msg.GetBlock4()); |
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.
We need to add tests for this new behaviour to ydb/library/yql/sql/v1/format/sql_format_ut.cpp
|
⚪
|
|
⚪
|
nepal
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.
sql part lgtm
Changelog entry
...
Changelog category
Additional information
...