Skip to content

don't remove DESC sorting from plan for sys view #28937

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

Merged
merged 2 commits into from
Nov 16, 2025
Merged

don't remove DESC sorting from plan for sys view #28937

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
fe21f71
implement the unit test
gridnevvvit Nov 15, 2025
a32e622
don't remove DESC sorting for sys view reads
gridnevvvit Nov 15, 2025
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
14 changes: 14 additions & 0 deletions ydb/core/kqp/opt/physical/kqp_opt_phy_sort.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,13 @@ TExprBase KqpRemoveRedundantSortOverReadTable(TExprBase node, TExprContext& ctx,
}

if (direction == ESortDirection::Reverse) {
// For sys views, we need to set reverse flag even if UseSource returns false
// because sys view actors can handle reverse direction
bool isSysView = tableDesc.Metadata->Kind == EKikimrTableKind::SysView;
if (isSysView) {
return node;
}
Comment on lines +96 to +101
Copy link

Copilot AI Nov 15, 2025

Choose a reason for hiding this comment

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

The logic for checking if a table is a sys view is duplicated in both KqpRemoveRedundantSortOverReadTable (lines 96-101) and KqpRemoveRedundantSortOverReadTableFSM (lines 216-219). Consider extracting this check into a helper function to reduce code duplication and improve maintainability.

Copilot uses AI. Check for mistakes.

if (!UseSource(kqpCtx, tableDesc) && kqpCtx.IsScanQuery()) {
return node;
}
Expand Down Expand Up @@ -206,6 +213,13 @@ TExprBase KqpRemoveRedundantSortOverReadTableFSM(
}

if (isReversed) {
// For sys views, we need to set reverse flag even if UseSource returns false
// because sys view actors can handle reverse direction
bool isSysView = tableDesc.Metadata->Kind == EKikimrTableKind::SysView;
if (isSysView) {
Comment on lines +216 to +219
Copy link

Copilot AI Nov 15, 2025

Choose a reason for hiding this comment

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

The sys view check is embedded in a complex boolean expression. This differs from the implementation in KqpRemoveRedundantSortOverReadTable (lines 96-101) where the sys view check uses an early return. Consider using a consistent approach in both functions for better code clarity and maintainability.

Copilot uses AI. Check for mistakes.
return node;
}

if (!UseSource(kqpCtx, tableDesc) && kqpCtx.IsScanQuery()) {
return node;
}
Expand Down
291 changes: 288 additions & 3 deletions ydb/core/kqp/ut/sysview/kqp_sys_view_ut.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -564,6 +564,291 @@ order by SessionId;)", "%Y-%m-%d %H:%M:%S %Z", sessionsSet.front().GetId().data(
CompareYson(expected, StreamResultToYson(it));
}

Y_UNIT_TEST(NodesOrderByDesc) {
// Test to reproduce issue #12585: ORDER BY DESC doesn't work for sys views
// The sys view actors ignore the direction flag and don't guarantee order
TKikimrRunner kikimr("", KikimrDefaultUtDomainRoot, 5);
auto client = kikimr.GetQueryClient();
auto session = client.GetSession().GetValueSync().GetSession();

ui32 offset = kikimr.GetTestServer().GetRuntime()->GetNodeId(0);

auto result = session.ExecuteQuery(R"(--!syntax_v1
SELECT NodeId, Host
FROM `/Root/.sys/nodes`
ORDER BY NodeId DESC
)", NYdb::NQuery::TTxControl::NoTx()).GetValueSync();

UNIT_ASSERT_C(result.IsSuccess(), result.GetIssues().ToString());

// Collect all results
TVector<ui32> nodeIds;
auto resultSet = result.GetResultSet(0);
NYdb::TResultSetParser parser(resultSet);
while (parser.TryNextRow()) {
auto nodeId = parser.ColumnParser("NodeId").GetOptionalUint32().value();
nodeIds.push_back(nodeId);
}

// Verify we got all 5 nodes
UNIT_ASSERT_VALUES_EQUAL(nodeIds.size(), 5);

// Verify results are in descending order (this should fail if the bug exists)
// According to issue #12585, sys view actors ignore the direction flag
// and don't guarantee order, so this assertion should fail
for (size_t i = 1; i < nodeIds.size(); ++i) {
UNIT_ASSERT_C(nodeIds[i - 1] >= nodeIds[i],
TStringBuilder() << "Results not in descending order: "
<< "nodeIds[" << (i - 1) << "] = " << nodeIds[i - 1]
<< " < nodeIds[" << i << "] = " << nodeIds[i]
<< ". ORDER BY DESC is being ignored by sys view actors.");
}

// Verify exact expected order: offset+4, offset+3, offset+2, offset+1, offset
UNIT_ASSERT_VALUES_EQUAL(nodeIds[0], offset + 4);
UNIT_ASSERT_VALUES_EQUAL(nodeIds[1], offset + 3);
UNIT_ASSERT_VALUES_EQUAL(nodeIds[2], offset + 2);
UNIT_ASSERT_VALUES_EQUAL(nodeIds[3], offset + 1);
UNIT_ASSERT_VALUES_EQUAL(nodeIds[4], offset);
}

Y_UNIT_TEST(PartitionStatsOrderByDesc) {
// Test ORDER BY DESC for partition_stats sys view
// Primary key: OwnerId, PathId, PartIdx, FollowerId
TKikimrRunner kikimr;
auto client = kikimr.GetQueryClient();
auto session = client.GetSession().GetValueSync().GetSession();

auto result = session.ExecuteQuery(R"(--!syntax_v1
SELECT OwnerId, PathId, PartIdx, FollowerId, Path
FROM `/Root/.sys/partition_stats`
ORDER BY OwnerId DESC, PathId DESC, PartIdx DESC, FollowerId DESC
)", NYdb::NQuery::TTxControl::NoTx()).GetValueSync();

UNIT_ASSERT_C(result.IsSuccess(), result.GetIssues().ToString());

// Collect all results
struct TPartitionKey {
ui64 OwnerId;
ui64 PathId;
ui64 PartIdx;
ui32 FollowerId;
};
TVector<TPartitionKey> partitionKeys;
auto resultSet = result.GetResultSet(0);
NYdb::TResultSetParser parser(resultSet);
while (parser.TryNextRow()) {
auto ownerId = parser.ColumnParser("OwnerId").GetOptionalUint64().value();
auto pathId = parser.ColumnParser("PathId").GetOptionalUint64().value();
auto partIdx = parser.ColumnParser("PartIdx").GetOptionalUint64().value();
auto followerId = parser.ColumnParser("FollowerId").GetOptionalUint32().value();
partitionKeys.push_back({ownerId, pathId, partIdx, followerId});
}

// Verify we got some results
UNIT_ASSERT_C(partitionKeys.size() > 0, "Expected at least one partition");

// Verify results are in descending order by OwnerId, PathId, PartIdx, FollowerId
for (size_t i = 1; i < partitionKeys.size(); ++i) {
const auto& prev = partitionKeys[i - 1];
const auto& curr = partitionKeys[i];

auto prevKey = std::tie(prev.OwnerId, prev.PathId, prev.PartIdx, prev.FollowerId);
auto currKey = std::tie(curr.OwnerId, curr.PathId, curr.PartIdx, curr.FollowerId);

UNIT_ASSERT_C(prevKey >= currKey,
TStringBuilder() << "Results not in descending order: "
<< "partitionKeys[" << (i - 1) << "] = (" << prev.OwnerId << ", " << prev.PathId << ", " << prev.PartIdx << ", " << prev.FollowerId << ")"
<< " < partitionKeys[" << i << "] = (" << curr.OwnerId << ", " << curr.PathId << ", " << curr.PartIdx << ", " << curr.FollowerId << ")"
<< ". ORDER BY DESC is being ignored by sys view actors.");
}
}

