Skip to content

Conversation

@iverase
Copy link
Contributor

@iverase iverase commented Feb 21, 2022

We currently compute the cost of the dense iterator using the following code:

final long cost = Math.round(counter / numValuesPerDoc);

Where counter is how many values have been added to the builder. This is inconsistent with the #grow method where the counter is increased as it expects grow to be called for documents and no values. Therefore in this PR is proposed to change the way we compute the cost to reflect that counter refers to documents and not to values:

final long cost = Math.min(counter, docCount)

@jpountz
Copy link
Contributor

jpountz commented Feb 22, 2022

This is inconsistent with the #grow method where the counter is increased as it expects grow to be called for documents and no values.

Actually my expectation is that grow() is called with a number of values, not unique documents. Javadocs say "documents" today, which might be a source of confusion, but it is really an upper bound of the number of times BulkAdder#add may be called, ie. an upper bound of the number of matching values?

@iverase
Copy link
Contributor Author

iverase commented Feb 22, 2022

Actually my expectation is that grow() is called with a number of values, not unique documents.

Then it is wrong that accepts an int and should accept a long? which is what Robert complains about

@iverase
Copy link
Contributor Author

iverase commented Mar 16, 2022

won't happen

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