{"action":"created","issue":{"url":"https://api.github.com/repos/google/openrtb/issues/16","labels_url":"https://api.github.com/repos/google/openrtb/issues/16/labels{/name}","comments_url":"https://api.github.com/repos/google/openrtb/issues/16/comments","events_url":"https://api.github.com/repos/google/openrtb/issues/16/events","html_url":"https://github.com/google/openrtb/pull/16","id":55363285,"number":16,"title":"Represent prices as integers internally","user":{"login":"metaquanta","id":333044,"avatar_url":"https://avatars.githubusercontent.com/u/333044?v=3","gravatar_id":"","url":"https://api.github.com/users/metaquanta","html_url":"https://github.com/metaquanta","followers_url":"https://api.github.com/users/metaquanta/followers","following_url":"https://api.github.com/users/metaquanta/following{/other_user}","gists_url":"https://api.github.com/users/metaquanta/gists{/gist_id}","starred_url":"https://api.github.com/users/metaquanta/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/metaquanta/subscriptions","organizations_url":"https://api.github.com/users/metaquanta/orgs","repos_url":"https://api.github.com/users/metaquanta/repos","events_url":"https://api.github.com/users/metaquanta/events{/privacy}","received_events_url":"https://api.github.com/users/metaquanta/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"milestone":null,"comments":1,"created_at":"2015-01-24T08:26:01Z","updated_at":"2015-01-24T23:06:13Z","closed_at":null,"pull_request":{"url":"https://api.github.com/repos/google/openrtb/pulls/16","html_url":"https://github.com/google/openrtb/pull/16","diff_url":"https://github.com/google/openrtb/pull/16.diff","patch_url":"https://github.com/google/openrtb/pull/16.patch"},"body":"These numbers are represented as decimals in the json. However, that does not imply they are floats. The spec even specifies, as duplicated in openrtb.proto, that \"WARNING/Best Practice Note: Although this value is a float, OpenRTB strongly suggests using integer math for accounting to avoid rounding errors.\"\r\nI have changed their internal representation to millionths of currency unit."},"comment":{"url":"https://api.github.com/repos/google/openrtb/issues/comments/71342132","html_url":"https://github.com/google/openrtb/pull/16#issuecomment-71342132","issue_url":"https://api.github.com/repos/google/openrtb/issues/16","id":71342132,"user":{"login":"opinali","id":3017776,"avatar_url":"https://avatars.githubusercontent.com/u/3017776?v=3","gravatar_id":"","url":"https://api.github.com/users/opinali","html_url":"https://github.com/opinali","followers_url":"https://api.github.com/users/opinali/followers","following_url":"https://api.github.com/users/opinali/following{/other_user}","gists_url":"https://api.github.com/users/opinali/gists{/gist_id}","starred_url":"https://api.github.com/users/opinali/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/opinali/subscriptions","organizations_url":"https://api.github.com/users/opinali/orgs","repos_url":"https://api.github.com/users/opinali/repos","events_url":"https://api.github.com/users/opinali/events{/privacy}","received_events_url":"https://api.github.com/users/opinali/received_events","type":"User","site_admin":false},"created_at":"2015-01-24T23:06:13Z","updated_at":"2015-01-24T23:06:13Z","body":"@metaquanta thanks for the patch! I just wish we had discussed this before writing the code. The problem is that storing values as decimals is not really helpful unless values are manipulated as decimals everywhere in the application that uses the library; using decimals only for storage or in parse/format operations makes no difference if apps still have to call getters/setters that only use doubles. And of course we can't have a `BigDecimal` in the API because protobuf doesn't support any type that maps to `BigDecimal` or equivalent language-specific decimal types.\r\n\r\nI considered using a `string` field for these values, but that would result in a really inconvenient API, forcing every get/set call to perform a conversion. And this still wouldn't provide any advantage over doubles: today you can just write code like `BigDecimal bidfloor = new BigDecimal(imp.getBidfloor)`. The double field is precise enough to represent any price; it supports 15-17 significant decimal digits of mantissa, and more critically, for any numbers up to 15 digits roundtrip conversions between a string (or precise decimal type) and a double are guaranteed to always be exact. This means `double` is perfectly safe for storage and parsing/formatting of any normal value used in OpenRTB.\r\n\r\nThe above is for background; now specifically on your patch: you're changing the model's field types to `int64` (= Java's `long`), and changing their scale to micros (just like DoubleClick's native protocol). This is another way to solve the problem, but then it forces apps to always manipulate prices in micros instead of \"currency units\" (USD or whatever used by the request). Unfortunately this is even more inconvenient for users of the API; as an end-user myself, if I care about precise decimal arithmetics I'd rather make some explicit `double`/`BigDecima`l conversions than work with micros all the time.\r\n\r\nTechnically, OpenRTB defines only a wire protocol and we have some flexibility for API/language-binding design, see for example how this library uses booleans instead of 0/1 integers for flags; or how I have introduced an extended `admNative` field [native is still WIP]. But these are changes that make the API significantly more type-safe and easy to use. The `admNative` will also result in significant performance gain. Changing the price to micros is a step backward in usability. (Also an important breaking change, one I'd rather avoid at this time, this code already has significant usage...) Even the potential advantage of precision is highly questionable, because `double` is not only good enough for storage and conversions, it's good enough even for arithmetics, since the values we manipulate here leave lots of mantissa bits unused – you'd need a crapton of error propagation before you get a significant error (I doubt any SSP or other platform will consider digits below 1^-6). This is remarkably true for monetary values, which are typically only manipulated by simple operations like sums or multiplies, which have maximum precision (<= 1ulp) in any language or platform; few if any people should calculate bid prices with complex formulas full of transcendental operations that sometimes have several bits of error in some \"hard\" value ranges. And if anyone is doing that, they certainly can't use `long`: even `BigDecimal` would be a huge ordeal because in most mainstream languages – anything other than specialized stuff like Mathematica – the math libraries cannot perform any decent math with decimal types; anything as sophisticated as a square root forces you to step down to floating point anyway.\r\n\r\nHaving said all that, my only concern with the current, simple usage of `double` is in the serialization; for example it would be ugly (although not a real problem) if the library emits JSON like `price: \"1.2000000000000000000003\"` due to accumulation of errors in the last few bits. Ideally, (1) the code for deserialization should ignore anything after some specific limit like the sixth digit after the decimal point, (2) the code for serialization should make sure to only emit digits up to the same limit. I think Java's default behavior in `double`/`String` conversions will take care of (2) but I'm not sure at which digit, and it won't take care of (1) at all. We could use `BigDecimal` to solve these problems, but I think just the `DecimalFormat` API should be good enough for that and way more efficient."}}