Skip to content

Commit

Permalink
Review fixes
Browse files Browse the repository at this point in the history 
- Unit tests for disabled feature flag.
- Explicit error in case views haven't been rewritten at later optimizer
stage.
- Explicit node typing in RewriteReadFromView.
- Remove unnecessary check in RewriteReadFromView.
- Formatting.
  • Loading branch information
@jepett0
jepett0 committed Jan 18, 2024
1 parent 1a3c7bb commit 03b2738
Show file tree
Hide file tree
Showing 4 changed files with 120 additions and 47 deletions.
39 changes: 21 additions & 18 deletions ydb/core/kqp/gateway/kqp_metadata_loader.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -278,24 +278,27 @@ TTableMetadataResult GetExternalDataSourceMetadataResult(const NSchemeCache::TSc
return result;
}

TTableMetadataResult GetViewMetadataResult(const NSchemeCache::TSchemeCacheNavigate::TEntry& schemeEntry,
const TString& cluster,
const TString& viewName) {
const auto& description = schemeEntry.ViewInfo->Description;

TTableMetadataResult builtResult;
builtResult.SetSuccess();

builtResult.Metadata = new NYql::TKikimrTableMetadata(cluster, viewName);
auto metadata = builtResult.Metadata;
metadata->DoesExist = true;
metadata->PathId = NYql::TKikimrPathId(description.GetPathId().GetOwnerId(), description.GetPathId().GetLocalId());
metadata->SchemaVersion = description.GetVersion();
metadata->Kind = NYql::EKikimrTableKind::View;
metadata->Attributes = schemeEntry.Attributes;
metadata->ViewPersistedData = {description.GetQueryText()};

return builtResult;
TTableMetadataResult GetViewMetadataResult(
const NSchemeCache::TSchemeCacheNavigate::TEntry& schemeEntry,
const TString& cluster,
const TString& viewName
) {
const auto& description = schemeEntry.ViewInfo->Description;

TTableMetadataResult builtResult;
builtResult.SetSuccess();

builtResult.Metadata = new NYql::TKikimrTableMetadata(cluster, viewName);
auto metadata = builtResult.Metadata;
metadata->DoesExist = true;
metadata->PathId = NYql::TKikimrPathId(description.GetPathId().GetOwnerId(),
description.GetPathId().GetLocalId());
metadata->SchemaVersion = description.GetVersion();
metadata->Kind = NYql::EKikimrTableKind::View;
metadata->Attributes = schemeEntry.Attributes;
metadata->ViewPersistedData = {description.GetQueryText()};

return builtResult;
}

TTableMetadataResult GetLoadTableMetadataResult(const NSchemeCache::TSchemeCacheNavigate::TEntry& entry,
Expand Down
3 changes: 2 additions & 1 deletion ydb/core/kqp/opt/logical/kqp_opt_log_ranges_predext.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -55,9 +55,10 @@ TMaybeNode<TExprBase> TryBuildTrivialReadTable(TCoFlatMap& flatmap, TKqlReadTabl
break;
case EKikimrTableKind::Olap:
case EKikimrTableKind::External:
case EKikimrTableKind::View:
case EKikimrTableKind::Unspecified:
return {};
case EKikimrTableKind::View:
YQL_ENSURE(false, "All views should have been rewritten at this stage.");
}

auto row = flatmap.Lambda().Args().Arg(0);
Expand Down
25 changes: 10 additions & 15 deletions ydb/core/kqp/provider/rewrite_io_utils.cpp
View file
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
#include "rewrite_io_utils.h"

#include <ydb/core/kqp/provider/yql_kikimr_expr_nodes.h>
#include <ydb/library/yql/core/expr_nodes/yql_expr_nodes.h>
#include <ydb/library/yql/core/yql_expr_optimize.h>
#include <ydb/library/yql/providers/common/provider/yql_provider.h>
Expand All @@ -24,7 +25,7 @@ NSQLTranslation::TTranslationSettings CreateViewTranslationSettings(const TStrin
return settings;
}

TExprNode::TPtr CompileQuery(
TExprNode::TPtr CompileViewQuery(
const TString& query,
TExprContext& ctx,
const TString& cluster
Expand All @@ -45,12 +46,6 @@ TExprNode::TPtr CompileQuery(
return queryGraph;
}

bool ContainsNullNode(const TExprNode::TPtr& root) {
return static_cast<bool>(FindNode(root, [](const TExprNode::TPtr& node) {
return !node;
}));
}

void AddChild(const TExprNode::TPtr& parent, const TExprNode::TPtr& newChild) {
auto childrenToChange = parent->ChildrenList();
childrenToChange.emplace_back(newChild);
Expand Down Expand Up @@ -137,22 +132,22 @@ TExprNode::TPtr RewriteReadFromView(
const TString& query,
const TString& cluster
) {
const auto readNode = node->ChildPtr(0);
const auto worldBeforeThisRead = readNode->ChildPtr(0);
const TCoRead readNode(node->ChildPtr(0));
const auto worldBeforeThisRead = readNode.World().Ptr();

TExprNode::TPtr queryGraph = FindSavedQueryGraph(readNode);
TExprNode::TPtr queryGraph = FindSavedQueryGraph(readNode.Ptr());
if (!queryGraph) {
queryGraph = CompileQuery(query, ctx, cluster);
if (!queryGraph || ContainsNullNode(queryGraph)) {
ctx.AddError(TIssue(readNode->Pos(ctx),
"The query stored in the view contains errors and cannot be compiled."));
queryGraph = CompileViewQuery(query, ctx, cluster);
if (!queryGraph) {
ctx.AddError(TIssue(ctx.GetPosition(readNode.Pos()),
"The query stored in the view cannot be compiled."));
return nullptr;
}
YQL_CLOG(TRACE, ProviderKqp) << "Expression graph of the query stored in the view:\n"
<< NCommon::ExprToPrettyString(ctx, *queryGraph);

InsertExecutionOrderDependencies(queryGraph, worldBeforeThisRead);
SaveQueryGraph(readNode, ctx, queryGraph);
SaveQueryGraph(readNode.Ptr(), ctx, queryGraph);
}

if (node->IsCallable(RightName)) {
Expand Down
100 changes: 87 additions & 13 deletions ydb/core/kqp/ut/view/view_ut.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,10 +12,14 @@ using namespace NYdb::NTable;

namespace {

void SetEnableViewsFeatureFlag(TKikimrRunner& kikimr) {
void EnableViewsFeatureFlag(TKikimrRunner& kikimr) {
kikimr.GetTestServer().GetRuntime()->GetAppData(0).FeatureFlags.SetEnableViews(true);
}

void DisableViewsFeatureFlag(TKikimrRunner& kikimr) {
kikimr.GetTestServer().GetRuntime()->GetAppData(0).FeatureFlags.SetEnableViews(false);
}

NKikimrSchemeOp::TViewDescription GetViewDescription(TTestActorRuntime& runtime, const TString& path) {
const auto pathQueryResult = Navigate(runtime,
runtime.AllocateEdgeActor(),
Expand Down Expand Up @@ -53,7 +57,7 @@ TString ReadWholeFile(const TString& path) {
void ExecuteDataDefinitionQuery(TSession& session, const TString& script) {
Cerr << "Executing the following DDL script:\n" << script;

const auto result = session.ExecuteSchemeQuery(script, {}).ExtractValueSync();
const auto result = session.ExecuteSchemeQuery(script).ExtractValueSync();
UNIT_ASSERT_C(result.IsSuccess(), result.GetIssues().ToString());
}

Expand All @@ -62,8 +66,7 @@ TDataQueryResult ExecuteDataModificationQuery(TSession& session, const TString&

const auto result = session.ExecuteDataQuery(
script,
TTxControl::BeginTx(TTxSettings::SerializableRW()).CommitTx(),
{}
TTxControl::BeginTx(TTxSettings::SerializableRW()).CommitTx()
).ExtractValueSync();
UNIT_ASSERT_C(result.IsSuccess(), result.GetIssues().ToString());

Expand Down Expand Up @@ -92,7 +95,7 @@ Y_UNIT_TEST_SUITE(TKQPViewTest) {

Y_UNIT_TEST(CheckCreatedView) {
TKikimrRunner kikimr(TKikimrSettings().SetWithSampleTables(false));
SetEnableViewsFeatureFlag(kikimr);
EnableViewsFeatureFlag(kikimr);
auto& runtime = *kikimr.GetTestServer().GetRuntime();
auto session = kikimr.GetTableClient().CreateSession().GetValueSync().GetSession();

Expand All @@ -111,9 +114,27 @@ Y_UNIT_TEST_SUITE(TKQPViewTest) {
UNIT_ASSERT_EQUAL(viewDescription.GetQueryText(), queryInView);
}

Y_UNIT_TEST(CreateViewDisabledFeatureFlag) {
TKikimrRunner kikimr(TKikimrSettings().SetWithSampleTables(false));
auto session = kikimr.GetTableClient().CreateSession().GetValueSync().GetSession();

constexpr const char* path = "/Root/TheView";

const TString creationQuery = std::format(R"(
CREATE VIEW `{}` WITH (security_invoker = true) AS SELECT 1;
)",
path
);

DisableViewsFeatureFlag(kikimr);
const auto creationResult = session.ExecuteSchemeQuery(creationQuery).ExtractValueSync();
UNIT_ASSERT(!creationResult.IsSuccess());
UNIT_ASSERT_STRING_CONTAINS(creationResult.GetIssues().ToString(), "Error: Views are disabled");
}

Y_UNIT_TEST(InvalidQuery) {
TKikimrRunner kikimr(TKikimrSettings().SetWithSampleTables(false));
SetEnableViewsFeatureFlag(kikimr);
EnableViewsFeatureFlag(kikimr);
auto session = kikimr.GetTableClient().CreateSession().GetValueSync().GetSession();

constexpr const char* path = "/Root/TheView";
Expand All @@ -131,14 +152,14 @@ Y_UNIT_TEST_SUITE(TKQPViewTest) {
queryInView
);

const auto creationResult = session.ExecuteSchemeQuery(creationQuery, {}).ExtractValueSync();
const auto creationResult = session.ExecuteSchemeQuery(creationQuery).ExtractValueSync();
UNIT_ASSERT(!creationResult.IsSuccess());
UNIT_ASSERT_STRING_CONTAINS(creationResult.GetIssues().ToString(), "Error: Cannot divide type String and String");
}

Y_UNIT_TEST(ListCreatedView) {
TKikimrRunner kikimr(TKikimrSettings().SetWithSampleTables(false));
SetEnableViewsFeatureFlag(kikimr);
EnableViewsFeatureFlag(kikimr);
auto session = kikimr.GetTableClient().CreateSession().GetValueSync().GetSession();

// .sys directory is always present in the `/Root`, that's why we need a subfolder
Expand Down Expand Up @@ -167,7 +188,7 @@ Y_UNIT_TEST_SUITE(TKQPViewTest) {

Y_UNIT_TEST(CreateSameViewTwice) {
TKikimrRunner kikimr(TKikimrSettings().SetWithSampleTables(false));
SetEnableViewsFeatureFlag(kikimr);
EnableViewsFeatureFlag(kikimr);
auto session = kikimr.GetTableClient().CreateSession().GetValueSync().GetSession();

constexpr const char* path = "/Root/TheView";
Expand All @@ -189,7 +210,7 @@ Y_UNIT_TEST_SUITE(TKQPViewTest) {

Y_UNIT_TEST(DropView) {
TKikimrRunner kikimr(TKikimrSettings().SetWithSampleTables(false));
SetEnableViewsFeatureFlag(kikimr);
EnableViewsFeatureFlag(kikimr);
auto& runtime = *kikimr.GetTestServer().GetRuntime();
auto session = kikimr.GetTableClient().CreateSession().GetValueSync().GetSession();

Expand All @@ -213,9 +234,34 @@ Y_UNIT_TEST_SUITE(TKQPViewTest) {
ExpectUnknownEntry(runtime, path);
}

Y_UNIT_TEST(DropViewDisabledFeatureFlag) {
TKikimrRunner kikimr(TKikimrSettings().SetWithSampleTables(false));
auto session = kikimr.GetTableClient().CreateSession().GetValueSync().GetSession();

constexpr const char* path = "/Root/TheView";

const TString creationQuery = std::format(R"(
CREATE VIEW `{}` WITH (security_invoker = true) AS SELECT 1;
)",
path
);
EnableViewsFeatureFlag(kikimr);
ExecuteDataDefinitionQuery(session, creationQuery);

const TString dropQuery = std::format(R"(
DROP VIEW `{}`;
)",
path
);
DisableViewsFeatureFlag(kikimr);
const auto dropResult = session.ExecuteSchemeQuery(dropQuery).ExtractValueSync();
UNIT_ASSERT(!dropResult.IsSuccess());
UNIT_ASSERT_STRING_CONTAINS(dropResult.GetIssues().ToString(), "Error: Views are disabled");
}

Y_UNIT_TEST(DropSameViewTwice) {
TKikimrRunner kikimr(TKikimrSettings().SetWithSampleTables(false));
SetEnableViewsFeatureFlag(kikimr);
EnableViewsFeatureFlag(kikimr);
auto session = kikimr.GetTableClient().CreateSession().GetValueSync().GetSession();

constexpr const char* path = "/Root/TheView";
Expand Down Expand Up @@ -247,7 +293,7 @@ Y_UNIT_TEST_SUITE(TSelectFromViewTest) {

Y_UNIT_TEST(OneTable) {
TKikimrRunner kikimr;
SetEnableViewsFeatureFlag(kikimr);
EnableViewsFeatureFlag(kikimr);
auto session = kikimr.GetTableClient().CreateSession().GetValueSync().GetSession();

constexpr const char* viewName = "/Root/TheView";
Expand Down Expand Up @@ -281,9 +327,37 @@ Y_UNIT_TEST_SUITE(TSelectFromViewTest) {
CompareResults(etalonResults, selectFromViewResults);
}

Y_UNIT_TEST(DisabledFeatureFlag) {
TKikimrRunner kikimr(TKikimrSettings().SetWithSampleTables(false));
auto session = kikimr.GetTableClient().CreateSession().GetValueSync().GetSession();

constexpr const char* path = "/Root/TheView";

const TString creationQuery = std::format(R"(
CREATE VIEW `{}` WITH (security_invoker = true) AS SELECT 1;
)",
path
);
EnableViewsFeatureFlag(kikimr);
ExecuteDataDefinitionQuery(session, creationQuery);

const TString selectQuery = std::format(R"(
SELECT * FROM `{}`;
)",
path
);
DisableViewsFeatureFlag(kikimr);
const auto selectResult = session.ExecuteDataQuery(
selectQuery,
TTxControl::BeginTx(TTxSettings::SerializableRW()).CommitTx()
).ExtractValueSync();
UNIT_ASSERT(!selectResult.IsSuccess());
UNIT_ASSERT_STRING_CONTAINS(selectResult.GetIssues().ToString(), "Error: Views are disabled");
}

Y_UNIT_TEST(ReadTestCasesFromFiles) {
TKikimrRunner kikimr;
SetEnableViewsFeatureFlag(kikimr);
EnableViewsFeatureFlag(kikimr);
auto session = kikimr.GetTableClient().CreateSession().GetValueSync().GetSession();

InitializeTablesAndSecondaryViews(session);
Expand Down

0 comments on commit 03b2738

Please sign in to comment.