From f47f575f74c1f5038d88f7c94d24fb15eeaf797d Mon Sep 17 00:00:00 2001 From: Vitaly Stoyan Date: Mon, 5 Feb 2024 23:18:11 +0000 Subject: [PATCH 1/3] init --- .../providers/config/yql_config_provider.cpp | 35 ++++++++++++++++++- .../sql/dq_file/part8/canondata/result.json | 5 +++ .../extracted | 5 +++ .../tests/sql/sql2yql/canondata/result.json | 14 ++++++++ .../tests/sql/suites/action/file_cycle.cfg | 2 ++ .../tests/sql/suites/action/file_cycle.sql | 3 ++ .../part8/canondata/result.json | 7 ++++ .../extracted | 5 +++ 8 files changed, 75 insertions(+), 1 deletion(-) create mode 100644 ydb/library/yql/tests/sql/dq_file/part8/canondata/test.test_action-file_cycle--Results_/extracted create mode 100644 ydb/library/yql/tests/sql/suites/action/file_cycle.cfg create mode 100644 ydb/library/yql/tests/sql/suites/action/file_cycle.sql create mode 100644 ydb/library/yql/tests/sql/yt_native_file/part8/canondata/test.test_action-file_cycle--Results_/extracted diff --git a/ydb/library/yql/providers/config/yql_config_provider.cpp b/ydb/library/yql/providers/config/yql_config_provider.cpp index 03c7a7f4a4d5..78a66d1bd534 100644 --- a/ydb/library/yql/providers/config/yql_config_provider.cpp +++ b/ydb/library/yql/providers/config/yql_config_provider.cpp @@ -260,7 +260,7 @@ 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 {}; @@ -268,6 +268,14 @@ namespace { args.push_back(node->Child(i)->Content()); } + if (hasPendingEvaluations) { + if (!ValidateEvaluation(command, *node, ctx)) { + return {}; + } + + return res; + } + if (!ApplyFlag(ctx.GetPosition(node->Child(2)->Pos()), command, args, ctx)) { return {}; } @@ -460,6 +468,28 @@ namespace { return true; } + 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 5 arguments, but got " << node.ChildrenSize())); + return false; + } + + if (node.Child(3)->IsCallable("EvaluateAtom")) { + return true; + } + + if (!PendingEvaluationFiles.insert(TString(node.Child(3)->Content())).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& args, TExprContext& ctx) { if (!IsSettingAllowed(pos, name, ctx)) { return false; @@ -991,6 +1021,7 @@ namespace { return false; } + PendingEvaluationFiles.erase(TString(args[0])); TStringBuf token = args.size() == 3 ? args[2] : TStringBuf(); if (token) { if (auto cred = Types.Credentials->FindCredential(token)) { @@ -1110,6 +1141,7 @@ namespace { return false; } + PendingEvaluationFiles.erase(TString(args[0])); TStringBuf token = args.size() == 3 ? args[2] : TStringBuf(); if (token) { if (auto cred = Types.Credentials->FindCredential(token)) { @@ -1206,6 +1238,7 @@ namespace { TString Username; const TAllowSettingPolicy Policy; TOperationStatistics Statistics; + THashSet PendingEvaluationFiles; }; } diff --git a/ydb/library/yql/tests/sql/dq_file/part8/canondata/result.json b/ydb/library/yql/tests/sql/dq_file/part8/canondata/result.json index 25cec54c7112..7ce45cf834b1 100644 --- a/ydb/library/yql/tests/sql/dq_file/part8/canondata/result.json +++ b/ydb/library/yql/tests/sql/dq_file/part8/canondata/result.json @@ -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", diff --git a/ydb/library/yql/tests/sql/dq_file/part8/canondata/test.test_action-file_cycle--Results_/extracted b/ydb/library/yql/tests/sql/dq_file/part8/canondata/test.test_action-file_cycle--Results_/extracted new file mode 100644 index 000000000000..833dc693f5a5 --- /dev/null +++ b/ydb/library/yql/tests/sql/dq_file/part8/canondata/test.test_action-file_cycle--Results_/extracted @@ -0,0 +1,5 @@ +/program.sql:
: Error: Pre type annotation + + /program.sql:
:2:18: Error: Detected evaluation cycle for file: f + pragma file("f", $p); + ^ \ No newline at end of file diff --git a/ydb/library/yql/tests/sql/sql2yql/canondata/result.json b/ydb/library/yql/tests/sql/sql2yql/canondata/result.json index 5d5be56f2e8e..0212a2c37745 100644 --- a/ydb/library/yql/tests/sql/sql2yql/canondata/result.json +++ b/ydb/library/yql/tests/sql/sql2yql/canondata/result.json @@ -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", @@ -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", diff --git a/ydb/library/yql/tests/sql/suites/action/file_cycle.cfg b/ydb/library/yql/tests/sql/suites/action/file_cycle.cfg new file mode 100644 index 000000000000..83cfd96179ac --- /dev/null +++ b/ydb/library/yql/tests/sql/suites/action/file_cycle.cfg @@ -0,0 +1,2 @@ +xfail + diff --git a/ydb/library/yql/tests/sql/suites/action/file_cycle.sql b/ydb/library/yql/tests/sql/suites/action/file_cycle.sql new file mode 100644 index 000000000000..fc1374cfc62e --- /dev/null +++ b/ydb/library/yql/tests/sql/suites/action/file_cycle.sql @@ -0,0 +1,3 @@ +$p=FileContent("f"); +pragma file("f", $p); + diff --git a/ydb/library/yql/tests/sql/yt_native_file/part8/canondata/result.json b/ydb/library/yql/tests/sql/yt_native_file/part8/canondata/result.json index 6a320dc12e0f..508824ee36b5 100644 --- a/ydb/library/yql/tests/sql/yt_native_file/part8/canondata/result.json +++ b/ydb/library/yql/tests/sql/yt_native_file/part8/canondata/result.json @@ -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", diff --git a/ydb/library/yql/tests/sql/yt_native_file/part8/canondata/test.test_action-file_cycle--Results_/extracted b/ydb/library/yql/tests/sql/yt_native_file/part8/canondata/test.test_action-file_cycle--Results_/extracted new file mode 100644 index 000000000000..833dc693f5a5 --- /dev/null +++ b/ydb/library/yql/tests/sql/yt_native_file/part8/canondata/test.test_action-file_cycle--Results_/extracted @@ -0,0 +1,5 @@ +/program.sql:
: Error: Pre type annotation + + /program.sql:
:2:18: Error: Detected evaluation cycle for file: f + pragma file("f", $p); + ^ \ No newline at end of file From bd9579b296d0697d97e086c0a1ce9e4a0c69d150 Mon Sep 17 00:00:00 2001 From: Vitaly Stoyan Date: Mon, 5 Feb 2024 23:21:04 +0000 Subject: [PATCH 2/3] fix --- ydb/library/yql/providers/config/yql_config_provider.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ydb/library/yql/providers/config/yql_config_provider.cpp b/ydb/library/yql/providers/config/yql_config_provider.cpp index 78a66d1bd534..64460d39e276 100644 --- a/ydb/library/yql/providers/config/yql_config_provider.cpp +++ b/ydb/library/yql/providers/config/yql_config_provider.cpp @@ -471,7 +471,7 @@ namespace { 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 5 arguments, but got " << node.ChildrenSize())); + ctx.AddError(TIssue(ctx.GetPosition(node.Pos()), TStringBuilder() << "Expected at least 4 arguments, but got " << node.ChildrenSize())); return false; } From e52c632e84430db4ead489f60a44a020ddc86452 Mon Sep 17 00:00:00 2001 From: Vitaly Stoyan Date: Mon, 5 Feb 2024 23:28:28 +0000 Subject: [PATCH 3/3] fix --- .../providers/config/yql_config_provider.cpp | 23 ++++++++++--------- 1 file changed, 12 insertions(+), 11 deletions(-) diff --git a/ydb/library/yql/providers/config/yql_config_provider.cpp b/ydb/library/yql/providers/config/yql_config_provider.cpp index 64460d39e276..a174b6bcd2d2 100644 --- a/ydb/library/yql/providers/config/yql_config_provider.cpp +++ b/ydb/library/yql/providers/config/yql_config_provider.cpp @@ -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; } } @@ -276,7 +276,7 @@ namespace { return res; } - if (!ApplyFlag(ctx.GetPosition(node->Child(2)->Pos()), command, args, ctx)) { + if (!ApplyFlag(ctx.GetPosition(node->Child(2)->Pos()), command, args, ctx, node->UniqueId())) { return {}; } @@ -479,7 +479,7 @@ namespace { return true; } - if (!PendingEvaluationFiles.insert(TString(node.Child(3)->Content())).second) { + 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; } @@ -490,7 +490,8 @@ namespace { } } - bool ApplyFlag(const TPosition& pos, const TStringBuf name, const TVector& args, TExprContext& ctx) { + bool ApplyFlag(const TPosition& pos, const TStringBuf name, const TVector& args, TExprContext& ctx, + ui64 nodeUniqueId) { if (!IsSettingAllowed(pos, name, ctx)) { return false; } @@ -504,7 +505,7 @@ namespace { return false; } } else if (name == "AddFileByUrl") { - if (!AddFileByUrl(pos, args, ctx)) { + if (!AddFileByUrl(pos, args, ctx, nodeUniqueId)) { return false; } } else if (name == "SetFileOption") { @@ -512,7 +513,7 @@ namespace { return false; } } else if (name == "AddFolderByUrl") { - if (!AddFolderByUrl(pos, args, ctx)) { + if (!AddFolderByUrl(pos, args, ctx, nodeUniqueId)) { return false; } } else if (name == "SetPackageVersion") { @@ -1015,13 +1016,13 @@ namespace { return true; } - bool AddFileByUrl(const TPosition& pos, const TVector& args, TExprContext& ctx) { + bool AddFileByUrl(const TPosition& pos, const TVector& 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])); + PendingEvaluationFiles.erase({TString(args[0]),nodeUniqueId}); TStringBuf token = args.size() == 3 ? args[2] : TStringBuf(); if (token) { if (auto cred = Types.Credentials->FindCredential(token)) { @@ -1135,13 +1136,13 @@ namespace { return url; } - bool AddFolderByUrl(const TPosition& pos, const TVector& args, TExprContext& ctx) { + bool AddFolderByUrl(const TPosition& pos, const TVector& 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])); + PendingEvaluationFiles.erase({TString(args[0]),nodeUniqueId}); TStringBuf token = args.size() == 3 ? args[2] : TStringBuf(); if (token) { if (auto cred = Types.Credentials->FindCredential(token)) { @@ -1238,7 +1239,7 @@ namespace { TString Username; const TAllowSettingPolicy Policy; TOperationStatistics Statistics; - THashSet PendingEvaluationFiles; + THashSet> PendingEvaluationFiles; }; }