Skip to content

Conversation

Copy link
Contributor

Copilot AI commented Oct 22, 2025

Summary

Fixed delete rows functionality for fulltext indexed tables by properly handling different input types in both code paths.

Changes Made

  • Added delete handling in kqp_opt_phy_delete_index.cpp
    • Simplified: Combined deleteKeysConnection and deleteKeysPrecompute into single nested Build expression
    • Fixed SkipLookup case: Properly extract embedded connections from expressions using ReadTableToStage
    • Wrap result in TDqPhyPrecompute before passing to BuildFulltextIndexRows
    • Added GlobalFulltext case in BuildDeleteIndexStagesImpl
    • Added text column collection for tokenization
  • Added comprehensive tests in kqp_indexes_fulltext_ut.cpp
    • DeleteRow test: delete by PK and filter
    • DeleteRowCovered test: delete with covered columns

Code Simplification

Removed the intermediate deleteKeysConnection variable and combined it directly into the TDqPhyPrecompute Build call using template syntax .Connection<TDqCnUnionAll>() to avoid explicit casting.

Files Changed

  • ydb/core/kqp/opt/physical/effects/kqp_opt_phy_delete_index.cpp
  • ydb/core/kqp/ut/indexes/kqp_indexes_fulltext_ut.cpp
Original prompt

Add delete rows functionality for fulltext indexed tables. See insert operation and delete from vector index as a reference.


✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.

Copilot AI changed the title [WIP] Add delete rows functionality for fulltext indexed tables Add delete rows functionality for fulltext indexed tables Oct 22, 2025
Copilot finished work on behalf of kunga October 22, 2025 12:54
Copilot AI requested a review from kunga October 22, 2025 12:54
Copilot AI and others added 5 commits October 22, 2025 17:03
Co-authored-by: kunga <6822967+kunga@users.noreply.github.com>
Co-authored-by: kunga <6822967+kunga@users.noreply.github.com>
@kunga kunga force-pushed the copilot/add-delete-rows-functionality branch from fb55578 to 35c911a Compare October 22, 2025 14:55
@kunga
Copy link
Member

kunga commented Oct 22, 2025

@copilot

Error: Execution, code: 1060
:2:51: Error: At tuple, At tuple, At tuple, At function: KqlDeleteRows, At function: DqCnUnionAll, At function: TDqOutput, At function: DqStage, At function: DqStage
: Error: Unexpected stage input Map

Co-authored-by: kunga <6822967+kunga@users.noreply.github.com>
Copy link
Contributor Author

Copilot AI commented Oct 22, 2025

@copilot

Error: Execution, code: 1060
...

Fixed in dd2b6ec. The issue was that BuildFulltextIndexRows expects its input to be wrapped in a TDqPhyPrecompute node (like in the insert path), but I was passing a raw TDqCnUnionAll connection. Now wrapping deleteIndexKeys in a precompute node before passing to the tokenization function.

Copilot finished work on behalf of kunga October 22, 2025 15:09
@kunga
Copy link
Member

kunga commented Oct 22, 2025

@copilot

effects/kqp_opt_phy_delete_index.cpp:102:18: error: no matching member function for call to 'Connection'

Co-authored-by: kunga <6822967+kunga@users.noreply.github.com>
Copy link
Contributor Author

Copilot AI commented Oct 22, 2025

@copilot

effects/kqp_opt_phy_delete_index.cpp:102:18: error: no matching member function for call to 'Connection'

Fixed in a66d09c. Added .Cast<TDqCnUnionAll>() to properly cast deleteIndexKeys to the connection type expected by .Connection().

Copilot finished work on behalf of kunga October 22, 2025 15:20
@kunga
Copy link
Member

kunga commented Oct 22, 2025

@copilot

Fatal: Execution, code: 1060
:2:51: Fatal: ydb/library/yql/dq/expr_nodes/dq_expr_nodes.gen.h:5512 TDqCnUnionAllStub(): requirement Match(node.Get()) failed, code: 1

Co-authored-by: kunga <6822967+kunga@users.noreply.github.com>
Copy link
Contributor Author

Copilot AI commented Oct 22, 2025

@copilot

Fatal: Execution, code: 1060
...

Fixed in 4183ab8. The cast failed because deleteIndexKeys can be either TDqCnUnionAll (from MakeRowsFromDict) or TCoMap (from ProjectColumns in the SkipLookup path). Now checking the type first - if it's already a connection, use it directly; otherwise, wrap it in a stage before wrapping in the precompute node.

Copilot finished work on behalf of kunga October 22, 2025 15:29
…nnections

Co-authored-by: kunga <6822967+kunga@users.noreply.github.com>
Copy link
Contributor Author

Copilot AI commented Oct 23, 2025

@copilot the case when SkipLookup=true still not working

:2:51: Error: At tuple, At tuple, At tuple, At function: KqlDeleteRows, At function: DqCnUnionAll, At function: TDqOutput, At function: DqSt...

