Skip to content

Conversation

Copy link
Contributor

Copilot AI commented Oct 24, 2025

  • Explore codebase to understand structure
  • Analyze delete operations for fulltext indexes (uses BuildFulltextIndexRows with includeDataColumns=false)
  • Analyze insert operations for fulltext indexes (uses BuildFulltextIndexRows with includeDataColumns=true)
  • Analyze upsert operations for vector indexes (reference implementation)
  • Implement fulltext index handling in upsert operation
  • Add fulltext index delete logic in upsert (when key columns are updated)
  • Add fulltext index insert logic in upsert (for new/updated rows)
  • Review code for correctness
  • Fix inputColumns to match actual columns in input rows
  • Final verification of implementation
  • Implement UpsertRow, UpsertRowMultipleTimes, UpsertRowReturning tests
  • Implement UpsertRowCovered, UpsertRowCoveredMultipleTimes, UpsertRowCoveredReturning tests
  • Implement UpdateRow and UpdateRowCovered tests
  • Enable CreateTable and CreateTableCovered tests
  • Add UpdateRowMultipleTimes, UpdateRowReturning, and UpdateRowCoveredReturning tests
  • Add UPDATE ON statement checks to all Update tests
  • Add cases when Upsert modifies existing rows in all UpsertRow tests
  • Add comprehensive REPLACE INTO tests (ReplaceRow, ReplaceRowMultipleTimes, ReplaceRowReturning, ReplaceRowCovered, ReplaceRowCoveredMultipleTimes, ReplaceRowCoveredReturning)
  • Fix type mismatch error by adding TDqPhyPrecompute stage to match delete operation pattern

The fix wraps deleteIndexKeys in a precompute stage before passing to BuildFulltextIndexRows, matching the pattern in delete operation.

Original prompt

Add upsert rows functionality for fulltext indexed tables. It should handle new rows and modifications. See delete and insert operations as a reference. Also see how vector index is handled in upserts. Do not build or run code, but check it carefully.


💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.

Copilot AI changed the title [WIP] Add upsert rows functionality for fulltext indexed tables Add upsert rows functionality for fulltext indexed tables Oct 24, 2025
Copilot AI requested a review from kunga October 24, 2025 13:18
Copilot finished work on behalf of kunga October 24, 2025 13:18
@kunga
Copy link
Member

kunga commented Oct 24, 2025

@copilot finish UpsertRow, UpsertRowCovered, UpdateRow, UpdateRowCovered tests in kqp_indexes_fulltext_ut.cpp

UpsertRow, UpsertRowCovered should follow Insert tests (also add MultipleTimes and Returning cases)

UpdateRow, UpdateRowCovered should use UpsertSomeTexts for filling also

Copy link
Contributor Author

Copilot AI commented Oct 24, 2025

@copilot finish UpsertRow, UpsertRowCovered, UpdateRow, UpdateRowCovered tests in kqp_indexes_fulltext_ut.cpp

UpsertRow, UpsertRowCovered should follow Insert tests (also add MultipleTimes and Retu...

Implemented all requested tests in commit 9ef194d:

UpsertRow tests (following Insert pattern):

  • UpsertRow - basic upsert test
  • UpsertRowMultipleTimes - multiple upserts in sequence
  • UpsertRowReturning - upsert with RETURNING clause

UpsertRowCovered tests (following InsertCovered pattern):

  • UpsertRowCovered - basic upsert with covered index
  • UpsertRowCoveredMultipleTimes - multiple upserts with covered index
  • UpsertRowCoveredReturning - upsert with RETURNING and covered index

UpdateRow tests (using UpsertSomeTexts for initial data):

  • UpdateRow - tests updating indexed Text column and non-indexed Data column
  • UpdateRowCovered - tests updating indexed Text column and covered Data column

Also enabled CreateTable and CreateTableCovered tests by removing the early return statements.

Copilot finished work on behalf of kunga October 24, 2025 13:45
@kunga
Copy link
Member

kunga commented Oct 24, 2025

@copilot add UpdateRowMultipleTimes, UpdateRowReturning, UpdateRowCoveredReturning tests

Copy link
Contributor Author

Copilot AI commented Oct 24, 2025

@copilot add UpdateRowMultipleTimes, UpdateRowReturning, UpdateRowCoveredReturning tests

Added all three tests in commit faf82bb:

UpdateRowMultipleTimes - tests updating multiple rows simultaneously
UpdateRowReturning - tests UPDATE with RETURNING clause for both indexed Text column and non-indexed Data column
UpdateRowCoveredReturning - tests UPDATE with RETURNING clause for covered index, updating both indexed Text column and covered Data column

All tests follow the pattern from DeleteRow tests and use UpsertSomeTexts for initial data setup.

Copilot finished work on behalf of kunga October 24, 2025 14:30
@kunga
Copy link
Member

kunga commented Oct 24, 2025

@copilot add update on statement checks like it's done in Delete tests

Copy link
Contributor Author

Copilot AI commented Oct 24, 2025

@copilot add update on statement checks like it's done in Delete tests

Added UPDATE ON statement checks in commit 9469521:

  • UpdateRow - added UPDATE ON SELECT for updating Text and Data columns
  • UpdateRowCovered - added UPDATE ON SELECT for covered index
  • UpdateRowReturning - added UPDATE ON SELECT with RETURNING clause
  • UpdateRowCoveredReturning - added UPDATE ON SELECT with RETURNING clause for covered index

All tests now follow the same pattern as Delete tests with three types of operations: WHERE by PK, WHERE by filter, and ON SELECT.

Copilot finished work on behalf of kunga October 24, 2025 14:38
@kunga
Copy link
Member

kunga commented Oct 24, 2025

@copilot add cases when Upsert modify existing row in UpsertRow tests

Copy link
Contributor Author

Copilot AI commented Oct 24, 2025

@copilot add cases when Upsert modify existing row in UpsertRow tests

Added modify existing row cases in commit a121c75:

  • UpsertRow - added test for modifying existing row with Key=100
  • UpsertRowMultipleTimes - added test for modifying existing row with Key=100 after inserting new rows
  • UpsertRowReturning - added test for modifying existing row with Key=200 with RETURNING clause
  • UpsertRowCovered - added test for modifying existing row with Key=100 for covered index
  • UpsertRowCoveredMultipleTimes - added test for modifying existing row with Key=200 for covered index
  • UpsertRowCoveredReturning - added test for modifying existing row with Key=200 for covered index with RETURNING clause

All UpsertRow tests now test both insert new row and modify existing row scenarios, verifying that the fulltext index is correctly maintained when rows are updated via UPSERT.

Copilot finished work on behalf of kunga October 24, 2025 14:50
@kunga
Copy link
Member

kunga commented Oct 24, 2025

@copilot add also bunch of tests on 'REPLACE INTO' statement

@kunga
Copy link
Member

kunga commented Oct 24, 2025

@copilot :2:25: Error: At tuple, At tuple, At tuple, At function: KqlDeleteRows, At function: DqCnUnionAll, At function: TDqOutput, At function: DqStage, At lambda, At function: Iterator, At function: Iterator, At function: Iterator
:2:25: Error: Expected (optional) list, but got: Flow<Struct<'Key':Uint64?,'__ydb_token':String>>

Copy link
Contributor Author

Copilot AI commented Oct 24, 2025

@copilot :2:25: Error: At tuple, At tuple, At tuple, At function: KqlDeleteRows, At function: DqCnUnionAll, At function: TDqOutput, At function: DqStage, At lambda, At function: Iterator, At function:...

Fixed in commit 526c2ed. The error was caused by a missing TDqPhyPrecompute stage wrapper around deleteIndexKeys before passing it to BuildFulltextIndexRows. The fulltext index requires the input to be wrapped in a precompute stage (similar to the delete operation in kqp_opt_phy_delete_index.cpp) because BuildFulltextIndexRows returns a Flow type that needs to be materialized before being used in the delete operation.

Copilot finished work on behalf of kunga October 24, 2025 18:10
@kunga kunga added the ok-to-test Special label used to approve a PR for testing on our infrastructure label Oct 27, 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 27, 2025
@github-actions
Copy link

github-actions bot commented Oct 27, 2025

2025-10-27 13:56:14 UTC Pre-commit check linux-x86_64-release-asan for 1efffb9 has started.
2025-10-27 13:56:29 UTC Artifacts will be uploaded here
2025-10-27 13:57:50 UTC ya make is running...
🟡 2025-10-27 15:57: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?
15291 14841 0 192 238 20

🟢 2025-10-27 15:57:52 UTC Build successful.
🟢 2025-10-27 15:58:16 UTC ydbd size 3.8 GiB changed* by +57.0 KiB, which is < 100.0 KiB vs main: OK

ydbd size dash main: fcdbfe7 merge: 1efffb9 diff diff %
ydbd size 4 060 520 416 Bytes 4 060 578 792 Bytes +57.0 KiB +0.001%
ydbd stripped size 1 508 593 024 Bytes 1 508 602 048 Bytes +8.8 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

@github-actions
Copy link

github-actions bot commented Oct 27, 2025

2025-10-27 13:57:04 UTC Pre-commit check linux-x86_64-relwithdebinfo for 1efffb9 has started.
2025-10-27 13:57:19 UTC Artifacts will be uploaded here
2025-10-27 13:58:44 UTC ya make is running...
🟡 2025-10-27 15:29:28 UTC Some tests failed, follow the links below. Going to retry failed tests...

Ya make output | Test bloat

TESTS PASSED ERRORS FAILED SKIPPED MUTED?
38559 35735 0 5 2787 32

2025-10-27 15:29:39 UTC ya make is running... (failed tests rerun, try 2)
🟢 2025-10-27 15:46:57 UTC Tests successful.

Ya make output | Test bloat | Test bloat

TESTS PASSED ERRORS FAILED SKIPPED MUTED?
314 (only retried tests) 294 0 0 0 20

🟢 2025-10-27 15:47:00 UTC Build successful.
🟢 2025-10-27 15:47:16 UTC ydbd size 2.3 GiB changed* by +62.6 KiB, which is < 100.0 KiB vs main: OK

ydbd size dash main: bfc5f0e merge: 1efffb9 diff diff %
ydbd size 2 439 891 816 Bytes 2 439 955 888 Bytes +62.6 KiB +0.003%
ydbd stripped size 518 917 000 Bytes 518 917 960 Bytes +960 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

@kunga kunga mentioned this pull request Oct 27, 2025
15 tasks
@kunga kunga marked this pull request as ready for review October 27, 2025 17:22
@kunga kunga requested a review from a team as a code owner October 27, 2025 17:22
@kunga kunga requested a review from vitalif October 27, 2025 17:22
const NYql::TKikimrTablesData& tables, TStringBuf cluster);

TVector<std::pair<NYql::TExprNode::TPtr, const NYql::TIndexDescription*>> BuildSecondaryIndexVector(
TVector<std::pair<NYql::TExprNode::TPtr, const NYql::TIndexDescription*>> BuildEffectedIndexTables(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Может хоть Affected уж тогда? :-)
А всё остальное норм

Copy link
Member

Choose a reason for hiding this comment

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

ok

@kunga kunga added the ok-to-test Special label used to approve a PR for testing on our infrastructure label Oct 28, 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 28, 2025
@github-actions
Copy link

github-actions bot commented Oct 28, 2025

2025-10-28 11:24:39 UTC Pre-commit check linux-x86_64-relwithdebinfo for b4a11b4 has started.
2025-10-28 11:24:44 UTC Artifacts will be uploaded here
2025-10-28 11:26:08 UTC ya make is running...
🟡 2025-10-28 13:59:03 UTC Some tests failed, follow the links below. Going to retry failed tests...

Ya make output | Test bloat

TESTS PASSED ERRORS FAILED SKIPPED MUTED?
38573 35750 0 4 2792 27

2025-10-28 13:59:14 UTC ya make is running... (failed tests rerun, try 2)
🟡 2025-10-28 14:25:36 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?
465 (only retried tests) 441 0 1 1 22

2025-10-28 14:25:38 UTC ya make is running... (failed tests rerun, try 3)
🟢 2025-10-28 14:42:08 UTC Tests successful.

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

TESTS PASSED ERRORS FAILED SKIPPED MUTED?
434 (only retried tests) 415 0 0 1 18

🟢 2025-10-28 14:42:11 UTC Build successful.
🟢 2025-10-28 14:42:30 UTC ydbd size 2.3 GiB changed* by +16.7 KiB, which is < 100.0 KiB vs main: OK

ydbd size dash main: 744e230 merge: b4a11b4 diff diff %
ydbd size 2 427 340 304 Bytes 2 427 357 432 Bytes +16.7 KiB +0.001%
ydbd stripped size 517 566 664 Bytes 517 567 048 Bytes +384 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 28, 2025

2025-10-28 11:24:50 UTC Pre-commit check linux-x86_64-release-asan for b4a11b4 has started.
2025-10-28 11:25:06 UTC Artifacts will be uploaded here
2025-10-28 11:26:29 UTC ya make is running...
🟡 2025-10-28 14:04:24 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?
15298 14758 0 303 217 20

🟢 2025-10-28 14:04:31 UTC Build successful.
🟡 2025-10-28 14:04:55 UTC ydbd size 3.8 GiB changed* by +301.9 KiB, which is >= 100.0 KiB vs main: Warning

ydbd size dash main: bd552ff merge: b4a11b4 diff diff %
ydbd size 4 062 507 112 Bytes 4 062 816 272 Bytes +301.9 KiB +0.008%
ydbd stripped size 1 509 400 312 Bytes 1 509 481 048 Bytes +78.8 KiB +0.005%

*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 merged commit 6bc52c7 into main Oct 28, 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