Skip to content

Refactor text index code a bit#89088

Merged
Ergus merged 5 commits intoClickHouse:masterfrom
Ergus:invert_index_text_refactor
Oct 28, 2025
Merged

Refactor text index code a bit#89088
Ergus merged 5 commits intoClickHouse:masterfrom
Ergus:invert_index_text_refactor

Conversation

@Ergus
Copy link
Copy Markdown
Member

@Ergus Ergus commented Oct 27, 2025

Changelog category (leave one):

  • Not for changelog (changelog entry is not required)

Details

Refactors the TextIndex code to make it more localized. This is a preparation for another PR.

Changes made to src/Storages/IndicesDescription.{h,cpp} and src/Storages/MergeTree/MergeTreeIndexText.{h,cpp}:

  • parseTextIndexArgumentFromAST moved to MergeTreeIndexText::parseArgumentsListFromAST
  • parseTextIndexArgumentsFromAST removed
  • getIndexFromAST now uses the new function MergeTreeIndexText::parseArgumentsListFromAST
  • initExpressionInfo: New function to group needed actions to initialize expression
  • parsePositionalArgumentsFromAST: New function to encapsulate generic arguments parsing
  • parseNamedArgumentFromAST and parseArgumentsListFromAST: New functions

* src/Storages/IndicesDescription.{h,cpp} (parseTextIndexArgumentFromAST -> MergeTreeIndexText::parseArgumentsListFromAST) :
(parseTextIndexArgumentsFromAST) : Removed
(getIndexFromAST) : Use the new function MergeTreeIndexText::parseArgumentsListFromAST
(initExpressionInfo) : New function to "group" needed actions to initialize expression.
(parsePositionalArgumentsFromAST) : New function to encapsulate generic arguments parsing
* src/Storages/MergeTree/MergeTreeIndexText.{h,cpp} (parseNamedArgumentFromAST) :
(parseArgumentsListFromAST) : New functions
@Ergus Ergus requested review from ahmadov and rschu1ze October 27, 2025 18:15
@clickhouse-gh
Copy link
Copy Markdown
Contributor

clickhouse-gh Bot commented Oct 27, 2025

Workflow [PR], commit [65bd165]

Summary:

job_name test_name status info comment
Integration tests (arm_binary, distributed plan, 4/4) failure
test_storage_rabbitmq/test.py::test_block_based_formats_1 FAIL cidb

@clickhouse-gh clickhouse-gh Bot added the pr-not-for-changelog This PR should not be mentioned in the changelog label Oct 27, 2025
@Ergus Ergus changed the title Refactor TextIndex code to be more localized Refactor text index code a bit Oct 27, 2025
@Ergus Ergus enabled auto-merge October 27, 2025 18:20
@rschu1ze

This comment was marked as resolved.

@Ergus

This comment was marked as resolved.

{

Tuple MergeTreeIndexText::parseNamedArgumentFromAST(const ASTFunction * ast_equal_function)
Tuple parseNamedArgumentFromAST(const ASTFunction * ast_equal_function)
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

We could argue forever about these two changes ;)

Copy link
Copy Markdown
Member

@rschu1ze rschu1ze left a comment

Choose a reason for hiding this comment

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

Alright, let's get this in then.

@Ergus
Copy link
Copy Markdown
Member Author

Ergus commented Oct 27, 2025

@rschu1ze I added this comment about the change.

I am still open to implement as you mentioned, but still don't think it is better than now.

@Ergus
Copy link
Copy Markdown
Member Author

Ergus commented Oct 28, 2025

Failing test already reported as flaky: #71049

@Ergus Ergus added this pull request to the merge queue Oct 28, 2025
Merged via the queue into ClickHouse:master with commit 589c9ed Oct 28, 2025
122 of 124 checks passed
@Ergus Ergus deleted the invert_index_text_refactor branch October 28, 2025 10:33
@robot-ch-test-poll4 robot-ch-test-poll4 added the pr-synced-to-cloud The PR is synced to the cloud repo label Oct 28, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pr-not-for-changelog This PR should not be mentioned in the changelog pr-synced-to-cloud The PR is synced to the cloud repo

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants