From 0924d7e1f59e15a3994abf6cf28a56b9efabb8b1 Mon Sep 17 00:00:00 2001 From: Vitalii Gridnev Date: Fri, 7 Nov 2025 20:00:17 +0300 Subject: [PATCH 1/3] implement the regression tests --- ydb/core/kqp/ut/opt/kqp_returning_ut.cpp | 161 +++++++++++++++++++++++ 1 file changed, 161 insertions(+) diff --git a/ydb/core/kqp/ut/opt/kqp_returning_ut.cpp b/ydb/core/kqp/ut/opt/kqp_returning_ut.cpp index dbfd7bdbbdbf..bd31437893ca 100644 --- a/ydb/core/kqp/ut/opt/kqp_returning_ut.cpp +++ b/ydb/core/kqp/ut/opt/kqp_returning_ut.cpp @@ -377,6 +377,25 @@ TString ExecuteReturningQuery(TKikimrRunner& kikimr, bool queryService, TString return FormatResultSetYson(result.GetResultSet(0)); } +TString ExecuteReturningQueryWithParams(TKikimrRunner& kikimr, bool queryService, TString query, const TParams& params) { + if (queryService) { + auto qdb = kikimr.GetQueryClient(); + auto qSession = qdb.GetSession().GetValueSync().GetSession(); + auto settings = NYdb::NQuery::TExecuteQuerySettings() + .Syntax(NYdb::NQuery::ESyntax::YqlV1); + auto result = qSession.ExecuteQuery( + query, NYdb::NQuery::TTxControl::BeginTx().CommitTx(), params, settings).ExtractValueSync(); + UNIT_ASSERT_VALUES_EQUAL_C(result.GetStatus(), EStatus::SUCCESS, result.GetIssues().ToString()); + return FormatResultSetYson(result.GetResultSet(0)); + } + + auto db = kikimr.GetTableClient(); + auto session = db.CreateSession().GetValueSync().GetSession(); + auto result = session.ExecuteDataQuery(query, TTxControl::BeginTx().CommitTx(), params).ExtractValueSync(); + UNIT_ASSERT_VALUES_EQUAL_C(result.GetStatus(), EStatus::SUCCESS, result.GetIssues().ToString()); + return FormatResultSetYson(result.GetResultSet(0)); +} + Y_UNIT_TEST_TWIN(ReturningWorks, QueryService) { auto kikimr = DefaultKikimrRunner(); auto db = kikimr.GetTableClient(); @@ -663,6 +682,148 @@ Y_UNIT_TEST(ReturningTypes) { } } +Y_UNIT_TEST_TWIN(ReturningUpsertAsTableListNotNullOnly, QueryService) { + // Test for issue #27021: Query fails when using RETURNING CLAUSE with UPSERT + // to table with only NOT NULL fields and query parameters of type List + auto kikimr = DefaultKikimrRunner(); + + auto client = kikimr.GetTableClient(); + auto session = client.CreateSession().GetValueSync().GetSession(); + + // Create table with only NOT NULL fields + const auto queryCreate = Q_(R"( + --!syntax_v1 + CREATE TABLE test_table ( + id Uint64 NOT NULL, + value Utf8 NOT NULL, + PRIMARY KEY (id) + ); + )"); + + auto resultCreate = session.ExecuteSchemeQuery(queryCreate).GetValueSync(); + UNIT_ASSERT_C(resultCreate.IsSuccess(), resultCreate.GetIssues().ToString()); + + { + // Test case from issue #27021 + const auto query = Q_(R"( + --!syntax_v1 + DECLARE $data AS List>; + + UPSERT INTO test_table + SELECT * FROM AS_TABLE($data) + RETURNING *; + )"); + + auto paramsBuilder = TParamsBuilder(); + auto& dataParam = paramsBuilder.AddParam("$data"); + + dataParam.BeginList(); + dataParam.AddListItem() + .BeginStruct() + .AddMember("id") + .Uint64(1) + .AddMember("value") + .Utf8("test1") + .EndStruct(); + dataParam.AddListItem() + .BeginStruct() + .AddMember("id") + .Uint64(2) + .AddMember("value") + .Utf8("test2") + .EndStruct(); + dataParam.EndList(); + dataParam.Build(); + + auto params = paramsBuilder.Build(); + + // This should succeed, but currently fails with infinite loop error + CompareYson(R"([[[1u];["test1"]];[[2u];["test2"]]])", + ExecuteReturningQueryWithParams(kikimr, QueryService, query, params)); + } + + { + // Test with explicit field names in SELECT clause + const auto query = Q_(R"( + --!syntax_v1 + DECLARE $data AS List>; + + UPSERT INTO test_table + SELECT id, value FROM AS_TABLE($data) + RETURNING *; + )"); + + auto paramsBuilder = TParamsBuilder(); + auto& dataParam = paramsBuilder.AddParam("$data"); + + dataParam.BeginList(); + dataParam.AddListItem() + .BeginStruct() + .AddMember("id") + .Uint64(3) + .AddMember("value") + .Utf8("test3") + .EndStruct(); + dataParam.EndList(); + dataParam.Build(); + + auto params = paramsBuilder.Build(); + + CompareYson(R"([[[3u];["test3"]]])", + ExecuteReturningQueryWithParams(kikimr, QueryService, query, params)); + } +} + +Y_UNIT_TEST_TWIN(ReturningUpsertAsTableListWithNullable, QueryService) { + // Test that nullable columns work correctly (this should work even before the fix) + auto kikimr = DefaultKikimrRunner(); + + auto client = kikimr.GetTableClient(); + auto session = client.CreateSession().GetValueSync().GetSession(); + + const auto queryCreate = Q_(R"( + --!syntax_v1 + CREATE TABLE test_table_nullable ( + id Uint64 NOT NULL, + value Utf8, + PRIMARY KEY (id) + ); + )"); + + auto resultCreate = session.ExecuteSchemeQuery(queryCreate).GetValueSync(); + UNIT_ASSERT_C(resultCreate.IsSuccess(), resultCreate.GetIssues().ToString()); + + { + const auto query = Q_(R"( + --!syntax_v1 + DECLARE $data AS List>; + + UPSERT INTO test_table_nullable + SELECT * FROM AS_TABLE($data) + RETURNING *; + )"); + + auto paramsBuilder = TParamsBuilder(); + auto& dataParam = paramsBuilder.AddParam("$data"); + + dataParam.BeginList(); + dataParam.AddListItem() + .BeginStruct() + .AddMember("id") + .Uint64(1) + .AddMember("value") + .OptionalUtf8("test1") + .EndStruct(); + dataParam.EndList(); + dataParam.Build(); + + auto params = paramsBuilder.Build(); + + CompareYson(R"([[[1u];["test1"]]])", + ExecuteReturningQueryWithParams(kikimr, QueryService, query, params)); + } +} + } } // namespace NKikimr::NKqp From 4032e7411a82abe2909ae37261271cc83a3ccecc Mon Sep 17 00:00:00 2001 From: Vitalii Gridnev Date: Fri, 7 Nov 2025 21:19:34 +0300 Subject: [PATCH 2/3] implement a fix and fix the test --- .../effects/kqp_opt_phy_returning.cpp | 50 +++++++------------ ydb/core/kqp/ut/opt/kqp_returning_ut.cpp | 6 +-- 2 files changed, 21 insertions(+), 35 deletions(-) diff --git a/ydb/core/kqp/opt/physical/effects/kqp_opt_phy_returning.cpp b/ydb/core/kqp/opt/physical/effects/kqp_opt_phy_returning.cpp index a8bfed7351b1..e51b2b2b7059 100644 --- a/ydb/core/kqp/opt/physical/effects/kqp_opt_phy_returning.cpp +++ b/ydb/core/kqp/opt/physical/effects/kqp_opt_phy_returning.cpp @@ -173,6 +173,7 @@ TExprBase KqpBuildReturning(TExprBase node, TExprContext& ctx, const TTypeAnnota return TExprBase(ctx.ChangeChild(*returning.Raw(), TKqlReturningList::idx_Update, inputExpr.Ptr())); }; + if (auto maybeList = returning.Update().Maybe()) { for (auto item : maybeList.Cast()) { if (auto upsert = item.Maybe()) { @@ -218,48 +219,33 @@ TExprBase KqpBuildReturning(TExprBase node, TExprContext& ctx, const TTypeAnnota return node; } -TExprBase KqpRewriteReturningUpsert(TExprBase node, TExprContext& ctx, const TKqpOptimizeContext&) { - auto upsert = node.Cast(); - if (upsert.ReturningColumns().Empty()) { +template +TExprBase KqpRewriteReturningInput(TExprBase node, TExprContext& ctx) { + auto effect = node.Cast(); + if (effect.ReturningColumns().Empty()) { return node; } - if (upsert.Input().Maybe() || upsert.Input().Maybe()) { + if (effect.Input().template Maybe() || effect.Input().template Maybe()) { return node; } - return - Build(ctx, upsert.Pos()) - .Input() - .Input(upsert.Input()) - .Build() - .Table(upsert.Table()) - .Columns(upsert.Columns()) - .IsBatch(upsert.IsBatch()) - .Settings(upsert.Settings()) - .ReturningColumns(upsert.ReturningColumns()) - .Done(); -} - -TExprBase KqpRewriteReturningDelete(TExprBase node, TExprContext& ctx, const TKqpOptimizeContext&) { - auto del = node.Cast(); - if (del.ReturningColumns().Empty()) { + if (NDq::IsDqPureExpr(effect.Input())) { return node; } - if (del.Input().Maybe() || del.Input().Maybe()) { - return node; - } + auto newInput = Build(ctx, effect.Pos()) + .Input(effect.Input()) + .Done().Ptr(); - return - Build(ctx, del.Pos()) - .Input() - .Input(del.Input()) - .Build() - .Table(del.Table()) - .IsBatch(del.IsBatch()) - .ReturningColumns(del.ReturningColumns()) - .Done(); + return TExprBase(ctx.ChangeChild(*effect.Raw(), TEffect::idx_ReturningColumns, std::move(newInput))); +} + +TExprBase KqpRewriteReturningDelete(TExprBase node, TExprContext& ctx, const TKqpOptimizeContext&) { + return KqpRewriteReturningInput(node, ctx); } +TExprBase KqpRewriteReturningUpsert(TExprBase node, TExprContext& ctx, const TKqpOptimizeContext&) { + return KqpRewriteReturningInput(node, ctx); +} } // namespace NKikimr::NKqp::NOpt diff --git a/ydb/core/kqp/ut/opt/kqp_returning_ut.cpp b/ydb/core/kqp/ut/opt/kqp_returning_ut.cpp index bd31437893ca..524d669f8a58 100644 --- a/ydb/core/kqp/ut/opt/kqp_returning_ut.cpp +++ b/ydb/core/kqp/ut/opt/kqp_returning_ut.cpp @@ -738,7 +738,7 @@ Y_UNIT_TEST_TWIN(ReturningUpsertAsTableListNotNullOnly, QueryService) { auto params = paramsBuilder.Build(); // This should succeed, but currently fails with infinite loop error - CompareYson(R"([[[1u];["test1"]];[[2u];["test2"]]])", + CompareYson(R"([[1u;"test1"];[2u;"test2"]])", ExecuteReturningQueryWithParams(kikimr, QueryService, query, params)); } @@ -769,7 +769,7 @@ Y_UNIT_TEST_TWIN(ReturningUpsertAsTableListNotNullOnly, QueryService) { auto params = paramsBuilder.Build(); - CompareYson(R"([[[3u];["test3"]]])", + CompareYson(R"([[3u;"test3"]])", ExecuteReturningQueryWithParams(kikimr, QueryService, query, params)); } } @@ -819,7 +819,7 @@ Y_UNIT_TEST_TWIN(ReturningUpsertAsTableListWithNullable, QueryService) { auto params = paramsBuilder.Build(); - CompareYson(R"([[[1u];["test1"]]])", + CompareYson(R"([[1u;["test1"]]])", ExecuteReturningQueryWithParams(kikimr, QueryService, query, params)); } } From ee8c231d2e9169fb8c2fdee97e948765b554217d Mon Sep 17 00:00:00 2001 From: Vitalii Gridnev Date: Fri, 7 Nov 2025 21:58:05 +0300 Subject: [PATCH 3/3] add more tests --- .../effects/kqp_opt_phy_returning.cpp | 49 +++--- ydb/core/kqp/ut/opt/kqp_returning_ut.cpp | 143 ++++++++++++++++++ 2 files changed, 175 insertions(+), 17 deletions(-) diff --git a/ydb/core/kqp/opt/physical/effects/kqp_opt_phy_returning.cpp b/ydb/core/kqp/opt/physical/effects/kqp_opt_phy_returning.cpp index e51b2b2b7059..d741f1ebc02a 100644 --- a/ydb/core/kqp/opt/physical/effects/kqp_opt_phy_returning.cpp +++ b/ydb/core/kqp/opt/physical/effects/kqp_opt_phy_returning.cpp @@ -219,33 +219,48 @@ TExprBase KqpBuildReturning(TExprBase node, TExprContext& ctx, const TTypeAnnota return node; } -template -TExprBase KqpRewriteReturningInput(TExprBase node, TExprContext& ctx) { - auto effect = node.Cast(); - if (effect.ReturningColumns().Empty()) { +TExprBase KqpRewriteReturningUpsert(TExprBase node, TExprContext& ctx, const TKqpOptimizeContext&) { + auto upsert = node.Cast(); + if (upsert.ReturningColumns().Empty()) { return node; } - if (effect.Input().template Maybe() || effect.Input().template Maybe()) { + if (upsert.Input().Maybe() || upsert.Input().Maybe() || upsert.Input().Maybe()) { return node; } - if (NDq::IsDqPureExpr(effect.Input())) { + return + Build(ctx, upsert.Pos()) + .Input() + .Input(upsert.Input()) + .Build() + .Table(upsert.Table()) + .Columns(upsert.Columns()) + .IsBatch(upsert.IsBatch()) + .Settings(upsert.Settings()) + .ReturningColumns(upsert.ReturningColumns()) + .Done(); +} + +TExprBase KqpRewriteReturningDelete(TExprBase node, TExprContext& ctx, const TKqpOptimizeContext&) { + auto del = node.Cast(); + if (del.ReturningColumns().Empty()) { return node; } - auto newInput = Build(ctx, effect.Pos()) - .Input(effect.Input()) - .Done().Ptr(); - - return TExprBase(ctx.ChangeChild(*effect.Raw(), TEffect::idx_ReturningColumns, std::move(newInput))); -} + if (del.Input().Maybe() || del.Input().Maybe() || del.Input().Maybe()) { + return node; + } -TExprBase KqpRewriteReturningDelete(TExprBase node, TExprContext& ctx, const TKqpOptimizeContext&) { - return KqpRewriteReturningInput(node, ctx); + return + Build(ctx, del.Pos()) + .Input() + .Input(del.Input()) + .Build() + .Table(del.Table()) + .IsBatch(del.IsBatch()) + .ReturningColumns(del.ReturningColumns()) + .Done(); } -TExprBase KqpRewriteReturningUpsert(TExprBase node, TExprContext& ctx, const TKqpOptimizeContext&) { - return KqpRewriteReturningInput(node, ctx); -} } // namespace NKikimr::NKqp::NOpt diff --git a/ydb/core/kqp/ut/opt/kqp_returning_ut.cpp b/ydb/core/kqp/ut/opt/kqp_returning_ut.cpp index 524d669f8a58..cef78e75a8b1 100644 --- a/ydb/core/kqp/ut/opt/kqp_returning_ut.cpp +++ b/ydb/core/kqp/ut/opt/kqp_returning_ut.cpp @@ -772,6 +772,78 @@ Y_UNIT_TEST_TWIN(ReturningUpsertAsTableListNotNullOnly, QueryService) { CompareYson(R"([[3u;"test3"]])", ExecuteReturningQueryWithParams(kikimr, QueryService, query, params)); } + + { + // Test DELETE with RETURNING using AS_TABLE with List parameter (same issue #27021) + // First insert some data without RETURNING + const auto insertQuery = Q_(R"( + --!syntax_v1 + DECLARE $data AS List>; + + UPSERT INTO test_table + SELECT * FROM AS_TABLE($data); + )"); + + auto insertParamsBuilder = TParamsBuilder(); + auto& insertDataParam = insertParamsBuilder.AddParam("$data"); + + insertDataParam.BeginList(); + insertDataParam.AddListItem() + .BeginStruct() + .AddMember("id") + .Uint64(10) + .AddMember("value") + .Utf8("delete1") + .EndStruct(); + insertDataParam.EndList(); + insertDataParam.Build(); + + auto insertParams = insertParamsBuilder.Build(); + + // Execute insert without RETURNING + if (QueryService) { + auto qdb = kikimr.GetQueryClient(); + auto qSession = qdb.GetSession().GetValueSync().GetSession(); + auto settings = NYdb::NQuery::TExecuteQuerySettings() + .Syntax(NYdb::NQuery::ESyntax::YqlV1); + auto insertResult = qSession.ExecuteQuery( + insertQuery, NYdb::NQuery::TTxControl::BeginTx().CommitTx(), insertParams, settings).ExtractValueSync(); + UNIT_ASSERT_VALUES_EQUAL_C(insertResult.GetStatus(), EStatus::SUCCESS, insertResult.GetIssues().ToString()); + } else { + auto db = kikimr.GetTableClient(); + auto session = db.CreateSession().GetValueSync().GetSession(); + auto insertResult = session.ExecuteDataQuery(insertQuery, TTxControl::BeginTx().CommitTx(), insertParams).ExtractValueSync(); + UNIT_ASSERT_VALUES_EQUAL_C(insertResult.GetStatus(), EStatus::SUCCESS, insertResult.GetIssues().ToString()); + } + + // Now DELETE with RETURNING using AS_TABLE + const auto deleteQuery = Q_(R"( + --!syntax_v1 + DECLARE $data AS List>; + + DELETE FROM test_table ON + SELECT * FROM AS_TABLE($data) + RETURNING *; + )"); + + auto deleteParamsBuilder = TParamsBuilder(); + auto& deleteDataParam = deleteParamsBuilder.AddParam("$data"); + + deleteDataParam.BeginList(); + deleteDataParam.AddListItem() + .BeginStruct() + .AddMember("id") + .Uint64(10) + .EndStruct(); + deleteDataParam.EndList(); + deleteDataParam.Build(); + + auto deleteParams = deleteParamsBuilder.Build(); + + // This should succeed, but currently fails with infinite loop error + CompareYson(R"([[10u;"delete1"]])", + ExecuteReturningQueryWithParams(kikimr, QueryService, deleteQuery, deleteParams)); + } } Y_UNIT_TEST_TWIN(ReturningUpsertAsTableListWithNullable, QueryService) { @@ -822,6 +894,77 @@ Y_UNIT_TEST_TWIN(ReturningUpsertAsTableListWithNullable, QueryService) { CompareYson(R"([[1u;["test1"]]])", ExecuteReturningQueryWithParams(kikimr, QueryService, query, params)); } + + { + // Test DELETE with RETURNING using AS_TABLE with List parameter (with nullable columns) + // First insert some data without RETURNING + const auto insertQuery = Q_(R"( + --!syntax_v1 + DECLARE $data AS List>; + + UPSERT INTO test_table_nullable + SELECT * FROM AS_TABLE($data); + )"); + + auto insertParamsBuilder = TParamsBuilder(); + auto& insertDataParam = insertParamsBuilder.AddParam("$data"); + + insertDataParam.BeginList(); + insertDataParam.AddListItem() + .BeginStruct() + .AddMember("id") + .Uint64(20) + .AddMember("value") + .OptionalUtf8("delete_nullable1") + .EndStruct(); + insertDataParam.EndList(); + insertDataParam.Build(); + + auto insertParams = insertParamsBuilder.Build(); + + // Execute insert without RETURNING + if (QueryService) { + auto qdb = kikimr.GetQueryClient(); + auto qSession = qdb.GetSession().GetValueSync().GetSession(); + auto settings = NYdb::NQuery::TExecuteQuerySettings() + .Syntax(NYdb::NQuery::ESyntax::YqlV1); + auto insertResult = qSession.ExecuteQuery( + insertQuery, NYdb::NQuery::TTxControl::BeginTx().CommitTx(), insertParams, settings).ExtractValueSync(); + UNIT_ASSERT_VALUES_EQUAL_C(insertResult.GetStatus(), EStatus::SUCCESS, insertResult.GetIssues().ToString()); + } else { + auto db = kikimr.GetTableClient(); + auto session = db.CreateSession().GetValueSync().GetSession(); + auto insertResult = session.ExecuteDataQuery(insertQuery, TTxControl::BeginTx().CommitTx(), insertParams).ExtractValueSync(); + UNIT_ASSERT_VALUES_EQUAL_C(insertResult.GetStatus(), EStatus::SUCCESS, insertResult.GetIssues().ToString()); + } + + // Now DELETE with RETURNING using AS_TABLE + const auto deleteQuery = Q_(R"( + --!syntax_v1 + DECLARE $data AS List>; + + DELETE FROM test_table_nullable ON + SELECT * FROM AS_TABLE($data) + RETURNING *; + )"); + + auto deleteParamsBuilder = TParamsBuilder(); + auto& deleteDataParam = deleteParamsBuilder.AddParam("$data"); + + deleteDataParam.BeginList(); + deleteDataParam.AddListItem() + .BeginStruct() + .AddMember("id") + .Uint64(20) + .EndStruct(); + deleteDataParam.EndList(); + deleteDataParam.Build(); + + auto deleteParams = deleteParamsBuilder.Build(); + + CompareYson(R"([[20u;["delete_nullable1"]]])", + ExecuteReturningQueryWithParams(kikimr, QueryService, deleteQuery, deleteParams)); + } } }