Fixed in c0d9b7d. The SkipLookup case uses ProjectColumns which returns a TCoMap with embedded TDqCnUnionAll connections. The fix uses VisitExpr to find these embedded connections, extracts them as stage inputs, creates corresponding arguments, and replaces the connections in the expression body with those arguments - matching the pattern from ReadTableToStage in vector_index.cpp.

Copilot finished work on behalf of kunga October 23, 2025 10:37
@kunga
Copy link
Member

kunga commented Oct 23, 2025

@copilot now everything works, but could you simplify deleteKeysConnection / deleteKeysPrecompute calls?

Co-authored-by: kunga <6822967+kunga@users.noreply.github.com>
Copy link
Contributor Author

Copilot AI commented Oct 23, 2025

@copilot now everything works, but could you simplify deleteKeysConnection / deleteKeysPrecompute calls?

Simplified in 498404f. Removed the intermediate deleteKeysConnection variable and combined the Build calls into a single nested expression using .Connection<TDqCnUnionAll>() template syntax.

Copilot finished work on behalf of kunga October 23, 2025 11:02
effects.emplace_back(std::move(indexDelete));
}

auto indexDelete = Build<TKqlDeleteRows>(ctx, del.Pos())
Copy link
Member

Choose a reason for hiding this comment

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

вытащил из ифов, вроде как было задублировано

}
}

TDqStageBase ReadTableToStage(const TExprBase& expr, TExprContext& ctx) {
Copy link
Member

Choose a reason for hiding this comment

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

вынес, без изменений

@kunga kunga added the ok-to-test Special label used to approve a PR for testing on our infrastructure label Oct 23, 2025
@github-actions github-actions bot removed the ok-to-test Special label used to approve a PR for testing on our infrastructure label Oct 23, 2025
@github-actions
Copy link

github-actions bot commented Oct 23, 2025

2025-10-23 11:41:16 UTC Pre-commit check linux-x86_64-relwithdebinfo for 004e0fa has started.
2025-10-23 11:41:35 UTC Artifacts will be uploaded here
2025-10-23 11:45:54 UTC ya make is running...
🟡 2025-10-23 13:13:30 UTC Some tests failed, follow the links below. Going to retry failed tests...

Ya make output | Test bloat

TESTS PASSED ERRORS FAILED SKIPPED MUTED?
38651 35846 0 5 2758 42

2025-10-23 13:13:39 UTC ya make is running... (failed tests rerun, try 2)
🟡 2025-10-23 13:31:55 UTC Some tests failed, follow the links below. Going to retry failed tests...

Ya make output | Test bloat | Test bloat

TESTS PASSED ERRORS FAILED SKIPPED MUTED?
623 (only retried tests) 597 0 1 0 25

2025-10-23 13:32:00 UTC ya make is running... (failed tests rerun, try 3)
🟢 2025-10-23 13:48:29 UTC Tests successful.

Ya make output | Test bloat | Test bloat | Test bloat

TESTS PASSED ERRORS FAILED SKIPPED MUTED?
442 (only retried tests) 423 0 0 0 19

🟢 2025-10-23 13:48:32 UTC Build successful.
🟢 2025-10-23 13:48:51 UTC ydbd size 2.3 GiB changed* by +18.0 KiB, which is < 100.0 KiB vs main: OK

ydbd size dash main: 74cf4c1 merge: 004e0fa diff diff %
ydbd size 2 436 392 128 Bytes 2 436 410 560 Bytes +18.0 KiB +0.001%
ydbd stripped size 517 948 936 Bytes 517 949 256 Bytes +320 Bytes +0.000%

*please be aware that the difference is based on comparing your commit and the last completed build from the post-commit, check comparation

@github-actions
Copy link

github-actions bot commented Oct 23, 2025

2025-10-23 11:42:09 UTC Pre-commit check linux-x86_64-release-asan for 004e0fa has started.
2025-10-23 11:42:36 UTC Artifacts will be uploaded here
2025-10-23 11:46:56 UTC ya make is running...
🟡 2025-10-23 13:50:47 UTC Some tests failed, follow the links below. This fail is not in blocking policy yet

Ya make output | Test bloat

TESTS PASSED ERRORS FAILED SKIPPED MUTED?
15687 15228 0 187 255 17

🟢 2025-10-23 13:50:53 UTC Build successful.
🟢 2025-10-23 13:51:16 UTC ydbd size 3.8 GiB changed* by +57.9 KiB, which is < 100.0 KiB vs main: OK

ydbd size dash main: 74cf4c1 merge: 004e0fa diff diff %
ydbd size 4 053 820 208 Bytes 4 053 879 512 Bytes +57.9 KiB +0.001%
ydbd stripped size 1 505 358 464 Bytes 1 505 367 168 Bytes +8.5 KiB +0.001%

*please be aware that the difference is based on comparing your commit and the last completed build from the post-commit, check comparation

@kunga kunga mentioned this pull request Oct 23, 2025
13 tasks
@kunga kunga marked this pull request as ready for review October 23, 2025 13:52
@kunga kunga requested a review from a team as a code owner October 23, 2025 13:52
@kunga kunga requested review from gridnevvvit and vitalif October 23, 2025 13:52
@kunga kunga merged commit a6a9fcf into main Oct 24, 2025
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants