Skip to content

Conversation

@Aksha1812
Copy link
Collaborator

Proposal to update nomenclature : Classes inheriting IndexBase handle a single field/attribute. This is not apparent from the name of the classes inheriting IndexBase. e.g Tag, Text etc.. . proposing to add 'Field' suffix to these classes.

 IndexBase
├── Tag
├── Text  
├── Numeric
└── VectorBase
    ├── VectorFlat
    └── VectorHNSW

changes to

IndexBase
├── TagField
├── TextField  
├── NumericField
└── VectorBaseField
    ├── VectorFlatField
    └── VectorHNSWField

I, Aksha Thakkar <[email protected]>, hereby add my Signed-off-by to this commit: 7c412ba

Signed-off-by: Aksha Thakkar <[email protected]>
@yairgott
Copy link
Collaborator

A couple of points:

  1. The terminology that we're using is Attribute rather than Field to generalize both JSON and Hash. I believe that we're using this terminology in our public documentations (if not, we should).
  2. I think that the it makes sense to also include the following changes:
    2.1. IndexBase -> AttributeBase , note that it's already using the namespace valkey_search::indexes
    2.2. IndexerType , kIndexerTypeByStr, etc -> AttributeType, ...

WDYT?

@allenss-amazon
Copy link
Member

Well, I'm 100% in favor of consistency. The Redis documents also use the term Attribute in many places. While I personally find field more usable than attribute, IDC which we end up with as long as it's consistently applied.

@Aksha1812
Copy link
Collaborator Author

Aksha1812 commented Aug 20, 2025

  1. I believe 'Field' is used more in this context as i have seen in Valkey code https://github.com/search?q=repo%3Avalkey-io%2Fvalkey+attribute&type=code vs https://github.com/search?q=repo%3Avalkey-io%2Fvalkey+field&type=code. I didn't find Attribute being used in the documentation as well. But in valkey-search I understand that we have been using 'Attribute' in this context. lmk if i am wrong. Keeping that in mind are you saying it should rather be called 'TagAttribute' or 'NumericAttribute' rather than using 'TagField' or 'NumericField' to be in line with terminology currently being used.

  2. Agreed will add these changes as well.

@yairgott
Copy link
Collaborator

Keeping that in mind are you saying it should rather be called 'TagAttribute' or 'NumericAttribute' rather than using 'TagField' or 'NumericField' to be in line with terminology currently being used.

Correct.

  1. Agreed will add these changes as well.

SG!

@allenss-amazon
Copy link
Member

Did we abandon this? If so, let's close it

@Aksha1812
Copy link
Collaborator Author

Did we abandon this? If so, let's close it

No haven't abandoned this, just caught up with some fulltext work currently, will update pr soon, once have some extra time

@allenss-amazon
Copy link
Member

Update or close please.

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.

3 participants