Skip to content
This repository has been archived by the owner on Apr 4, 2023. It is now read-only.

Remove limit of 1000 position per attribute #368

Merged
merged 2 commits into from
Oct 12, 2021

Conversation

ManyTheFish
Copy link
Member

@ManyTheFish ManyTheFish commented Sep 22, 2021

Instead of using an arbitrary limit we encode the absolute position in a u32
using one strong u16 for the field id and a weak u16 for the relative position in the attribute.

  • check database size difference

below is the database size difference for each dataset:
Capture d’écran 2021-09-27 à 18 01 44

  • check search time on big dataset

Related to product#202

@ManyTheFish ManyTheFish force-pushed the remove-max-position-per-attribute-limit branch 4 times, most recently from c9719b8 to 131c0c9 Compare September 22, 2021 16:13
@ManyTheFish ManyTheFish force-pushed the remove-max-position-per-attribute-limit branch from 131c0c9 to 6506d63 Compare October 6, 2021 09:17
@ManyTheFish ManyTheFish marked this pull request as ready for review October 6, 2021 10:12
irevoire
irevoire previously approved these changes Oct 11, 2021
Copy link
Member

@irevoire irevoire left a comment

Choose a reason for hiding this comment

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

Looks good to me, I think we can merge


// Compute the absolute word position with the field id of the attribute and relative position in the attribute.
pub fn absolute_from_relative_position(field_id: FieldId, relative: RelativePosition) -> Position {
(field_id as u32) << 16 | (relative as u32)
Copy link
Member

Choose a reason for hiding this comment

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

👍 I always mess up my casts

Copy link
Member Author

Choose a reason for hiding this comment

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

Me too! That's why I made tests!

milli/src/update/index_documents/mod.rs Show resolved Hide resolved
irevoire
irevoire previously approved these changes Oct 11, 2021
@ManyTheFish
Copy link
Member Author

bors merge

@bors
Copy link
Contributor

bors bot commented Oct 12, 2021

Merge conflict.

Instead of using an arbitrary limit we encode the absolute position in a u32
using one strong u16 for the field id and a weak u16 for the relative position in the attribute.
Copy link
Member

@irevoire irevoire left a comment

Choose a reason for hiding this comment

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

bors merge

@bors bors bot merged commit 3f7f24b into main Oct 12, 2021
@bors bors bot deleted the remove-max-position-per-attribute-limit branch October 12, 2021 08:55
@bors
Copy link
Contributor

bors bot commented Oct 12, 2021

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants