don't remove DESC sorting from plan for sys view #28937
Conversation
gridnevvvit
force-pushed
the
fix-the-unit-test
branch
from
28d2049 to
62f20bc
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR fixes issue #12585 where ORDER BY DESC was being incorrectly removed from query plans for system views. The fix prevents the optimizer from removing DESC sorting when querying sys views, as these actors can handle reverse direction.
Key changes:
- Added logic to preserve DESC sorting for sys views in query optimization
- Added a test case to verify DESC ordering works correctly for the nodes sys view
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| ydb/core/kqp/opt/physical/kqp_opt_phy_sort.cpp | Modified two optimization functions to check if table is a sys view and preserve reverse sorting accordingly |
| ydb/core/kqp/ut/sysview/kqp_sys_view_ut.cpp | Added NodesOrderByDesc test to verify DESC ordering works, plus minor whitespace cleanup |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // 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; | ||
| } |
There was a problem hiding this comment.
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.
| // 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) { |
There was a problem hiding this comment.
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.
|
🟢 |
gridnevvvit
force-pushed
the
fix-the-unit-test
branch
from
62f20bc to
8859610
Compare
|
⚪ ⚪ Ya make output | Test bloat | Test bloat
⚪ Ya make output | Test bloat | Test bloat | Test bloat
🟢
*please be aware that the difference is based on comparing your commit and the last completed build from the post-commit, check comparation |
|
⚪
🟢
*please be aware that the difference is based on comparing your commit and the last completed build from the post-commit, check comparation |
gridnevvvit
force-pushed
the
fix-the-unit-test
branch
from
8859610 to
a32e622
Compare
|
⚪ ⚪ Ya make output | Test bloat | Test bloat
🟢
*please be aware that the difference is based on comparing your commit and the last completed build from the post-commit, check comparation |
|
⚪
🟢
*please be aware that the difference is based on comparing your commit and the last completed build from the post-commit, check comparation |
gridnevvvit
changed the title
2 participants
Changelog entry
Changelog category
Description for reviewers
fixes #12585