Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/8830 )
Change subject: KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client) ...................................................................... Patch Set 8: (21 comments) Still tweaking the decimal impl a bit and adding some tests, but pushing an update for review and discussion. http://gerrit.cloudera.org:8080/#/c/8830/8//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/8830/8//COMMIT_MSG@17 PS8, Line 17: stored > not stored? it must be stored in the metadata somewhere right? Right, I should clarify. What I mean is they are not serialized with each value. I will update this. http://gerrit.cloudera.org:8080/#/c/8830/8//COMMIT_MSG@24 PS8, Line 24: int128 suffixes > perhaps we should guard them with a preprocessor check for c++11 instead? Yeah, I think we should. I want to do that in a separate patch since the base functionality block progress on the Impala side. I can remove it in a separate patch too if that helps. http://gerrit.cloudera.org:8080/#/c/8830/8/src/kudu/client/schema.h File src/kudu/client/schema.h: http://gerrit.cloudera.org:8080/#/c/8830/8/src/kudu/client/schema.h@63 PS8, Line 63: class KuduColumnTypeAttributes { > should this be exported for users? If so I think we need to PIMPL it for AB Yes, this should be exported. I will add KUDU_EXPORT. I modeled this to match KuduColumnStorageAttributes which doesn't use PImpl. Do you know why? http://gerrit.cloudera.org:8080/#/c/8830/8/src/kudu/client/schema.h@177 PS8, Line 177: /// @todo KUDU-809: make this hard-to-use constructor private. : /// Clients should use the Builder API. Currently only the Python API : /// uses this old API. > this has been deprecated for several releases now and I dont think python u Oh right, even defaulted arguments aren't compatible. I can make remove (make it private) if thats the route we should take. http://gerrit.cloudera.org:8080/#/c/8830/8/src/kudu/client/schema.h@323 PS8, Line 323: floating-point > it's not floating point, right? This was taken right from the Impala docs. I agree though, I think a more accurate term would be "fractional". http://gerrit.cloudera.org:8080/#/c/8830/8/src/kudu/client/schema.h@328 PS8, Line 328: /// The precision must be between 0 and 38. > curious what a precision of 0 means... that can only store the value 0? Should be between 1 and 0. http://gerrit.cloudera.org:8080/#/c/8830/8/src/kudu/client/schema.h@343 PS8, Line 343: /// columns precision. > typo: column's Done http://gerrit.cloudera.org:8080/#/c/8830/8/src/kudu/client/schema.cc File src/kudu/client/schema.cc: http://gerrit.cloudera.org:8080/#/c/8830/8/src/kudu/client/schema.cc@133 PS8, Line 133: return kudu::DECIMAL128; > nit: weird indentation Done http://gerrit.cloudera.org:8080/#/c/8830/8/src/kudu/client/schema.cc@153 PS8, Line 153: case kudu::DECIMAL32 : return KuduColumnSchema::DECIMAL; > nit: extra space Done http://gerrit.cloudera.org:8080/#/c/8830/8/src/kudu/client/schema.cc@266 PS8, Line 266: return Status::InvalidArgument("no scale provided for decimal column", data_->name); > is a default scale of 0 not reasonable? I seem to recall that sql allows DE This was discussed some on the design doc, but we never settled for sure. I think a default scale of 0 is reasonable and fairly common. Happy to change to use that default. http://gerrit.cloudera.org:8080/#/c/8830/8/src/kudu/client/schema.cc@285 PS8, Line 285: } > can we check that no precision/scale are specified for non-decimal column t Absolutely. Will add that. http://gerrit.cloudera.org:8080/#/c/8830/8/src/kudu/client/schema.cc@287 PS8, Line 287: int32_t precision = 0; : if (data_->has_precision) { : precision = data_->precision; : } > think ternary would be easier to read here and below agreed. http://gerrit.cloudera.org:8080/#/c/8830/8/src/kudu/client/schema.cc@297 PS8, Line 297: KuduColumnTypeAttributes kuduColumnTypeAttributes = > nit: var name style Done http://gerrit.cloudera.org:8080/#/c/8830/8/src/kudu/client/schema.cc@641 PS8, Line 641: typeAttrs > nit: style Done http://gerrit.cloudera.org:8080/#/c/8830/8/src/kudu/common/decimal.h File src/kudu/common/decimal.h: http://gerrit.cloudera.org:8080/#/c/8830/8/src/kudu/common/decimal.h@53 PS8, Line 53: static const int32_t MIN_DECIMAL_PRECISION = 0; > see comment elsewhere about precision 0 Done http://gerrit.cloudera.org:8080/#/c/8830/8/src/kudu/common/decimal.h@63 PS8, Line 63: explicit Decimal(int128_t s) : > I sort of feel like this Decimal instance should retain its precision and s Yeah, I commented that I was pretty conflicted about this too on one of Dan's reviews. Had I created this class with no reference at all I would definitely have gone that route. The only thing that made me leave off precision and scale was the Impala Implementation. The comment on Impala's DecimalValue is, "The decimal does not store its precision and scale since we'd like to keep the storage as small as possible". http://gerrit.cloudera.org:8080/#/c/8830/8/src/kudu/common/decimal.cc File src/kudu/common/decimal.cc: http://gerrit.cloudera.org:8080/#/c/8830/8/src/kudu/common/decimal.cc@25 PS8, Line 25: int last_char_idx = precision > nit: indentation off in this file Done http://gerrit.cloudera.org:8080/#/c/8830/8/src/kudu/common/decimal.cc@29 PS8, Line 29: string > no need to repeat type name Done http://gerrit.cloudera.org:8080/#/c/8830/8/src/kudu/common/types.h File src/kudu/common/types.h: http://gerrit.cloudera.org:8080/#/c/8830/8/src/kudu/common/types.h@558 PS8, Line 558: // to format it. > maybe we should still format it in such a way that it's clear the value bei Yeah, I think thats a good idea. I could add _D32, _D64, and _D128 to the end of the numbers. That gives the added benefit of knowing the storage size. http://gerrit.cloudera.org:8080/#/c/8830/8/src/kudu/common/wire_protocol.cc File src/kudu/common/wire_protocol.cc: http://gerrit.cloudera.org:8080/#/c/8830/8/src/kudu/common/wire_protocol.cc@214 PS8, Line 214: pb->mutable_type_attributes()->set_precision(col_schema.type_attributes().precision); > maybe we should only serialize them in the case that it is decimal, to save Good idea. Will do. http://gerrit.cloudera.org:8080/#/c/8830/8/src/kudu/common/wire_protocol.cc@264 PS8, Line 264: typeAttributes > style (also below) Done -- To view, visit http://gerrit.cloudera.org:8080/8830 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I3b06142f43c66973f36376bd2c88ca6f8d9f7632 Gerrit-Change-Number: 8830 Gerrit-PatchSet: 8 Gerrit-Owner: Grant Henke <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Dan Burkert <[email protected]> Gerrit-Reviewer: Grant Henke <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-Comment-Date: Tue, 16 Jan 2018 19:17:14 +0000 Gerrit-HasComments: Yes
