-
Notifications
You must be signed in to change notification settings - Fork 268
Index merging improvements #1151
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
base: main
Are you sure you want to change the base?
Conversation
adeelsohailahmed
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @mikelei8291, thanks for spotting and fixing this bug. Your implementation looks sound to me. Could you please add a test for this case too?
staticxterm
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @mikelei8291, thank you for the PR.
As Adeel mentioned, please fix any failing tests and write some new ones with example models and indexes so that we have this behavior (what was previously a bug) covered with tests.
|
This PR is stale because it has been open 45 days with no activity. |
|
This PR is stale because it has been open 45 days with no activity. |
|
I am still working on this. It's tricky. Sometimes we want to merge the indexes, sometimes not. |
26cc53c to
21d0e6f
Compare
21d0e6f to
1746baa
Compare
1746baa to
2541da4
Compare
|
This PR is stale because it has been open 45 days with no activity. |
|
This PR is stale because it has been open 45 days with no activity. |
I was trying to create two indexes on the same field for my model:
However, the current implementation will overwrite the first index definition with the second one because they have the same dict key if the key is
index.fields. I changed the key toindex.nameso by providing a different name than the default one, the indexes will no longer overwrite each others.Also I was trying to make it work with
Annotatedtoo:but it would change the return type of
get_index_attributes()so I didn't go further.