Skip to content

Capture TablePathPrefix (and other parts of the parser context) in CREATE VIEW #8991

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

Merged
merged 8 commits into from
Sep 17, 2024
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions ydb/core/kqp/gateway/behaviour/view/manager.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
#include <ydb/core/kqp/gateway/actors/scheme.h>
#include <ydb/core/kqp/gateway/utils/scheme_helpers.h>
#include <ydb/core/tx/tx_proxy/proxy.h>
#include <ydb/library/yql/sql/settings/protos/translation_settings.pb.h>

namespace NKikimr::NKqp {

Expand Down Expand Up @@ -58,6 +59,12 @@ void FillCreateViewProposal(NKikimrSchemeOp::TModifyScheme& modifyScheme,
viewDesc.SetName(pathPair.second);
viewDesc.SetQueryText(GetByKeyOrDefault(settings, "query_text"));

NYql::NProto::TTranslationSettings capturedContext;
if (!capturedContext.ParseFromString(GetByKeyOrDefault(settings, "captured_context"))) {
ythrow TBadArgumentException() << "Parser context captured in the view is invalid";
}
*viewDesc.MutableCapturedContext() = std::move(capturedContext);

if (!settings.GetFeaturesExtractor().IsFinished()) {
ythrow TBadArgumentException() << "Unknown property: " << settings.GetFeaturesExtractor().GetRemainedParamsString();
}
Expand Down
2 changes: 1 addition & 1 deletion ydb/core/kqp/gateway/kqp_metadata_loader.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -302,7 +302,7 @@ TTableMetadataResult GetViewMetadataResult(
metadata->SchemaVersion = description.GetVersion();
metadata->Kind = NYql::EKikimrTableKind::View;
metadata->Attributes = schemeEntry.Attributes;
metadata->ViewPersistedData = {description.GetQueryText()};
metadata->ViewPersistedData = {description.GetQueryText(), description.GetCapturedContext()};

return builtResult;
}
Expand Down
11 changes: 7 additions & 4 deletions ydb/core/kqp/provider/rewrite_io_utils.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -19,10 +19,12 @@ TExprNode::TPtr CompileViewQuery(
const TString& query,
TExprContext& ctx,
NKikimr::NKqp::TKqpTranslationSettingsBuilder& settingsBuilder,
IModuleResolver::TPtr moduleResolver
IModuleResolver::TPtr moduleResolver,
const NYql::NProto::TTranslationSettings& capturedContext
) {
auto translationSettings = settingsBuilder.Build(ctx);
translationSettings.Mode = NSQLTranslation::ESqlMode::LIMITED_VIEW;
translationSettings.UpdateWith(capturedContext);

TAstParseResult queryAst;
queryAst = NSQLTranslation::SqlToYql(query, translationSettings);
Expand Down Expand Up @@ -118,7 +120,8 @@ TExprNode::TPtr RewriteReadFromView(
TExprContext& ctx,
const TString& query,
NKikimr::NKqp::TKqpTranslationSettingsBuilder& settingsBuilder,
IModuleResolver::TPtr moduleResolver
IModuleResolver::TPtr moduleResolver,
const NYql::NProto::TTranslationSettings& capturedContext
) {
YQL_PROFILE_FUNC(DEBUG);

Expand All @@ -127,7 +130,7 @@ TExprNode::TPtr RewriteReadFromView(

TExprNode::TPtr queryGraph = FindSavedQueryGraph(readNode.Ptr());
if (!queryGraph) {
queryGraph = CompileViewQuery(query, ctx, settingsBuilder, moduleResolver);
queryGraph = CompileViewQuery(query, ctx, settingsBuilder, moduleResolver, capturedContext);
if (!queryGraph) {
ctx.AddError(TIssue(ctx.GetPosition(readNode.Pos()),
"The query stored in the view cannot be compiled."));
Expand All @@ -151,4 +154,4 @@ TExprNode::TPtr RewriteReadFromView(
return Build<TCoLeft>(ctx, node->Pos()).Input(topLevelRead).Done().Ptr();
}

}
}
6 changes: 4 additions & 2 deletions ydb/core/kqp/provider/rewrite_io_utils.h
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

#include <ydb/core/kqp/host/kqp_translate.h>
#include <ydb/library/yql/ast/yql_expr.h>
#include <ydb/library/yql/sql/settings/protos/translation_settings.pb.h>

namespace NYql {

Expand All @@ -12,7 +13,8 @@ TExprNode::TPtr RewriteReadFromView(
TExprContext& ctx,
const TString& query,
NKikimr::NKqp::TKqpTranslationSettingsBuilder& settingsBuilder,
IModuleResolver::TPtr moduleResolver
IModuleResolver::TPtr moduleResolver,
const NYql::NProto::TTranslationSettings& capturedContext
);

}
}
8 changes: 4 additions & 4 deletions ydb/core/kqp/provider/yql_kikimr_datasink.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -186,9 +186,9 @@ class TKiSinkIntentDeterminationTransformer: public TKiSinkVisitorTransformer {
}

TStatus HandleDropObject(TKiDropObject node, TExprContext& ctx) override {
ctx.AddError(TIssue(ctx.GetPosition(node.Pos()), TStringBuilder()
<< "DropObject is not yet implemented for intent determination transformer"));
return TStatus::Error;
Y_UNUSED(node);
Y_UNUSED(ctx);
return TStatus::Ok;
Copy link
Collaborator Author

@jepett0 jepett0 Sep 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change is needed for the intent determination transformer to be able to handle the following query:

DROP VIEW some_view;
CREATE VIEW some_other_view ...;

The same (i.e. nothing) is done in HandleCreateObject 12 lines above.

}

TStatus HandleCreateGroup(TKiCreateGroup node, TExprContext& ctx) override {
Expand Down Expand Up @@ -893,7 +893,7 @@ class TKikimrDataSink : public TDataProviderBase

if (tableDesc.Metadata->Kind == EKikimrTableKind::Datashard && mode == "analyze") {
ctx.AddError(TIssue(ctx.GetPosition(node->Pos()), TStringBuilder() << static_cast<TStringBuf>(mode) << " is not supported for oltp tables."));
return true;
return true;
}

return false;
Expand Down
4 changes: 3 additions & 1 deletion ydb/core/kqp/provider/yql_kikimr_datasource.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -772,6 +772,8 @@ class TKikimrDataSource : public TDataProviderBase {
.Repeat(TExprStep::RewriteIO);

const auto& query = tableDesc.Metadata->ViewPersistedData.QueryText;
const auto& capturedContext = tableDesc.Metadata->ViewPersistedData.CapturedContext;

NKqp::TKqpTranslationSettingsBuilder settingsBuilder(
SessionCtx->Query().Type,
SessionCtx->Config()._KqpYqlSyntaxVersion.Get().GetRef(),
Expand All @@ -780,7 +782,7 @@ class TKikimrDataSource : public TDataProviderBase {
SessionCtx->Config().BindingsMode,
GUCSettings
);
return RewriteReadFromView(node, ctx, query, settingsBuilder, Types.Modules);
return RewriteReadFromView(node, ctx, query, settingsBuilder, Types.Modules, capturedContext);
}
}

Expand Down
2 changes: 2 additions & 0 deletions ydb/core/kqp/provider/yql_kikimr_gateway.h
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
#include <ydb/library/yql/dq/runtime/dq_transport.h>
#include <ydb/library/yql/minikql/computation/mkql_computation_node_holders.h>
#include <ydb/library/yql/utils/resetable_setting.h>
#include <ydb/library/yql/sql/settings/protos/translation_settings.pb.h>
#include <ydb/public/sdk/cpp/client/ydb_topic/topic.h>
#include <ydb/services/metadata/abstract/kqp_common.h>
#include <ydb/services/metadata/manager/abstract.h>
Expand Down Expand Up @@ -446,6 +447,7 @@ enum EMetaSerializationType : ui64 {

struct TViewPersistedData {
TString QueryText;
NYql::NProto::TTranslationSettings CapturedContext;
};

struct TKikimrTableMetadata : public TThrRefBase {
Expand Down
40 changes: 40 additions & 0 deletions ydb/core/kqp/ut/view/view_ut.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -408,6 +408,46 @@ Y_UNIT_TEST_SUITE(TSelectFromViewTest) {
CompareResults(etalonResults, selectFromViewResults);
}

Y_UNIT_TEST(OneTableUsingRelativeName) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would like to write an extensive test suite for views after this PR, so I add only a basic test of the new feature.

TKikimrRunner kikimr;

auto& runtime = *kikimr.GetTestServer().GetRuntime();
runtime.SetLogPriority(NKikimrServices::FLAT_TX_SCHEMESHARD, NLog::PRI_DEBUG);

EnableViewsFeatureFlag(kikimr);
auto session = kikimr.GetTableClient().CreateSession().GetValueSync().GetSession();

constexpr const char* viewName = "TheView";
constexpr const char* testTable = "Test";
const auto innerQuery = std::format(R"(
SELECT * FROM {}
)",
testTable
);

const TString creationQuery = std::format(R"(
CREATE VIEW {} WITH (security_invoker = true) AS {};
)",
viewName,
innerQuery
);
ExecuteDataDefinitionQuery(session, creationQuery);

const auto etalonResults = ExecuteDataModificationQuery(session, std::format(R"(
SELECT * FROM ({});
)",
innerQuery
)
);
const auto selectFromViewResults = ExecuteDataModificationQuery(session, std::format(R"(
SELECT * FROM {};
)",
viewName
)
);
CompareResults(etalonResults, selectFromViewResults);
}

Y_UNIT_TEST(DisabledFeatureFlag) {
TKikimrRunner kikimr(TKikimrSettings().SetWithSampleTables(false));
auto session = kikimr.GetTableClient().CreateSession().GetValueSync().GetSession();
Expand Down
2 changes: 2 additions & 0 deletions ydb/core/protos/flat_scheme_op.proto
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ import "ydb/public/api/protos/ydb_table.proto";
import "ydb/public/api/protos/ydb_value.proto";
import "ydb/library/actors/protos/actors.proto";
import "ydb/library/mkql_proto/protos/minikql.proto";
import "ydb/library/yql/sql/settings/protos/translation_settings.proto";
import "ydb/core/protos/index_builder.proto";
import "ydb/core/tx/columnshard/engines/scheme/defaults/protos/data.proto";
import "ydb/core/tx/columnshard/common/protos/snapshot.proto";
Expand Down Expand Up @@ -2179,6 +2180,7 @@ message TViewDescription {
optional NKikimrProto.TPathID PathId = 2;
optional uint64 Version = 3;
optional string QueryText = 4;
optional NYql.NProto.TTranslationSettings CapturedContext = 5;
}

message TResourcePoolProperties {
Expand Down
1 change: 1 addition & 0 deletions ydb/core/protos/ya.make
Original file line number Diff line number Diff line change
Expand Up @@ -167,6 +167,7 @@ PEERDIR(
ydb/library/yql/providers/common/proto
ydb/library/yql/public/issue/protos
ydb/library/yql/public/types
ydb/library/yql/sql/settings/protos
ydb/library/services
ydb/library/ydb_issue/proto
ydb/core/tx/columnshard/engines/scheme/defaults/protos
Expand Down
3 changes: 3 additions & 0 deletions ydb/core/tx/schemeshard/schemeshard__init.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1915,6 +1915,9 @@ struct TSchemeShard::TTxInit : public TTransactionBase<TSchemeShard> {
auto& view = Self->Views[pathId] = new TViewInfo();
view->AlterVersion = rowset.GetValue<Schema::View::AlterVersion>();
view->QueryText = rowset.GetValue<Schema::View::QueryText>();
Y_PROTOBUF_SUPPRESS_NODISCARD view->CapturedContext.ParseFromString(
rowset.GetValue<Schema::View::CapturedContext>()
);
Self->IncrementPathDbRefCount(pathId);

if (!rowset.Next()) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ class TPropose: public TSubOperationState {
Y_ABORT_UNLESS(txState->TxType == TTxState::TxCreateView);

context.SS->TabletCounters->Simple()[COUNTER_VIEW_COUNT].Add(1);

const auto pathId = txState->TargetPathId;
auto path = TPath::Init(pathId, context.SS);

Expand All @@ -68,6 +68,7 @@ TViewInfo::TPtr CreateView(const NKikimrSchemeOp::TViewDescription& desc) {
TViewInfo::TPtr viewInfo = new TViewInfo;
viewInfo->AlterVersion = 1;
viewInfo->QueryText = desc.GetQueryText();
viewInfo->CapturedContext = desc.GetCapturedContext();
return viewInfo;
}

Expand Down Expand Up @@ -109,7 +110,7 @@ class TCreateView: public TSubOperation {
const auto& viewDescription = Transaction.GetCreateView();

const TString& name = viewDescription.GetName();

LOG_N("TCreateView Propose"
<< ", path: " << parentPathStr << "/" << name
<< ", opId: " << OperationId
Expand Down
4 changes: 3 additions & 1 deletion ydb/core/tx/schemeshard/schemeshard_impl.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2963,7 +2963,9 @@ void TSchemeShard::PersistView(NIceDb::TNiceDb &db, TPathId pathId) {

db.Table<Schema::View>().Key(pathId.LocalPathId).Update(
NIceDb::TUpdate<Schema::View::AlterVersion>{viewInfo->AlterVersion},
NIceDb::TUpdate<Schema::View::QueryText>{viewInfo->QueryText});
NIceDb::TUpdate<Schema::View::QueryText>{viewInfo->QueryText},
NIceDb::TUpdate<Schema::View::CapturedContext>{viewInfo->CapturedContext.SerializeAsString()}
);
}

void TSchemeShard::PersistRemoveView(NIceDb::TNiceDb& db, TPathId pathId) {
Expand Down
2 changes: 2 additions & 0 deletions ydb/core/tx/schemeshard/schemeshard_info_types.h
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
#include <ydb/core/util/pb.h>

#include <ydb/library/login/protos/login.pb.h>
#include <ydb/library/yql/sql/settings/protos/translation_settings.pb.h>

#include <ydb/public/api/protos/ydb_import.pb.h>
#include <ydb/core/protos/blockstore_config.pb.h>
Expand Down Expand Up @@ -3363,6 +3364,7 @@ struct TViewInfo : TSimpleRefCount<TViewInfo> {

ui64 AlterVersion = 0;
TString QueryText;
NYql::NProto::TTranslationSettings CapturedContext;
};

struct TResourcePoolInfo : TSimpleRefCount<TResourcePoolInfo> {
Expand Down
1 change: 1 addition & 0 deletions ydb/core/tx/schemeshard/schemeshard_path_describer.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1052,6 +1052,7 @@ void TPathDescriber::DescribeView(const TActorContext&, TPathId pathId, TPathEle
PathIdFromPathId(pathId, entry->MutablePathId());
entry->SetVersion(viewInfo->AlterVersion);
entry->SetQueryText(viewInfo->QueryText);
*entry->MutableCapturedContext() = viewInfo->CapturedContext;
}

void TPathDescriber::DescribeResourcePool(TPathId pathId, TPathElement::TPtr pathEl) {
Expand Down
4 changes: 3 additions & 1 deletion ydb/core/tx/schemeshard/schemeshard_schema.h
Original file line number Diff line number Diff line change
Expand Up @@ -1790,9 +1790,11 @@ struct Schema : NIceDb::Schema {
struct PathId: Column<1, NScheme::NTypeIds::Uint64> { using Type = TLocalPathId; };
struct AlterVersion: Column<2, NScheme::NTypeIds::Uint64> {};
struct QueryText: Column<3, NScheme::NTypeIds::String> {};
// CapturedContext is a serialized NYql::NProto::TTranslationSettings.
struct CapturedContext: Column<4, NScheme::NTypeIds::String> {};

using TKey = TableKey<PathId>;
using TColumns = TableColumns<PathId, AlterVersion, QueryText>;
using TColumns = TableColumns<PathId, AlterVersion, QueryText, CapturedContext>;
};

struct BackgroundSessions: Table<109> {
Expand Down
1 change: 1 addition & 0 deletions ydb/core/tx/schemeshard/ya.make
Original file line number Diff line number Diff line change
Expand Up @@ -286,6 +286,7 @@ PEERDIR(
ydb/library/protobuf_printer
ydb/library/yql/minikql
ydb/library/yql/providers/common/proto
ydb/library/yql/sql/settings/protos
ydb/services/bg_tasks
ydb/core/tx/columnshard/bg_tasks/manager
)
Expand Down
14 changes: 14 additions & 0 deletions ydb/library/yql/sql/settings/protos/translation_settings.proto
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
syntax = "proto3";

package NYql.NProto;

option java_package = "com.yandex.yql.proto";

message TTranslationSettings {
optional string PathPrefix = 1;
optional uint32 SyntaxVersion = 2;
optional bool AnsiLexer = 3;
optional bool Antlr4Parser = 4;
optional bool PgParser = 5;
repeated string Pragmas = 6;
}
9 changes: 9 additions & 0 deletions ydb/library/yql/sql/settings/protos/ya.make
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
PROTO_LIBRARY()

SRCS(
translation_settings.proto
)

EXCLUDE_TAGS(GO_PROTO)

END()
31 changes: 31 additions & 0 deletions ydb/library/yql/sql/settings/translation_settings.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,37 @@ namespace NSQLTranslation {
, AssumeYdbOnClusterWithSlash(false)
{}

NYql::NProto::TTranslationSettings TTranslationSettings::Serialize() const {
NYql::NProto::TTranslationSettings serializedSettings;

serializedSettings.SetPathPrefix(PathPrefix);
serializedSettings.SetSyntaxVersion(SyntaxVersion);
serializedSettings.SetAnsiLexer(AnsiLexer);
serializedSettings.SetAntlr4Parser(Antlr4Parser);
serializedSettings.SetPgParser(PgParser);

auto* pragmas = serializedSettings.MutablePragmas();
pragmas->Add(Flags.begin(), Flags.end());

return serializedSettings;
}

void TTranslationSettings::UpdateWith(const NYql::NProto::TTranslationSettings& serializedSettings) {
#define DeserializeSetting(settingName) \
if (serializedSettings.Has##settingName()) { \
settingName = serializedSettings.Get##settingName(); \
}

DeserializeSetting(PathPrefix);
DeserializeSetting(SyntaxVersion);
DeserializeSetting(AnsiLexer);
DeserializeSetting(Antlr4Parser);
DeserializeSetting(PgParser);

#undef DeserializeSetting

Flags.insert(serializedSettings.GetPragmas().begin(), serializedSettings.GetPragmas().end());
}

bool ParseTranslationSettings(const TString& query, TTranslationSettings& settings, NYql::TIssues& issues) {
if (!NYql::IsUtf8(query)) {
Expand Down
4 changes: 4 additions & 0 deletions ydb/library/yql/sql/settings/translation_settings.h
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
#pragma once

#include <ydb/library/yql/core/pg_settings/guc_settings.h>
#include <ydb/library/yql/sql/settings/protos/translation_settings.pb.h>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

не надо ссылаться на protobuf-ы в основной либе

сделайте отдельную library, в которой будет save/load в protobuf классы

и вообще этот protobuf и library унесите в ydb

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Не знаю, как унести в YDB, учитывая, что эта библиотека нужна в sql_translation.cpp для валидации текста запроса view.

В отдельную библиотеку выделил, но она по-прежнему лежит в YQL юрисдикции: ydb/library/yql/sql/settings/serializer/serializer.h.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Унёс всё в YDB. Мотивация следующая:

  • KQP создаёт TTransltaionSettings, с которыми будет запущен парсер. Парсер не меняет эти настройки.
  • Сериализация нужна в KQP в момент создания TModifyScheme для CreateView.
  • Раз translation settings и создаются, и используются вне парсера, то логично их там и сериализовывать. Парсер в таком подходе ничего не знает об этой части сохранения контекста для view.

Важно отметить, что теперь, когда мы сериализуем контекст в KQP, этот процесс становится по смыслу равен сохранению применённых дефолтных настроек парсера в теле view. Query replay для тех же целей запоминает query type, с которым был выполнен запрос. Мы могли бы делать так же (query type было бы вполне достаточно), но поскольку view - это объект из мира парсера, то мы выбрали формат более подходящий для объекта из парсера.


#include <util/generic/hash.h>
#include <util/generic/hash_set.h>
Expand Down Expand Up @@ -125,6 +126,9 @@ namespace NSQLTranslation {

TMaybe<TString> ApplicationName;
bool PgSortNulls = false;

NYql::NProto::TTranslationSettings Serialize() const;
void UpdateWith(const NYql::NProto::TTranslationSettings& serializedSettings);
};

bool ParseTranslationSettings(const TString& query, NSQLTranslation::TTranslationSettings& settings, NYql::TIssues& issues);
Expand Down
1 change: 1 addition & 0 deletions ydb/library/yql/sql/settings/ya.make
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ PEERDIR(
ydb/library/yql/core/issue
ydb/library/yql/core/pg_settings
ydb/library/yql/core/issue/protos
ydb/library/yql/sql/settings/protos
ydb/library/yql/utils
)

Expand Down
Loading
Loading