Skip to content

PrimitiveObjectFormatter eagerly reads integer where double could be preferred. #4649

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

Merged
merged 1 commit into from
Apr 21, 2020

Conversation

Mpdreamz
Copy link
Member

A bug in PrimitiveObjectFormatter which is used when deserializing
object was eagerly reading doubles as integers. This was flagged in
failing yaml test reproduce:

./build.sh rest-spec-tests xpack -f analytics -t t_test.yml -s "heteroscedastic t-test"

Scrollbars now also collapse when running the yaml tests now that
ShellProgressbar properly scrolls children into view this got to be a
tad excessive

A bug in `PrimitiveObjectFormatter` which is used when deserializing
`object` was eagerly reading doubles as integers. This was flagged in
failing yaml test reproduce:

build rest-spec -- xpack -f analytics -t t_test.yml -s "heteroscedastic t-test"

Scrollbars now also collapse when running the yaml tests now that
ShellProgressbar properly scrolls children into view this got to be a
tad excessive
@Mpdreamz
Copy link
Member Author

1 failing test not related:

[xUnit.net 00:14:30.6164584]     Tests.Ingest.PutPipeline.PutPipelineApiTests.ReturnsExpectedIsValid [FAIL]
[2020-04-21T20:07:29,326][INFO ][o.e.x.s.a.r.TransportPutRoleAction] [xpack-node-7bea8e9200] added role [role-nest-initializer-96b8dc0e]
[2020-04-21T20:07:29,356][INFO ][o.e.x.s.a.r.TransportPutRoleAction] [xpack-node-7bea8e9200] added role [role-ois-baae57e0-role]
  X Tests.Ingest.PutPipeline.PutPipelineApiTests.ReturnsExpectedResponse [7s 187ms]
  Error Message:
   Tests.Framework.EndpointTests.ResponseAssertionException : Expected response.Acknowledged to be true, but found False.
Response Under Test:
Invalid NEST response built from a unsuccessful (400) low level call on PUT: /_ingest/pipeline/pipeline-1?pretty=true&error_trace=true
# Audit trail of this API call:
 - [1] BadResponse: Node: https://localhost:9200/ Took: 00:00:01.2789750
# OriginalException: Elasticsearch.Net.ElasticsearchClientException: Request failed to execute. Call: Status code 400 from: PUT /_ingest/pipeline/pipeline-1?pretty=true&error_trace=true. ServerError: Type: parse_exception Reason: "processor [csv] doesn't support one or more provided configuration parameters [empty_value]"
# Request:
{"description":"My test pipeline","processors":[{"append":{"field":"state","value":["Stable","VeryActive"]}},{"csv":{"field":"name","target_fields":["targetField1","targetField2"],"trim":true,"empty_value":"empty"}},{"convert":{"field":"numberOfCommits","target_field":"targetField","type":"string"}},{"date":{"field":"startedOn","formats":["dd/MM/yyyy hh:mm:ss"],"target_field":"timestamp","timezone":"Europe/Amsterdam"}},{"enrich":{"field":"name","policy_name":"enrich_processor_policy","target_field":"target_field"}},{"fail":{"message":"an error message"}},{"foreach":{"field":"tags","processor":{"uppercase":{"field":"_value.name"}}}},{"grok":{"field":"description","pattern_definitions":{"FAVORITE_DOG":"border collie","RGB":"RED|BLUE|GREEN"},"patterns":["my %{FAVORITE_DOG:dog} is colored %{RGB:color}"]}},{"gsub":{"field":"name","pattern":"-","replacement":"_"}},{"join":{"field":"branches","separator":","}},{"lowercase":{"field":"name"}},{"remove":{"field":["suggest"]}},{"rename":{"field":"leadDeveloper","target_field":"projectLead"}},{"set":{"field":"name","value":"foo"}},{"split":{"field":"description","separator":"."}},{"trim":{"field":"name"}},{"uppercase":{"field":"name"}},{"dot_expander":{"field":"field.withDots"}},{"script":{"source":"ctx.numberOfCommits++"}},{"urldecode":{"field":"description","ignore_missing":true}},{"attachment":{"field":"description","ignore_missing":true,"indexed_chars":100000,"properties":["title","author"]}},{"circle":{"field":"description","target_field":"arbitraryShape","ignore_missing":true,"error_distance":10.0,"shape_type":"shape"}},{"bytes":{"field":"description","ignore_missing":true}},{"dissect":{"field":"description","pattern":"%{clientip} %{ident} %{auth} [%{@timestamp}] \"%{verb} %{request} HTTP/%{httpversion}\" %{status} %{size}","ignore_missing":true,"append_separator":" "}},{"drop":{"if":"true"}},{"kv":{"field":"description","field_split":"_","ignore_missing":true,"value_split":" "}},{"kv":{"field":"description","field_split":"_","ignore_missing":true,"strip_brackets":true,"trim_key":"xyz","trim_value":"abc","value_split":" "}},{"set_security_user":{"field":"name"}},{"pipeline":{"name":"x"}}]}
# Response:
{
  "error" : {
    "root_cause" : [
      {
        "type" : "parse_exception",
        "reason" : "processor [csv] doesn't support one or more provided configuration parameters [empty_value]",
        "processor_type" : "csv",
        "stack_trace" : "org.elasticsearch.ElasticsearchParseException: processor [csv] doesn't support one or more provided configuration parameters [empty_value]\r\n\tat .... "
  },
  "status" : 400
}

@Mpdreamz Mpdreamz merged commit 50369b6 into master Apr 21, 2020
@Mpdreamz Mpdreamz deleted the fix/master/yaml-tests branch April 21, 2020 20:21
github-actions bot pushed a commit that referenced this pull request Apr 21, 2020
A bug in `PrimitiveObjectFormatter` which is used when deserializing
`object` was eagerly reading doubles as integers. This was flagged in
failing yaml test reproduce:

build rest-spec -- xpack -f analytics -t t_test.yml -s "heteroscedastic t-test"

Scrollbars now also collapse when running the yaml tests now that
ShellProgressbar properly scrolls children into view this got to be a
tad excessive
github-actions bot pushed a commit that referenced this pull request Apr 21, 2020
A bug in `PrimitiveObjectFormatter` which is used when deserializing
`object` was eagerly reading doubles as integers. This was flagged in
failing yaml test reproduce:

build rest-spec -- xpack -f analytics -t t_test.yml -s "heteroscedastic t-test"

Scrollbars now also collapse when running the yaml tests now that
ShellProgressbar properly scrolls children into view this got to be a
tad excessive
russcam added a commit that referenced this pull request Apr 22, 2020
Relates: #4649

This commit fixes a bug in the IsLong implementation, and prefers
reading long values over double values in PrimitiveObjectFormatter
when the bytes represent a valid long value, to avoid floating
point rounding errors that may happen when parsing a double from
a long value.

The bug in IsLong was that the loop over the array segment bytes was
exiting early when the count of bytes was the same as long.Min/MaxValue
length, and the current loop byte is less than the byte at index in
long.Min/MaxValue. In this scenario, it should not exit early, but
not continue to check following loop bytes against byte at index in
long.Min/MaxValue. Following loop iterations still need to check
whether remaining bytes are numbers.
Mpdreamz pushed a commit that referenced this pull request Apr 22, 2020
Relates: #4649

This commit fixes a bug in the IsLong implementation, and prefers
reading long values over double values in PrimitiveObjectFormatter
when the bytes represent a valid long value, to avoid floating
point rounding errors that may happen when parsing a double from
a long value.

The bug in IsLong was that the loop over the array segment bytes was
exiting early when the count of bytes was the same as long.Min/MaxValue
length, and the current loop byte is less than the byte at index in
long.Min/MaxValue. In this scenario, it should not exit early, but
not continue to check following loop bytes against byte at index in
long.Min/MaxValue. Following loop iterations still need to check
whether remaining bytes are numbers.
github-actions bot pushed a commit that referenced this pull request Apr 22, 2020
Relates: #4649

This commit fixes a bug in the IsLong implementation, and prefers
reading long values over double values in PrimitiveObjectFormatter
when the bytes represent a valid long value, to avoid floating
point rounding errors that may happen when parsing a double from
a long value.

The bug in IsLong was that the loop over the array segment bytes was
exiting early when the count of bytes was the same as long.Min/MaxValue
length, and the current loop byte is less than the byte at index in
long.Min/MaxValue. In this scenario, it should not exit early, but
not continue to check following loop bytes against byte at index in
long.Min/MaxValue. Following loop iterations still need to check
whether remaining bytes are numbers.
github-actions bot pushed a commit that referenced this pull request Apr 22, 2020
Relates: #4649

This commit fixes a bug in the IsLong implementation, and prefers
reading long values over double values in PrimitiveObjectFormatter
when the bytes represent a valid long value, to avoid floating
point rounding errors that may happen when parsing a double from
a long value.

The bug in IsLong was that the loop over the array segment bytes was
exiting early when the count of bytes was the same as long.Min/MaxValue
length, and the current loop byte is less than the byte at index in
long.Min/MaxValue. In this scenario, it should not exit early, but
not continue to check following loop bytes against byte at index in
long.Min/MaxValue. Following loop iterations still need to check
whether remaining bytes are numbers.
Mpdreamz pushed a commit that referenced this pull request Apr 22, 2020
Relates: #4649

This commit fixes a bug in the IsLong implementation, and prefers
reading long values over double values in PrimitiveObjectFormatter
when the bytes represent a valid long value, to avoid floating
point rounding errors that may happen when parsing a double from
a long value.

The bug in IsLong was that the loop over the array segment bytes was
exiting early when the count of bytes was the same as long.Min/MaxValue
length, and the current loop byte is less than the byte at index in
long.Min/MaxValue. In this scenario, it should not exit early, but
not continue to check following loop bytes against byte at index in
long.Min/MaxValue. Following loop iterations still need to check
whether remaining bytes are numbers.

Co-authored-by: Russ Cam <russ.cam@elastic.co>
Mpdreamz pushed a commit that referenced this pull request Apr 22, 2020
Relates: #4649

This commit fixes a bug in the IsLong implementation, and prefers
reading long values over double values in PrimitiveObjectFormatter
when the bytes represent a valid long value, to avoid floating
point rounding errors that may happen when parsing a double from
a long value.

The bug in IsLong was that the loop over the array segment bytes was
exiting early when the count of bytes was the same as long.Min/MaxValue
length, and the current loop byte is less than the byte at index in
long.Min/MaxValue. In this scenario, it should not exit early, but
not continue to check following loop bytes against byte at index in
long.Min/MaxValue. Following loop iterations still need to check
whether remaining bytes are numbers.

Co-authored-by: Russ Cam <russ.cam@elastic.co>
russcam added a commit that referenced this pull request May 7, 2020
Relates: #4649

This commit fixes a bug in the IsLong implementation, and prefers
reading long values over double values in PrimitiveObjectFormatter
when the bytes represent a valid long value, to avoid floating
point rounding errors that may happen when parsing a double from
a long value.

The bug in IsLong was that the loop over the array segment bytes was
exiting early when the count of bytes was the same as long.Min/MaxValue
length, and the current loop byte is less than the byte at index in
long.Min/MaxValue. In this scenario, it should not exit early, but
not continue to check following loop bytes against byte at index in
long.Min/MaxValue. Following loop iterations still need to check
whether remaining bytes are numbers.

(cherry picked from commit f8c6940)
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.

1 participant