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

remove phrase field in Document model #148

Merged
merged 1 commit into from
Jun 21, 2022
Merged

Conversation

missinglink
Copy link
Member

@missinglink missinglink commented Apr 19, 2022

remove phrase field in Document in favour of duplicating name during serialisation.

Screenshot 2022-04-19 at 14 11 26

historically we've had two separate fields named name and phrase which correspond to fields containing prefix grams and those which don't, respectively.

the two fields contain exactly the same strings, and I've tried over the years to figure out a clean way of combining them into a single field, a trivial task but one which seems impossible to do without introducing breaking changes 😿

this PR makes a change to the Document model such that there aren't two copies of all the strings being passed around in memory and operated on.

a quick look over the diff in this PR shows that we were doing a bunch of CPU work which was duplicatory and unnecessary, plus we're using up more memory storing all the strings twice 🤷‍♂️ .

@orangejulius could you please sanity check this for me?

@orangejulius
Copy link
Member

Cool, I think this looks good. I have a vague memory that we have occasionally not had the same values in the name and phrase fields, though that might have been unintentional and something we eventually fixed. Do you remember?

In any case, this looks like a good step forward, and hopefully someday we can actually finish off pelias/schema#285

@missinglink
Copy link
Member Author

missinglink commented Apr 19, 2022

I also had a vague memory of discussing it, but it would have been 5+ years back, I had a quick look through the blame and found comments mentioning moving from quattroshapes to whosonfirst, so that's how long ago it was 😆

I don't recall the two fields ever being different, maybe in the schema analysis, but not in the model.

ref: 4cc3676

@orangejulius
Copy link
Member

I think I was thinking of #132, which was indeed a bug. So we should be all good.

@missinglink missinglink force-pushed the phrase-as-alias-for-name branch from d4cfa21 to b848c5a Compare June 21, 2022 12:32
@missinglink missinglink force-pushed the phrase-as-alias-for-name branch from b848c5a to 8f08b7b Compare June 21, 2022 12:37
@missinglink missinglink merged commit 52d4069 into master Jun 21, 2022
@missinglink missinglink deleted the phrase-as-alias-for-name branch June 21, 2022 12:38
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