Y_UNIT_TEST(QuerySessionsOrderByDesc) {
// Test ORDER BY DESC for query_sessions sys view
NKikimrConfig::TFeatureFlags featureFlags;
featureFlags.SetEnableRealSystemViewPaths(true);
TKikimrSettings settings;
settings.SetWithSampleTables(false);
settings.SetFeatureFlags(featureFlags);
settings.SetAuthToken("root@builtin");
TKikimrRunner kikimr(settings);

auto client = kikimr.GetQueryClient();
auto session = client.GetSession().GetValueSync().GetSession();

// Create some sessions to have data
const size_t sessionsCount = 5;
std::vector<NYdb::NQuery::TSession> sessionsSet;
for(ui32 i = 0; i < sessionsCount; i++) {
sessionsSet.emplace_back(std::move(client.GetSession().GetValueSync().GetSession()));
}

// Wait a bit for sessions to be registered
::Sleep(TDuration::MilliSeconds(100));

auto result = session.ExecuteQuery(R"(--!syntax_v1
SELECT SessionId, State, NodeId
FROM `/Root/.sys/query_sessions`
ORDER BY SessionId DESC
)", NYdb::NQuery::TTxControl::NoTx()).GetValueSync();

UNIT_ASSERT_C(result.IsSuccess(), result.GetIssues().ToString());

// Collect all results
TVector<std::string> sessionIds;
auto resultSet = result.GetResultSet(0);
NYdb::TResultSetParser parser(resultSet);
while (parser.TryNextRow()) {
auto sessionId = parser.ColumnParser("SessionId").GetOptionalUtf8().value();
sessionIds.push_back(sessionId);
}

// Verify we got some results
UNIT_ASSERT_C(sessionIds.size() > 0, "Expected at least one session");

// Verify results are in descending order (lexicographically)
for (size_t i = 1; i < sessionIds.size(); ++i) {
UNIT_ASSERT_C(sessionIds[i - 1] >= sessionIds[i],
TStringBuilder() << "Results not in descending order: "
<< "sessionIds[" << (i - 1) << "] = \"" << sessionIds[i - 1] << "\""
<< " < sessionIds[" << i << "] = \"" << sessionIds[i] << "\""
<< ". ORDER BY DESC is being ignored by sys view actors.");
}
}

Y_UNIT_TEST(CompileCacheQueriesOrderByDesc) {
// Test ORDER BY DESC for compile_cache_queries sys view
// Primary key: NodeId, QueryId
auto serverSettings = TKikimrSettings().SetKqpSettings({ NKikimrKqp::TKqpSetting() });
serverSettings.AppConfig.MutableTableServiceConfig()->SetEnableImplicitQueryParameterTypes(true);
TKikimrRunner kikimr(serverSettings);
kikimr.GetTestServer().GetRuntime()->GetAppData().FeatureFlags.SetEnableCompileCacheView(true);
auto client = kikimr.GetQueryClient();
auto session = client.GetSession().GetValueSync().GetSession();

// Execute some queries to populate compile cache
auto tableClient = kikimr.GetTableClient();
for (ui32 i = 0; i < 3; ++i) {
auto tableSession = tableClient.CreateSession().GetValueSync().GetSession();
auto paramsBuilder = TParamsBuilder();
paramsBuilder.AddParam("$k").Uint64(i).Build();
auto executedResult = tableSession.ExecuteDataQuery(
R"(DECLARE $k AS Uint64;
SELECT COUNT(*) FROM `/Root/EightShard` WHERE Key = $k;)",
TTxControl::BeginTx().CommitTx(),
paramsBuilder.Build()
).GetValueSync();
UNIT_ASSERT_C(executedResult.IsSuccess(), executedResult.GetIssues().ToString());
}

// Wait a bit for compile cache to be updated
::Sleep(TDuration::MilliSeconds(100));

auto result = session.ExecuteQuery(R"(--!syntax_v1
SELECT NodeId, QueryId, Query, CompilationDuration
FROM `/Root/.sys/compile_cache_queries`
ORDER BY NodeId DESC, QueryId DESC
)", NYdb::NQuery::TTxControl::NoTx()).GetValueSync();

UNIT_ASSERT_C(result.IsSuccess(), result.GetIssues().ToString());

// Collect all results
struct TCompileCacheKey {
ui32 NodeId;
std::string QueryId;
};
TVector<TCompileCacheKey> compileCacheKeys;
auto resultSet = result.GetResultSet(0);
NYdb::TResultSetParser parser(resultSet);
while (parser.TryNextRow()) {
auto nodeId = parser.ColumnParser("NodeId").GetOptionalUint32().value();
auto queryId = parser.ColumnParser("QueryId").GetOptionalUtf8().value();
compileCacheKeys.push_back({nodeId, queryId});
}

// Verify we got some results
if (compileCacheKeys.size() > 0) {
// Verify results are in descending order by NodeId, then QueryId
for (size_t i = 1; i < compileCacheKeys.size(); ++i) {
const auto& prev = compileCacheKeys[i - 1];
const auto& curr = compileCacheKeys[i];

auto prevKey = std::tie(prev.NodeId, prev.QueryId);
auto currKey = std::tie(curr.NodeId, curr.QueryId);

UNIT_ASSERT_C(prevKey >= currKey,
TStringBuilder() << "Results not in descending order: "
<< "compileCacheKeys[" << (i - 1) << "] = (" << prev.NodeId << ", \"" << prev.QueryId << "\")"
<< " < compileCacheKeys[" << i << "] = (" << curr.NodeId << ", \"" << curr.QueryId << "\")"
<< ". ORDER BY DESC is being ignored by sys view actors.");
}
}
}

