Skip to content
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.

Sign up for GitHub

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

YQL-17476 check for cycle during evaluation of file args #1608

Merged
merged 3 commits into from
Feb 6, 2024
Merged

YQL-17476 check for cycle during evaluation of file args #1608

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f47f575
init
vitstn Feb 5, 2024
bd9579b
fix
vitstn Feb 5, 2024
e52c632
fix
vitstn Feb 5, 2024
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
50 changes: 42 additions & 8 deletions ydb/library/yql/providers/config/yql_config_provider.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -168,7 +168,7 @@ namespace {
for (auto& arg: flag.GetArgs()) {
args.push_back(arg);
}
if (!ApplyFlag(pos, flag.GetName(), args, ctx)) {
if (!ApplyFlag(pos, flag.GetName(), args, ctx, 0)) {
return false;
}
}
Expand Down Expand Up @@ -260,15 +260,23 @@ namespace {
for (size_t i = 3; i < node->ChildrenSize(); ++i) {
if (node->Child(i)->IsCallable("EvaluateAtom")) {
hasPendingEvaluations = true;
return res;
break;
}
if (!EnsureAtom(*node->Child(i), ctx)) {
return {};
}
args.push_back(node->Child(i)->Content());
}

if (!ApplyFlag(ctx.GetPosition(node->Child(2)->Pos()), command, args, ctx)) {
if (hasPendingEvaluations) {
if (!ValidateEvaluation(command, *node, ctx)) {
return {};
}

return res;
}

if (!ApplyFlag(ctx.GetPosition(node->Child(2)->Pos()), command, args, ctx, node->UniqueId())) {
return {};
}

Expand Down Expand Up @@ -460,7 +468,30 @@ namespace {
return true;
}

bool ApplyFlag(const TPosition& pos, const TStringBuf name, const TVector<TStringBuf>& args, TExprContext& ctx) {
bool ValidateEvaluation(const TStringBuf name, const TExprNode& node, TExprContext& ctx) {
if (name == "AddFileByUrl" || name == "AddFolderByUrl") {
if (node.ChildrenSize() < 4) {
ctx.AddError(TIssue(ctx.GetPosition(node.Pos()), TStringBuilder() << "Expected at least 4 arguments, but got " << node.ChildrenSize()));
return false;
}

if (node.Child(3)->IsCallable("EvaluateAtom")) {
return true;
}

if (!PendingEvaluationFiles.insert({TString(node.Child(3)->Content()), node.UniqueId()}).second) {
ctx.AddError(TIssue(ctx.GetPosition(node.Pos()), TStringBuilder() << "Detected evaluation cycle for file: " << node.Child(3)->Content()));
return false;
}

return true;
} else {
return true;
}
}

bool ApplyFlag(const TPosition& pos, const TStringBuf name, const TVector<TStringBuf>& args, TExprContext& ctx,
ui64 nodeUniqueId) {
if (!IsSettingAllowed(pos, name, ctx)) {
return false;
}
Expand All @@ -474,15 +505,15 @@ namespace {
return false;
}
} else if (name == "AddFileByUrl") {
if (!AddFileByUrl(pos, args, ctx)) {
if (!AddFileByUrl(pos, args, ctx, nodeUniqueId)) {
return false;
}
} else if (name == "SetFileOption") {
if (!SetFileOption(pos, args, ctx)) {
return false;
}
} else if (name == "AddFolderByUrl") {
if (!AddFolderByUrl(pos, args, ctx)) {
if (!AddFolderByUrl(pos, args, ctx, nodeUniqueId)) {
return false;
}
} else if (name == "SetPackageVersion") {
Expand Down Expand Up @@ -985,12 +1016,13 @@ namespace {
return true;
}

bool AddFileByUrl(const TPosition& pos, const TVector<TStringBuf>& args, TExprContext& ctx) {
bool AddFileByUrl(const TPosition& pos, const TVector<TStringBuf>& args, TExprContext& ctx, ui64 nodeUniqueId) {
if (args.size() < 2 || args.size() > 3) {
ctx.AddError(TIssue(pos, TStringBuilder() << "Expected 2 or 3 arguments, but got " << args.size()));
return false;
}

PendingEvaluationFiles.erase({TString(args[0]),nodeUniqueId});
TStringBuf token = args.size() == 3 ? args[2] : TStringBuf();
if (token) {
if (auto cred = Types.Credentials->FindCredential(token)) {
Expand Down Expand Up @@ -1104,12 +1136,13 @@ namespace {
return url;
}

bool AddFolderByUrl(const TPosition& pos, const TVector<TStringBuf>& args, TExprContext& ctx) {
bool AddFolderByUrl(const TPosition& pos, const TVector<TStringBuf>& args, TExprContext& ctx, ui64 nodeUniqueId) {
if (args.size() < 2 || args.size() > 3) {
ctx.AddError(TIssue(pos, TStringBuilder() << "Expected 2 or 3 arguments, but got " << args.size()));
return false;
}

PendingEvaluationFiles.erase({TString(args[0]),nodeUniqueId});
TStringBuf token = args.size() == 3 ? args[2] : TStringBuf();
if (token) {
if (auto cred = Types.Credentials->FindCredential(token)) {
Expand Down Expand Up @@ -1206,6 +1239,7 @@ namespace {
TString Username;
const TAllowSettingPolicy Policy;
TOperationStatistics Statistics;
THashSet<std::pair<TString, ui64>> PendingEvaluationFiles;
};
}

Expand Down
5 changes: 5 additions & 0 deletions ydb/library/yql/tests/sql/dq_file/part8/canondata/result.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,11 @@
}
],
"test.test[action-evaluate_pure--Results]": [],
"test.test[action-file_cycle--Results]": [
{
"uri": "file://test.test_action-file_cycle--Results_/extracted"
}
],
"test.test[action-nested_eval-default.txt-Analyze]": [
{
"checksum": "b4dd508a329723c74293d80f0278c705",
Expand Down
5 changes: 5 additions & 0 deletions ...ary/yql/tests/sql/dq_file/part8/canondata/test.test_action-file_cycle--Results_/extracted
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
<tmp_path>/program.sql:<main>: Error: Pre type annotation

<tmp_path>/program.sql:<main>:2:18: Error: Detected evaluation cycle for file: f
pragma file("f", $p);
^
14 changes: 14 additions & 0 deletions ydb/library/yql/tests/sql/sql2yql/canondata/result.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -412,6 +412,13 @@
"uri": "https://{canondata_backend}/1936947/659b615f15086142a8960946dabd06b519d43335/resource.tar.gz#test_sql2yql.test_action-export_action_/sql.yql"
}
],
"test_sql2yql.test[action-file_cycle]": [
{
"checksum": "095c2c315ba26037554c056a02f5af05",
"size": 343,
"uri": "https://{canondata_backend}/1871002/4d084a25cea0c62bd09f3c9e9e7c2b56d112c5b1/resource.tar.gz#test_sql2yql.test_action-file_cycle_/sql.yql"
}
],
"test_sql2yql.test[action-inline_action]": [
{
"checksum": "d019514b3fd1f8a7c0f9d37db4231b93",
Expand Down Expand Up @@ -17891,6 +17898,13 @@
"uri": "https://{canondata_backend}/1871102/17992aa919577eec0f31ef167084ab70f41ccc80/resource.tar.gz#test_sql_format.test_action-export_action_/formatted.sql"
}
],
"test_sql_format.test[action-file_cycle]": [
{
"checksum": "5476df82092007027246993752e24f68",
"size": 45,
"uri": "https://{canondata_backend}/1871002/4d084a25cea0c62bd09f3c9e9e7c2b56d112c5b1/resource.tar.gz#test_sql_format.test_action-file_cycle_/formatted.sql"
}
],
"test_sql_format.test[action-inline_action]": [
{
"checksum": "7da554999ea3520183678c730ae2fffb",
Expand Down
2 changes: 2 additions & 0 deletions ydb/library/yql/tests/sql/suites/action/file_cycle.cfg
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
xfail

3 changes: 3 additions & 0 deletions ydb/library/yql/tests/sql/suites/action/file_cycle.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
$p=FileContent("f");
pragma file("f", $p);

7 changes: 7 additions & 0 deletions ydb/library/yql/tests/sql/yt_native_file/part8/canondata/result.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,13 @@
"uri": "https://{canondata_backend}/1900335/4871e48d29e4933514d3ed0c9c5d19de571eda1f/resource.tar.gz#test.test_action-evaluate_pure--Results_/results.txt"
}
],
"test.test[action-file_cycle--Debug]": [],
"test.test[action-file_cycle--Plan]": [],
"test.test[action-file_cycle--Results]": [
{
"uri": "file://test.test_action-file_cycle--Results_/extracted"
}
],
"test.test[action-nested_eval-default.txt-Debug]": [
{
"checksum": "79af7d3cdd20c4123718aae10c3202d7",
Expand Down
5 changes: 5 additions & 0 deletions .../tests/sql/yt_native_file/part8/canondata/test.test_action-file_cycle--Results_/extracted
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
<tmp_path>/program.sql:<main>: Error: Pre type annotation

<tmp_path>/program.sql:<main>:2:18: Error: Detected evaluation cycle for file: f
pragma file("f", $p);
^
Loading