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

feat(search): Added Alias Commands to Search #2577

Open
wants to merge 1 commit into
base: unstable
Choose a base branch
from

Conversation

jonathanc-n
Copy link
Contributor

@jonathanc-n jonathanc-n commented Oct 4, 2024

references #2555

Added alias commands to search with unit tests in Go. (FT.ALIASADD, FT.ALIASDEL, FT.ALIASUPDATE)

Just wanted to see if this seemed fine before adding it to the actual alias functionality of the other search commands. Will create the alias functionality immediately if this PR gets merged.

Alias Subkey
ns | type | index | alias_name => index_name

@jonathanc-n
Copy link
Contributor Author

Is somebody able to resolve the conflict, its just the previous command is just in the same place. When I try to retrieve from origin the previous commits aren't showing up right now, sorry about that

Copy link
Member

@PragmaTwice PragmaTwice left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution.

However FIELD_ALIAS is not for this purpose. It's for alias of index fields.

Also we should see the encoding design in your proposal, but I think now it's not consistent with the implementation.

@jonathanc-n
Copy link
Contributor Author

@PragmaTwice I decided to switch it up the serialization on the implementation, I can include it on the pr. As for the field_alias, you can use this implementation to use index fields to look and see if the alias matches when it receives a non matching index in a command (which will be implemented in a GetAlias function).

@PragmaTwice
Copy link
Member

PragmaTwice commented Oct 5, 2024

FT.ALIAS* is for alias of indexes.

FIELD_ALIAS is for alias of fields.

Indexes is not fields, and one index can includes several fields.

Your implementation is quite confusing, if you really want to add alias of indexes, the encoding should be something like

ns | INDEX_ALIAS | alias => index

If you put index name before alias, how do you quickly map from alias to the index name before you know the original index?

I'd suggest that figure out what you want to do and how to do before writing some random code.

@jonathanc-n
Copy link
Contributor Author

jonathanc-n commented Oct 5, 2024

@PragmaTwice Sorry about that, I didn't know there was aliases for the fields. I wanted to make use of the SearchKey for search fields, and saw that it had an index field and thought I could include it for loading up index information. When implementing the add/delete/update commands I would fetch with the index field in the search key being empty. Would this make it slower?

Sorry again about the ignorance, I just started learning computer science in my last year of highschool and was excited about databases. After a couple months I tried to learn about Kvrocks and make contributions since I was pretty excited about the database itself. I will try harder to make better contributions in the future. Do you have any tips on improvement?

@PragmaTwice
Copy link
Member

Besides ADD, UPDATE and DELETE operations of alias, the most important part of aliasing is to find the corresponding index by the user-provided alias in query commands (e.g. FT.SEARCH, FT.SEARCHSQL).

So we need to consider how to efficiently identify the alias and find the corresponding index in the query (in a very little runtime overhead). e.g. for index a, b and their aliases (c -> index a, d -> index b), we can put a key like this:

// alias
ns | INDEX_ALIAS | c -> a
ns | INDEX_ALIAS | d ->b

// index metadata (already exists)
ns | INDEX_META | a -> ...
ns | INDEX_META | b -> ...

And when we saw a query like select * from <idx>, we need to check if <idx> is a real index name or it's an alias, e.g.

  • select * from a: a is a real index name.
  • select * from d: d is an alias and the real index is b.
  • select * from e: e doesn't exist.

For index names and its metadata, we will load all of them into the memory from start instead of getting it from rocksdb every time we need it to achieve better performance.

So I think for alias we should also load from start.

In your design:

Alias Subkey
ns | type | index | alias_name => index_name

The index name seems repeated in both keys and values, which is not so useful.

@jonathanc-n
Copy link
Contributor Author

jonathanc-n commented Oct 5, 2024

@PragmaTwice While the load commands happen do you think it'd be better to create a global alias map in index_manager?
There would be two places for aliases to be stored:

  1. A global alias map where it is 'alias -> index' for quick alias to index lookups.
  2. A alias array in IndexInfo for deleting all aliases that are associated with the index quickly if ft.dropindex is called.

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.

2 participants