Skip to content

Conversation

@jpountz
Copy link
Contributor

@jpountz jpountz commented Nov 27, 2018

CompositeBytesReference#slice has two bugs:

  • One that makes it fail if the reference is empty and an empty slice is
    created, this is [CI] CompositeBytesReferenceTests::testSlice failure #35950 and is fixed by special-casing empty-slices.
  • One performance bug that makes it always create a composite slice when
    creating a slice that ends on a boundary, this is fixed by computing limit
    as the index of the sub reference that holds the last element rather than
    the next element after the slice.

Closes #35950

…ets.

CompositeBytesReference#slice has two bugs:
 - One that makes it fail if the reference is empty and an empty slice is
   created, this is elastic#35950 and is fixed by special-casing empty-slices.
 - One performance bug that makes it always create a composite slice when
   creating a slice that ends on a boundary, this is fixed by computing `limit`
   as the index of the sub reference that holds the last element rather than
   the next element after the slice.

Closes elastic#35950
@jpountz jpountz added >bug :Core/Infra/Core Core issues without another label v7.0.0 v6.6.0 labels Nov 27, 2018
@jpountz jpountz requested a review from s1monw November 27, 2018 16:45
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra

FutureObjects.checkFromIndexSize(from, length, this.length);

if (length == 0) {
return new BytesArray(BytesRef.EMPTY_BYTES);
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you use BytesArray.EMPTY here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Even better, thanks for the pointer.

@jpountz jpountz requested a review from Tim-Brooks November 28, 2018 08:39
Copy link
Contributor

@Tim-Brooks Tim-Brooks left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@s1monw s1monw left a comment

Choose a reason for hiding this comment

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

LGTM

@jpountz jpountz merged commit fa3d365 into elastic:master Nov 30, 2018
jpountz added a commit that referenced this pull request Nov 30, 2018
…ets. (#35955)

CompositeBytesReference#slice has two bugs:
 - One that makes it fail if the reference is empty and an empty slice is
   created, this is #35950 and is fixed by special-casing empty-slices.
 - One performance bug that makes it always create a composite slice when
   creating a slice that ends on a boundary, this is fixed by computing `limit`
   as the index of the sub reference that holds the last element rather than
   the next element after the slice.

Closes #35950
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

>bug :Core/Infra/Core Core issues without another label v6.6.0 v7.0.0-beta1

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[CI] CompositeBytesReferenceTests::testSlice failure

5 participants