Skip to content
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.

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

CNDB-9422 vsearch: Add CQL function to get the tokens produced by a Lucene analyzer #1115

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

adelapena
Copy link

The function replaces the virtual table system_views.analyzer, as described in CNDB-9422.

@adelapena
Copy link
Author

adelapena commented May 14, 2024

I wonder if the function should be named simply "analyze", or we should mention Lucene in the name, since there can be other analyzers. For example, I think SASI analyzer doesn't use Lucene.

@jkni jkni self-requested a review June 21, 2024 14:26
@jkni
Copy link

jkni commented Jun 21, 2024

I wonder if the function should be named simply "analyze", or we should mention Lucene in the name, since there can be other analyzers. For example, I think SASI analyzer doesn't use Lucene.

This is a good point. I think I'd prefer calling it sai_analyze if we want to distinguish that this is the SAI analysis path. While we use Lucene for analysis at the moment, I think that's more of an implementation detail, and I'd rather not couple function naming to that.

@jkni
Copy link

jkni commented Jun 21, 2024

LGTM from an implementation perspective. Should we add in-tree doc coverage for this?

@adelapena
Copy link
Author

Thanks for the review. I have renamed the function to sai_analyze and added some doc. Documentation for SAI is something that we should definitively add at some point; I haven't found anything about it in the in-tree doc.

Copy link

sonarcloud bot commented Jun 24, 2024

Copy link

@jkni jkni left a comment

Choose a reason for hiding this comment

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

LGTM! Test failure is benign and related to test quality, not this PR. Thanks for the PR. Agreed on SAI coverage in-tree.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants