{"action":"created","issue":{"url":"https://api.github.com/repos/cockroachdb/cockroach/issues/5820","repository_url":"https://api.github.com/repos/cockroachdb/cockroach","labels_url":"https://api.github.com/repos/cockroachdb/cockroach/issues/5820/labels{/name}","comments_url":"https://api.github.com/repos/cockroachdb/cockroach/issues/5820/comments","events_url":"https://api.github.com/repos/cockroachdb/cockroach/issues/5820/events","html_url":"https://github.com/cockroachdb/cockroach/pull/5820","id":145323899,"number":5820,"title":"encoding: Escape null bytes in decimal encoding","user":{"login":"nvanbenschoten","id":5438456,"avatar_url":"https://avatars.githubusercontent.com/u/5438456?v=3","gravatar_id":"","url":"https://api.github.com/users/nvanbenschoten","html_url":"https://github.com/nvanbenschoten","followers_url":"https://api.github.com/users/nvanbenschoten/followers","following_url":"https://api.github.com/users/nvanbenschoten/following{/other_user}","gists_url":"https://api.github.com/users/nvanbenschoten/gists{/gist_id}","starred_url":"https://api.github.com/users/nvanbenschoten/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/nvanbenschoten/subscriptions","organizations_url":"https://api.github.com/users/nvanbenschoten/orgs","repos_url":"https://api.github.com/users/nvanbenschoten/repos","events_url":"https://api.github.com/users/nvanbenschoten/events{/privacy}","received_events_url":"https://api.github.com/users/nvanbenschoten/received_events","type":"User","site_admin":false},"labels":[{"url":"https://api.github.com/repos/cockroachdb/cockroach/labels/bug","name":"bug","color":"fc2929"},{"url":"https://api.github.com/repos/cockroachdb/cockroach/labels/noteworthy","name":"noteworthy","color":"d4c5f9"}],"state":"open","locked":false,"assignee":{"login":"petermattis","id":6201335,"avatar_url":"https://avatars.githubusercontent.com/u/6201335?v=3","gravatar_id":"","url":"https://api.github.com/users/petermattis","html_url":"https://github.com/petermattis","followers_url":"https://api.github.com/users/petermattis/followers","following_url":"https://api.github.com/users/petermattis/following{/other_user}","gists_url":"https://api.github.com/users/petermattis/gists{/gist_id}","starred_url":"https://api.github.com/users/petermattis/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/petermattis/subscriptions","organizations_url":"https://api.github.com/users/petermattis/orgs","repos_url":"https://api.github.com/users/petermattis/repos","events_url":"https://api.github.com/users/petermattis/events{/privacy}","received_events_url":"https://api.github.com/users/petermattis/received_events","type":"User","site_admin":false},"milestone":null,"comments":2,"created_at":"2016-04-02T01:49:29Z","updated_at":"2016-04-02T17:29:50Z","closed_at":null,"pull_request":{"url":"https://api.github.com/repos/cockroachdb/cockroach/pulls/5820","html_url":"https://github.com/cockroachdb/cockroach/pull/5820","diff_url":"https://github.com/cockroachdb/cockroach/pull/5820.diff","patch_url":"https://github.com/cockroachdb/cockroach/pull/5820.patch"},"body":"Fixes #5812.\r\n\r\ncc. @bdarnell @knz \r\n\r\nNOTE: this is a **breaking change** for our on-disk encoding, but\r\nfixes a serious issue.\r\n\r\nPreviously we terminated our decimal encoding with a null byte,\r\nbut we never escaped null bytes when we ripped them out of the\r\ndecimal's `big.Int`. This meant that when we went to decode, we\r\nwould search and find the first null byte, and used that as a\r\nterminator, resulting in an incorrect `big.Int` value within the\r\n`inf.Dec`. This change escapes these null bytes during encoding,\r\nwhich fixes the issue, but unfortunately also hurt performance\r\nslightly.\r\n\r\n```\r\nname               old time/op    new time/op    delta\r\nEncodeDecimal-4       141ns ± 1%     159ns ± 2%  +12.48%        (p=0.000 n=9+10)\r\nDecodeDecimal-4       203ns ± 4%     244ns ± 1%  +20.44%        (p=0.000 n=10+8)\r\nValueSetDecimal-4     193ns ± 3%     198ns ± 1%   +2.49%        (p=0.002 n=10+10)\r\nValueGetDecimal-4     204ns ± 2%     242ns ± 2%  +18.84%        (p=0.000 n=10+10)\r\n\r\nname               old alloc/op   new alloc/op   delta\r\nEncodeDecimal-4      0.00B ±NaN%    0.00B ±NaN%     ~     (all samples are equal)\r\nDecodeDecimal-4       96.0B ± 0%     96.0B ± 0%     ~     (all samples are equal)\r\nValueSetDecimal-4     32.0B ± 0%     32.0B ± 0%     ~     (all samples are equal)\r\nValueGetDecimal-4     96.0B ± 0%     96.0B ± 0%     ~     (all samples are equal)\r\n\r\nname               old allocs/op  new allocs/op  delta\r\nEncodeDecimal-4       0.00 ±NaN%     0.00 ±NaN%     ~     (all samples are equal)\r\nDecodeDecimal-4        2.00 ± 0%      2.00 ± 0%     ~     (all samples are equal)\r\nValueSetDecimal-4      1.00 ± 0%      1.00 ± 0%     ~     (all samples are equal)\r\nValueGetDecimal-4      2.00 ± 0%      2.00 ± 0%     ~     (all samples are equal)\r\n```\r\n\r\nThis change also provides a fix for the larger entries (2^64+) in the `digitsLookupTable`.\r\nPreviously the left-shift strategy for populating the digitsLookupTable\r\nwhen the element number was over #63 would overflow. This commit fixes\r\nthis and adds a test for the table.\n\n<!-- Reviewable:start -->\n---\nThis change is [<img src=\"https://reviewable.io/review_button.svg\" height=\"35\" align=\"absmiddle\" alt=\"Reviewable\"/>](https://reviewable.io/reviews/cockroachdb/cockroach/5820)\n<!-- Reviewable:end -->\n"},"comment":{"url":"https://api.github.com/repos/cockroachdb/cockroach/issues/comments/204763119","html_url":"https://github.com/cockroachdb/cockroach/pull/5820#issuecomment-204763119","issue_url":"https://api.github.com/repos/cockroachdb/cockroach/issues/5820","id":204763119,"user":{"login":"nvanbenschoten","id":5438456,"avatar_url":"https://avatars.githubusercontent.com/u/5438456?v=3","gravatar_id":"","url":"https://api.github.com/users/nvanbenschoten","html_url":"https://github.com/nvanbenschoten","followers_url":"https://api.github.com/users/nvanbenschoten/followers","following_url":"https://api.github.com/users/nvanbenschoten/following{/other_user}","gists_url":"https://api.github.com/users/nvanbenschoten/gists{/gist_id}","starred_url":"https://api.github.com/users/nvanbenschoten/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/nvanbenschoten/subscriptions","organizations_url":"https://api.github.com/users/nvanbenschoten/orgs","repos_url":"https://api.github.com/users/nvanbenschoten/repos","events_url":"https://api.github.com/users/nvanbenschoten/events{/privacy}","received_events_url":"https://api.github.com/users/nvanbenschoten/received_events","type":"User","site_admin":false},"created_at":"2016-04-02T17:29:50Z","updated_at":"2016-04-02T17:29:50Z","body":"\n\n\n\nReview status: 0 of 7 files reviewed at latest revision, 5 unresolved discussions.\n\n---\n\n*[util/encoding/decimal.go, line 367 \\[r1\\]](https://reviewable.io:443/reviews/cockroachdb/cockroach/5820#-KEMMN13NdtofCl49IN_:-KEMynmchgDtdrgGwIeQ:-1009279113) ([raw file](https://github.com/cockroachdb/cockroach/blob/7b737e06fb8d0cbfc108931f278a0e9a34dc725e/util/encoding/decimal.go#L367)):*\nAdded a comment. Also, just to double check for complete confidence (yours and mine), check out https://play.golang.org/p/VGIQWiqp0o\n\n---\n\n*[util/encoding/decimal_test.go, line 37 \\[r1\\]](https://reviewable.io:443/reviews/cockroachdb/cockroach/5820#-KEMNAkSZ-7oL3Z32_z4:-KEMz_K-3pWYZRq9bezi:381448792) ([raw file](https://github.com/cockroachdb/cockroach/blob/7b737e06fb8d0cbfc108931f278a0e9a34dc725e/util/encoding/decimal_test.go#L37)):*\nIn a similar way to how we handle descending byte encodings (see `TestEncodeDecodeBytesDescending`),  we ones complement the entire encoded byte slice for inverted numbers (negative xor descending), so even the terminators are complemented. During decoding, we detect either of these two situations from the marker and ones complement the byte slice again before calling `decodeBytesInternal`. \n\nWe could choose not to complement the last two bytes on either end, but that approach seems slightly more susceptible to issues. For instance, I would have to add an assertion that the byte slice is at least 2 bytes long in `decodeDecimalValue`, instead of letting `decodeBytesInternal` deal with any malformed input.\n\n---\n\n\n*Comments from [Reviewable](https://reviewable.io:443/reviews/cockroachdb/cockroach/5820)*\n<!-- Sent from Reviewable.io -->\n"}}