Skip to content

Handle BigInteger in xcontent copy#111937

Merged
elasticsearchmachine merged 12 commits intoelastic:mainfrom
rjernst:xcontent/numeric_copy
Aug 21, 2024
Merged

Handle BigInteger in xcontent copy#111937
elasticsearchmachine merged 12 commits intoelastic:mainfrom
rjernst:xcontent/numeric_copy

Conversation

@rjernst
Copy link
Member

@rjernst rjernst commented Aug 15, 2024

When xcontent is copied, the parse tree is walked and each element is passed to the given generator. In the case of numbers, BigInteger is currently not handled. Although arbitrary precision BigIntegers are not supported in Elasticsearch, they appear in xcontent when using unsigned long fields. This commit adds handling for that case, and also ensures all token types are handled. Note that BigDecimal are not supported at all since double is the largest floating point mapper supported.

closes #111812

When xcontent is copied, the parse tree is walked and each element is
passed to the given generator. In the case of numbers, BigInteger and
BigDecimal are not handled. This commit adds handling for those cases,
and also ensures all token types are handled.
@rjernst rjernst added >bug :Core/Infra/Core Core issues without another label labels Aug 15, 2024
@rjernst rjernst requested a review from a team as a code owner August 15, 2024 19:16
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra (Team:Core/Infra)

@elasticsearchmachine elasticsearchmachine added v8.16.0 Team:Core/Infra Meta label for core/infra team labels Aug 15, 2024
@elasticsearchmachine
Copy link
Collaborator

Hi @rjernst, I've created a changelog YAML for you.

@rjernst rjernst requested a review from a team as a code owner August 16, 2024 17:56
@rjernst rjernst changed the title Handle BigInteger and BigDecimal in xcontent copy Handle BigIntegerl in xcontent copy Aug 20, 2024
@rjernst rjernst changed the title Handle BigIntegerl in xcontent copy Handle BigInteger in xcontent copy Aug 20, 2024
@rjernst
Copy link
Member Author

rjernst commented Aug 20, 2024

I've adjusted this PR to only add support for BigInteger, no longer BigDecimal. The only reason BigInteger is handled at all is that Elasticsearch has unsigned long fields which are not representable in Java's signed long type.

Copy link
Contributor

@ldematte ldematte left a comment

Choose a reason for hiding this comment

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

One little thing to fix, but LGTM

@@ -0,0 +1,5 @@
pr: 111937
summary: Handle `BigInteger` and `BigDecimal` in xcontent copy
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this needs to be adjusted too

@rjernst rjernst added the auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) label Aug 21, 2024
@elasticsearchmachine elasticsearchmachine merged commit ba87a48 into elastic:main Aug 21, 2024
@rjernst rjernst deleted the xcontent/numeric_copy branch August 21, 2024 14:12
cbuescher pushed a commit to cbuescher/elasticsearch that referenced this pull request Sep 4, 2024
When xcontent is copied, the parse tree is walked and each element is
passed to the given generator. In the case of numbers, BigInteger is
currently not handled. Although arbitrary precision BigIntegers are not
supported in Elasticsearch, they appear in xcontent when using unsigned
long fields. This commit adds handling for that case, and also ensures
all token types are handled. Note that BigDecimal are not supported at
all since double is the largest floating point mapper supported.

closes elastic#111812
davidkyle pushed a commit to davidkyle/elasticsearch that referenced this pull request Sep 5, 2024
When xcontent is copied, the parse tree is walked and each element is
passed to the given generator. In the case of numbers, BigInteger is
currently not handled. Although arbitrary precision BigIntegers are not
supported in Elasticsearch, they appear in xcontent when using unsigned
long fields. This commit adds handling for that case, and also ensures
all token types are handled. Note that BigDecimal are not supported at
all since double is the largest floating point mapper supported.

closes elastic#111812
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) >bug :Core/Infra/Core Core issues without another label Team:Core/Infra Meta label for core/infra team v8.16.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

XContentGenerator#copyCurrentEvent does not handle BigInteger and BigDecimal

4 participants