Y_UNIT_TEST(TopQueriesOrderByDesc) {
// Test ORDER BY DESC for top_queries sys views
TKikimrRunner kikimr("", KikimrDefaultUtDomainRoot, 3);
auto client = kikimr.GetQueryClient();
auto session = client.GetSession().GetValueSync().GetSession();

// Execute some queries to populate stats
auto tableClient = kikimr.GetTableClient();
auto tableSession = tableClient.CreateSession().GetValueSync().GetSession();
{
auto result = tableSession.ExecuteDataQuery(Q_(R"(
SELECT * FROM `/Root/TwoShard`
)"), TTxControl::BeginTx().CommitTx()).GetValueSync();
UNIT_ASSERT_VALUES_EQUAL(result.GetStatus(), EStatus::SUCCESS);
}
{
auto result = tableSession.ExecuteDataQuery(Q_(R"(
SELECT * FROM `/Root/EightShard`
)"), TTxControl::BeginTx().CommitTx()).GetValueSync();
UNIT_ASSERT_VALUES_EQUAL(result.GetStatus(), EStatus::SUCCESS);
}

// Wait a bit for stats to be collected
::Sleep(TDuration::MilliSeconds(500));

// Test top_queries_by_read_bytes_one_minute
auto result = session.ExecuteQuery(R"(--!syntax_v1
SELECT IntervalEnd, Rank, ReadBytes
FROM `/Root/.sys/top_queries_by_read_bytes_one_minute`
ORDER BY IntervalEnd DESC, Rank DESC
)", NYdb::NQuery::TTxControl::NoTx()).GetValueSync();

UNIT_ASSERT_C(result.IsSuccess(), result.GetIssues().ToString());

// Collect all results
TVector<std::pair<TInstant, ui32>> intervalEndRank;
auto resultSet = result.GetResultSet(0);
NYdb::TResultSetParser parser(resultSet);
while (parser.TryNextRow()) {
auto intervalEnd = parser.ColumnParser("IntervalEnd").GetOptionalTimestamp().value();
auto rank = parser.ColumnParser("Rank").GetOptionalUint32().value();
intervalEndRank.push_back({intervalEnd, rank});
}

// Verify we got some results (may be empty if no stats collected yet)
if (intervalEndRank.size() > 0) {
// Verify results are in descending order by IntervalEnd, then Rank
for (size_t i = 1; i < intervalEndRank.size(); ++i) {
const auto& prev = intervalEndRank[i - 1];
const auto& curr = intervalEndRank[i];

auto prevKey = std::tie(prev.first, prev.second);
auto currKey = std::tie(curr.first, curr.second);

UNIT_ASSERT_C(prevKey >= currKey,
TStringBuilder() << "Results not in descending order: "
<< "intervalEndRank[" << (i - 1) << "] = (" << prev.first.ToString() << ", " << prev.second << ")"
<< " < intervalEndRank[" << i << "] = (" << curr.first.ToString() << ", " << curr.second << ")"
<< ". ORDER BY DESC is being ignored by sys view actors.");
}
}
}

Y_UNIT_TEST(QueryStatsSimple) {
auto checkTable = [&] (const TStringBuf tableName) {
TKikimrRunner kikimr("", KikimrDefaultUtDomainRoot, 3);
Expand Down Expand Up @@ -920,15 +1205,15 @@ order by SessionId;)", "%Y-%m-%d %H:%M:%S %Z", sessionsSet.front().GetId().data(
);
)").ExtractValueSync();
UNIT_ASSERT_VALUES_EQUAL_C(createResult.GetStatus(), EStatus::SUCCESS, createResult.GetIssues());

auto insertResult = session.ExecuteDataQuery(R"(
INSERT INTO TestTable (Key, Value) VALUES
(42, "val"), (NULL, "val1");
)", TTxControl::BeginTx().CommitTx()).GetValueSync();
UNIT_ASSERT_C(insertResult.IsSuccess(), insertResult.GetIssues().ToString());
}


auto session = db.CreateSession().GetValueSync().GetSession();
TString query = R"(
SELECT * FROM TestTable WHERE Key in [42, NULL];
Expand Down
Loading