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

Indexing constants simplify #15090

Open
giannik opened this issue Jan 14, 2024 · 3 comments
Open

Indexing constants simplify #15090

giannik opened this issue Jan 14, 2024 · 3 comments
Milestone

Comments

@giannik
Copy link
Contributor

giannik commented Jan 14, 2024

Currently the indexing constants when using elastic search use the dot notatation in the name :

image

When creating an index in elasticsearch the mappings are structured in a hierarchy based on the dot notation and not as a flat property as expected.

image

this makes it difficult to read and follow .

I propose to remove the dot notation and use an underscore and make the constant names more shorter as so :

image

@Skrypt @MikeAlhayek any reason not to do this ?
Before i submit a PR.

@Skrypt
Copy link
Contributor

Skrypt commented Jan 14, 2024

Some of these IndexingConstants are based on what ElasticSearch does by default. Changing this means changing the default naming convention of ElasticSearch.

As for the ContentItem it is a question of taste. I prefer how they actually are because they reflect how the shape is built. In the end what you are doing is removing the "Content" part to shorten the name here. If you are changing these, you are also breaking everyone's integrations.

I would strongly advise to not change these.

@giannik
Copy link
Contributor Author

giannik commented Jan 14, 2024

Some of these IndexingConstants are based on what ElasticSearch does by default. Changing this means changing the default naming convention of ElasticSearch.

By using the dot notation we are inferring that these are nested properties (sub fields) of a composite object when in fact its just a flat field.
Also when running queries we view them as fields.

Just think it would be cleaner to treat the mappings as flat objects

https://www.elastic.co/guide/en/elasticsearch/reference/8.11/properties.html

@sebastienros
Copy link
Member

Option 1:
"this makes it difficult to read and follow"

Option 2:
We break everyone's index, queries and settings even for those who don't use elasticsearch

Easy to pick one. Can you think of a solution that would work for everyone? I know @MikeAlhayek was replacing these dots for Azure AI Search.

@sebastienros sebastienros added this to the backlog milestone Feb 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants