You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Yifan Zhang (Code Review)" <ge...@cloudera.org> on 2019/08/21 10:50:10 UTC
[kudu-CR] Add ExtractFloat method to JsonReader and improve ExtractDouble method
Yifan Zhang has uploaded this change for review. ( http://gerrit.cloudera.org:8080/14114
Change subject: Add ExtractFloat method to JsonReader and improve ExtractDouble method
......................................................................
Add ExtractFloat method to JsonReader and improve ExtractDouble method
ExtractDouble method should not only parse double values, int values
that can be losslessly converted to double should also be parsed.
Though there are already IsLosslessDouble()/IsLosslessFloat() methods
in rapidjson library, the implementation seems have problems.
This patch defined new methods to determine whether a value can be
losslessly converted to a double/float value.
Change-Id: Ica0a04fb21daceaa44233995a42a60686f59361b
---
M src/kudu/tools/tool_action_table.cc
M src/kudu/util/jsonreader-test.cc
M src/kudu/util/jsonreader.cc
M src/kudu/util/jsonreader.h
4 files changed, 108 insertions(+), 6 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/14/14114/1
--
To view, visit http://gerrit.cloudera.org:8080/14114
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ica0a04fb21daceaa44233995a42a60686f59361b
Gerrit-Change-Number: 14114
Gerrit-PatchSet: 1
Gerrit-Owner: Yifan Zhang <ch...@163.com>
[kudu-CR] [util] Add ExtractFloat method to JsonReader and improve ExtractDouble method
Posted by "Yifan Zhang (Code Review)" <ge...@cloudera.org>.
Yifan Zhang has posted comments on this change. ( http://gerrit.cloudera.org:8080/14114 )
Change subject: [util] Add ExtractFloat method to JsonReader and improve ExtractDouble method
......................................................................
Patch Set 3:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/14114/2/src/kudu/util/jsonreader.cc
File src/kudu/util/jsonreader.cc:
http://gerrit.cloudera.org:8080/#/c/14114/2/src/kudu/util/jsonreader.cc@248
PS2, Line 248:
> This seems to be one of the major differences from the rapidjson implementa
There are some precision issues in rapidjson implementation.
Some of tests in rapidjson are:
EXPECT_TRUE(Value(12.25).IsLosslessFloat());
EXPECT_FALSE(Value(0.3).IsLosslessFloat());
0.3 is an approximation, for it is not powers of two or integer multiples thereof, there may be precision lost in conversion between double and float.
The comparison 'a >= b && a <=b' seems not reasonable, for we just want to extract a float value, I think 'fabs(a-b) <= 1e-7' is better.
--
To view, visit http://gerrit.cloudera.org:8080/14114
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ica0a04fb21daceaa44233995a42a60686f59361b
Gerrit-Change-Number: 14114
Gerrit-PatchSet: 3
Gerrit-Owner: Yifan Zhang <ch...@163.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yifan Zhang <ch...@163.com>
Gerrit-Comment-Date: Thu, 22 Aug 2019 08:19:49 +0000
Gerrit-HasComments: Yes
[kudu-CR] [util] Add ExtractFloat method to JsonReader and improve ExtractDouble method
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/14114 )
Change subject: [util] Add ExtractFloat method to JsonReader and improve ExtractDouble method
......................................................................
Patch Set 2:
The ASAN failure looks real:
[ RUN ] JsonReaderTest.SignedAndUnsignedInts
/home/jenkins-slave/workspace/kudu-master/2/src/kudu/util/jsonreader.cc:230:38: runtime error: 9.22337e+18 is outside the range of representable values of type 'long'
#0 0x7f6e43eb2c30 in kudu::JsonReader::IsLosslessDoubleValue(rapidjson::GenericValue<rapidjson::UTF8<char>, rapidjson::MemoryPoolAllocator<rapidjson::CrtAllocator> > const*) const /home/jenkins-slave/workspace/kudu-master/2/src/kudu/util/jsonreader.cc:230:38
#1 0x7f6e43eb27a6 in kudu::JsonReader::ExtractDouble(rapidjson::GenericValue<rapidjson::UTF8<char>, rapidjson::MemoryPoolAllocator<rapidjson::CrtAllocator> > const*, char const*, double*) const /home/jenkins-slave/workspace/kudu-master/2/src/kudu/util/jsonreader.cc:148:7
#2 0x551130 in kudu::JsonReaderTest_SignedAndUnsignedInts_Test::TestBody() /home/jenkins-slave/workspace/kudu-master/2/src/kudu/util/jsonreader-test.cc:212:3
--
To view, visit http://gerrit.cloudera.org:8080/14114
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ica0a04fb21daceaa44233995a42a60686f59361b
Gerrit-Change-Number: 14114
Gerrit-PatchSet: 2
Gerrit-Owner: Yifan Zhang <ch...@163.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Wed, 21 Aug 2019 18:02:20 +0000
Gerrit-HasComments: No
[kudu-CR] [util] Add ExtractFloat method to JsonReader and improve ExtractDouble method
Posted by "Yifan Zhang (Code Review)" <ge...@cloudera.org>.
Yifan Zhang has posted comments on this change. ( http://gerrit.cloudera.org:8080/14114 )
Change subject: [util] Add ExtractFloat method to JsonReader and improve ExtractDouble method
......................................................................
Patch Set 4:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/14114/3/src/kudu/util/jsonreader.cc
File src/kudu/util/jsonreader.cc:
http://gerrit.cloudera.org:8080/#/c/14114/3/src/kudu/util/jsonreader.cc@164
PS3, Line 164: lue(
> nit: missing newline?
Done
--
To view, visit http://gerrit.cloudera.org:8080/14114
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ica0a04fb21daceaa44233995a42a60686f59361b
Gerrit-Change-Number: 14114
Gerrit-PatchSet: 4
Gerrit-Owner: Yifan Zhang <ch...@163.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yifan Zhang <ch...@163.com>
Gerrit-Comment-Date: Fri, 23 Aug 2019 07:00:59 +0000
Gerrit-HasComments: Yes
[kudu-CR] [util] Add ExtractFloat method to JsonReader and improve ExtractDouble method
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/14114 )
Change subject: [util] Add ExtractFloat method to JsonReader and improve ExtractDouble method
......................................................................
Patch Set 4: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/14114
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ica0a04fb21daceaa44233995a42a60686f59361b
Gerrit-Change-Number: 14114
Gerrit-PatchSet: 4
Gerrit-Owner: Yifan Zhang <ch...@163.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yifan Zhang <ch...@163.com>
Gerrit-Comment-Date: Fri, 23 Aug 2019 07:27:55 +0000
Gerrit-HasComments: No
[kudu-CR] [util] Add ExtractFloat method to JsonReader and improve ExtractDouble method
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/14114 )
Change subject: [util] Add ExtractFloat method to JsonReader and improve ExtractDouble method
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/14114/2/src/kudu/util/jsonreader.cc
File src/kudu/util/jsonreader.cc:
http://gerrit.cloudera.org:8080/#/c/14114/2/src/kudu/util/jsonreader.cc@248
PS2, Line 248: return fabs(a-b) <= 1e-7;
This seems to be one of the major differences from the rapidjson implementation. What issues exist with using the rapidjson implementation?
--
To view, visit http://gerrit.cloudera.org:8080/14114
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ica0a04fb21daceaa44233995a42a60686f59361b
Gerrit-Change-Number: 14114
Gerrit-PatchSet: 2
Gerrit-Owner: Yifan Zhang <ch...@163.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Wed, 21 Aug 2019 20:23:56 +0000
Gerrit-HasComments: Yes
[kudu-CR] [util] Add ExtractFloat method to JsonReader and improve ExtractDouble method
Posted by "Yifan Zhang (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins, Andrew Wong, Adar Dembo,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/14114
to look at the new patch set (#4).
Change subject: [util] Add ExtractFloat method to JsonReader and improve ExtractDouble method
......................................................................
[util] Add ExtractFloat method to JsonReader and improve ExtractDouble method
ExtractDouble method should not only parse double values, int values
that can be losslessly converted to double should also be parsed.
Though there is already IsLosslessFloat() in rapidjson library,
the implementation has some precision issues.
This patch re-implement the method.
Change-Id: Ica0a04fb21daceaa44233995a42a60686f59361b
---
M src/kudu/tools/tool_action_table.cc
M src/kudu/util/jsonreader-test.cc
M src/kudu/util/jsonreader.cc
M src/kudu/util/jsonreader.h
4 files changed, 102 insertions(+), 12 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/14/14114/4
--
To view, visit http://gerrit.cloudera.org:8080/14114
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ica0a04fb21daceaa44233995a42a60686f59361b
Gerrit-Change-Number: 14114
Gerrit-PatchSet: 4
Gerrit-Owner: Yifan Zhang <ch...@163.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yifan Zhang <ch...@163.com>
[kudu-CR] [util] Add ExtractFloat method to JsonReader and improve ExtractDouble method
Posted by "Yifan Zhang (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/14114
to look at the new patch set (#2).
Change subject: [util] Add ExtractFloat method to JsonReader and improve ExtractDouble method
......................................................................
[util] Add ExtractFloat method to JsonReader and improve ExtractDouble method
ExtractDouble method should not only parse double values, int values
that can be losslessly converted to double should also be parsed.
Though there are already IsLosslessDouble()/IsLosslessFloat() methods
in rapidjson library, the implementation seems have problems.
This patch defined new methods to determine whether a value can be
losslessly converted to a double/float value.
Change-Id: Ica0a04fb21daceaa44233995a42a60686f59361b
---
M src/kudu/tools/tool_action_table.cc
M src/kudu/util/jsonreader-test.cc
M src/kudu/util/jsonreader.cc
M src/kudu/util/jsonreader.h
4 files changed, 108 insertions(+), 6 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/14/14114/2
--
To view, visit http://gerrit.cloudera.org:8080/14114
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ica0a04fb21daceaa44233995a42a60686f59361b
Gerrit-Change-Number: 14114
Gerrit-PatchSet: 2
Gerrit-Owner: Yifan Zhang <ch...@163.com>
Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] [util] Add ExtractFloat method to JsonReader and improve ExtractDouble method
Posted by "Yifan Zhang (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins, Andrew Wong, Adar Dembo,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/14114
to look at the new patch set (#3).
Change subject: [util] Add ExtractFloat method to JsonReader and improve ExtractDouble method
......................................................................
[util] Add ExtractFloat method to JsonReader and improve ExtractDouble method
ExtractDouble method should not only parse double values, int values
that can be losslessly converted to double should also be parsed.
Though there is already IsLosslessFloat() in rapidjson library,
the implementation has some precision issues.
This patch re-implement the method.
Change-Id: Ica0a04fb21daceaa44233995a42a60686f59361b
---
M src/kudu/tools/tool_action_table.cc
M src/kudu/util/jsonreader-test.cc
M src/kudu/util/jsonreader.cc
M src/kudu/util/jsonreader.h
4 files changed, 89 insertions(+), 6 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/14/14114/3
--
To view, visit http://gerrit.cloudera.org:8080/14114
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ica0a04fb21daceaa44233995a42a60686f59361b
Gerrit-Change-Number: 14114
Gerrit-PatchSet: 3
Gerrit-Owner: Yifan Zhang <ch...@163.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] [util] Add ExtractFloat method to JsonReader and improve ExtractDouble method
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/14114 )
Change subject: [util] Add ExtractFloat method to JsonReader and improve ExtractDouble method
......................................................................
Patch Set 3: Code-Review+1
(2 comments)
http://gerrit.cloudera.org:8080/#/c/14114/2/src/kudu/util/jsonreader.cc
File src/kudu/util/jsonreader.cc:
http://gerrit.cloudera.org:8080/#/c/14114/2/src/kudu/util/jsonreader.cc@248
PS2, Line 248:
> There are some precision issues in rapidjson implementation.
I see, thanks for clarifying. I agree that having an upper bound on error seems reasonable.
http://gerrit.cloudera.org:8080/#/c/14114/3/src/kudu/util/jsonreader.cc
File src/kudu/util/jsonreader.cc:
http://gerrit.cloudera.org:8080/#/c/14114/3/src/kudu/util/jsonreader.cc@164
PS3, Line 164: ; }
nit: missing newline?
--
To view, visit http://gerrit.cloudera.org:8080/14114
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ica0a04fb21daceaa44233995a42a60686f59361b
Gerrit-Change-Number: 14114
Gerrit-PatchSet: 3
Gerrit-Owner: Yifan Zhang <ch...@163.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yifan Zhang <ch...@163.com>
Gerrit-Comment-Date: Fri, 23 Aug 2019 06:38:47 +0000
Gerrit-HasComments: Yes
[kudu-CR] [util] Add ExtractFloat method to JsonReader and improve ExtractDouble method
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/14114 )
Change subject: [util] Add ExtractFloat method to JsonReader and improve ExtractDouble method
......................................................................
[util] Add ExtractFloat method to JsonReader and improve ExtractDouble method
ExtractDouble method should not only parse double values, int values
that can be losslessly converted to double should also be parsed.
Though there is already IsLosslessFloat() in rapidjson library,
the implementation has some precision issues.
This patch re-implement the method.
Change-Id: Ica0a04fb21daceaa44233995a42a60686f59361b
Reviewed-on: http://gerrit.cloudera.org:8080/14114
Tested-by: Kudu Jenkins
Reviewed-by: Andrew Wong <aw...@cloudera.com>
---
M src/kudu/tools/tool_action_table.cc
M src/kudu/util/jsonreader-test.cc
M src/kudu/util/jsonreader.cc
M src/kudu/util/jsonreader.h
4 files changed, 102 insertions(+), 12 deletions(-)
Approvals:
Kudu Jenkins: Verified
Andrew Wong: Looks good to me, approved
--
To view, visit http://gerrit.cloudera.org:8080/14114
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: Ica0a04fb21daceaa44233995a42a60686f59361b
Gerrit-Change-Number: 14114
Gerrit-PatchSet: 5
Gerrit-Owner: Yifan Zhang <ch...@163.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yifan Zhang <ch...@163.com>