{"action":"created","comment":{"url":"https://api.github.com/repos/druid-io/druid/pulls/comments/143855080","pull_request_review_id":68424524,"id":143855080,"diff_hunk":"@@ -22,6 +22,7 @@\n import io.druid.collections.bitmap.MutableBitmap;\n import io.druid.query.aggregation.Aggregator;\n import io.druid.segment.DimensionSelector;\n+import io.druid.segment.NullHandlingHelper;\n import io.druid.segment.data.IndexedInts;\n \n public class DistinctCountAggregator implements Aggregator","path":"extensions-contrib/distinctcount/src/main/java/io/druid/query/aggregation/distinctcount/DistinctCountAggregator.java","position":7,"original_position":7,"commit_id":"c5fbbd532f859f48bc6583f92e8866cd01fcb824","original_commit_id":"824c2575fc414adbc7a39b541ed82772b44c6788","user":{"login":"leventov","id":609240,"avatar_url":"https://avatars0.githubusercontent.com/u/609240?v=4","gravatar_id":"","url":"https://api.github.com/users/leventov","html_url":"https://github.com/leventov","followers_url":"https://api.github.com/users/leventov/followers","following_url":"https://api.github.com/users/leventov/following{/other_user}","gists_url":"https://api.github.com/users/leventov/gists{/gist_id}","starred_url":"https://api.github.com/users/leventov/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/leventov/subscriptions","organizations_url":"https://api.github.com/users/leventov/orgs","repos_url":"https://api.github.com/users/leventov/repos","events_url":"https://api.github.com/users/leventov/events{/privacy}","received_events_url":"https://api.github.com/users/leventov/received_events","type":"User","site_admin":false},"body":"DistinctCountAggregator seems to be broken, when selector doesn't have cardinality, or `idLookup()` returns null. E. g. when there is extraction function, SingleScanTimeDimSelector, etc.","created_at":"2017-10-10T21:17:39Z","updated_at":"2017-10-10T22:02:21Z","html_url":"https://github.com/druid-io/druid/pull/4754#discussion_r143855080","pull_request_url":"https://api.github.com/repos/druid-io/druid/pulls/4754","author_association":"MEMBER","_links":{"self":{"href":"https://api.github.com/repos/druid-io/druid/pulls/comments/143855080"},"html":{"href":"https://github.com/druid-io/druid/pull/4754#discussion_r143855080"},"pull_request":{"href":"https://api.github.com/repos/druid-io/druid/pulls/4754"}}},"pull_request":{"url":"https://api.github.com/repos/druid-io/druid/pulls/4754","id":139384253,"html_url":"https://github.com/druid-io/druid/pull/4754","diff_url":"https://github.com/druid-io/druid/pull/4754.diff","patch_url":"https://github.com/druid-io/druid/pull/4754.patch","issue_url":"https://api.github.com/repos/druid-io/druid/issues/4754","number":4754,"state":"open","locked":false,"title":"Make NULL handling in Druid more compatible to SQL standard","user":{"login":"nishantmonu51","id":5023786,"avatar_url":"https://avatars3.githubusercontent.com/u/5023786?v=4","gravatar_id":"","url":"https://api.github.com/users/nishantmonu51","html_url":"https://github.com/nishantmonu51","followers_url":"https://api.github.com/users/nishantmonu51/followers","following_url":"https://api.github.com/users/nishantmonu51/following{/other_user}","gists_url":"https://api.github.com/users/nishantmonu51/gists{/gist_id}","starred_url":"https://api.github.com/users/nishantmonu51/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/nishantmonu51/subscriptions","organizations_url":"https://api.github.com/users/nishantmonu51/orgs","repos_url":"https://api.github.com/users/nishantmonu51/repos","events_url":"https://api.github.com/users/nishantmonu51/events{/privacy}","received_events_url":"https://api.github.com/users/nishantmonu51/received_events","type":"User","site_admin":false},"body":"This PR contains changes to improve handling of null values in druid by treating nulls as missing values which don’t actually exist. This will make it more compatible to SQL standard and help with integration with other existing BI systems which support ODBC/JDBC. (Detailed description in proposal - https://github.com/druid-io/druid/issues/4349)\r\n\r\nChanges include - \r\n\r\n## New Configuration \r\n\r\n* New configuration - **_\"druid.null.handling.useDefaultValueForNull\"_** in class NullValueHandlingConfig default to true for preserving old behavior.\r\n* NullHandlingHelper - this a helper class with static methods for handling optional nullToEmpty / emptyToNull conversions for switching between old/new behavior. Without this static helper, we would need to inject NullValueHandlingConfig almost everywhere throughout the querying and indexing layer. \r\n\r\n## Storage Layer - \r\n\r\n* GenericIndexed - Added ability to distinguish between null/empty bytes. the value buffer for GenericIndexed contains element size followed by  the bytes, for null values size is written as -1 and for empty bytes size is 0. \r\n* String Columns - the dictionary is stores as a genericIndexed, which now can contain null value. An inverted index is created with the id assigned to null value. Null and empty strings will get different id's in dictionary encoding.\r\n* Numeric Columns - The column is serialized as primitive objects of long/float/double type, a nullRowsBitmap is also stored which is used to distinguish rows having 0 and null values.  \r\n* Complex columns - Complex columns use genericIndexed internally for storage, so supported nulls in genericIndexed make this work. the ObjectStrategy can choose to return null objects when deserializing data. \r\n\r\n## Indexing Layer - \r\n\r\n* Row - instead of returning primitive types return Float/Long/Double objects so that indexer can distinguish between null and 0 values.\r\n* StringIndexer,DoubleIndexer,LongIndexer,FloatIndexer changed to distinguish between null and empty values. \r\n* ColumnValueSelector - added new method isNull() to check whether a value is null or not while aggregating. this is mainly done in order to avoid boxing/unboxing from primitive values while doing segment scans. \r\n\r\n## Nullability for Aggregators/Metrics\r\n\r\nAs per the current implementation, most aggregators are coded to replace null values with default e.g. sum treats them as zeroes, min treats them as positive infinity, etc.\r\nTo match the new semantics and make aggregators nullable, where if an aggregator encounters only the null values the result will be null value following changes are made -\r\n\r\n* Aggrgator/BufferAggregator - Added a new method boolean isNull() which returns false by default. aggregators that support nullability can choose to override this and return true if the aggregated result is null. \r\n* Added a NullableAggregator/NullableBufferAggregator which is be used as a decorator for existing aggregator implementations to make them nullable. This will return null values if all the values aggregated are null, If any of the aggregated value is non-null, it will have same behavior as the delegate aggregator. \r\n* If new null behavior is enabled, aggregator factories will decorate the aggregators with NullableAggregator/NullableBufferAggregator. \r\n* Cardinality and HyperUnique  aggregators ignore null values and only count non-null values, If all the encountered values are null, the result will be 0. This is different from current behavior where null values are also counted.\r\n* Count aggregator - since count aggregator is not associated with any column and just a count of the number of rows, it has the same behavior as before. \r\n\r\n## Math Expressions Null Handling\r\n* For general calculations like sum, full expression is be considered as null if any of the components is null. \r\n* StringLiteral now supports null and and expressions containing non-quoted null is parsed as StringLiteral with null \r\n* Specifying a default value for null is supported by the use of NVL or IF clause to assign default values at query time.\r\n\r\n## Filtering on Null values\r\n* SelectorDimFilter currently specifies filtering on null values but the implementation assumes null and empty strings as equivalent. The implementation is changed to consider null and empty string differently. Generation of cache key for selectorDimFilter is also modified to support null.\r\n* InFilter/ExpressionFilter/LikeFilter are also modified to not treat nulls as empty strings. \r\n\r\n## Changes to Druid build-in SQL layer\r\n* NULL and empty strings are treated differently. \r\n* IS NULL and IS NOT NULL filters against null values instead of empty strings. \r\n* NVL, IFNULL and COALESCE work as per SQL standard. \r\n* count now skips null values and works as per SQL standard. \r\n\r\n## Misc Changes\r\n\r\nAbove are the major changes in the user facing APIs and behavior. Other than these there are multiple places in the code where we convert empty Strings to null and vice-versa. They are changed in order to treat null and String values separately.\r\n\r\n## Backwards Compatibility \r\n* **_\"druid.null.handling.useDefaultValueForNull\"_** is added as a new config. The default value is set to true for backwards compatibility. \r\n*  code places in query/ingestion where we converted emptyToNull or nullToEmpty now use NullHandlingHelper which only does the conversion if backwards compatibility is enabled. \r\n* Storage layer changes are backwards compatible for GenericIndexed/String columns. \r\n* For Numeric columns new V2 serde implementations are introduced which also supports serde of nullRowsBitmaps. Previous implementations are kept unchanged to read any existing segments. \r\n\r\n## Testing \r\n* Most of the unit tests that tested nulls are modified to have two branches of expected results to test new/old behavior both. \r\n* For CI have added new runs to test matrix to test new behavior for nulls. \r\n* To run the tests with new behavior run **_\"-Ddruid.null.handling.useDefaultValueForNull=false\"_**\r\n\r\n\r\n\r\n\r\n\r\n","created_at":"2017-09-05T17:54:52Z","updated_at":"2017-10-10T22:02:21Z","closed_at":null,"merged_at":null,"merge_commit_sha":"980ab1f97d1a5256bd99dc29e82be52cea5906b3","assignee":null,"assignees":[],"requested_reviewers":[{"login":"gianm","id":1214075,"avatar_url":"https://avatars3.githubusercontent.com/u/1214075?v=4","gravatar_id":"","url":"https://api.github.com/users/gianm","html_url":"https://github.com/gianm","followers_url":"https://api.github.com/users/gianm/followers","following_url":"https://api.github.com/users/gianm/following{/other_user}","gists_url":"https://api.github.com/users/gianm/gists{/gist_id}","starred_url":"https://api.github.com/users/gianm/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/gianm/subscriptions","organizations_url":"https://api.github.com/users/gianm/orgs","repos_url":"https://api.github.com/users/gianm/repos","events_url":"https://api.github.com/users/gianm/events{/privacy}","received_events_url":"https://api.github.com/users/gianm/received_events","type":"User","site_admin":false}],"milestone":null,"commits_url":"https://api.github.com/repos/druid-io/druid/pulls/4754/commits","review_comments_url":"https://api.github.com/repos/druid-io/druid/pulls/4754/comments","review_comment_url":"https://api.github.com/repos/druid-io/druid/pulls/comments{/number}","comments_url":"https://api.github.com/repos/druid-io/druid/issues/4754/comments","statuses_url":"https://api.github.com/repos/druid-io/druid/statuses/c5fbbd532f859f48bc6583f92e8866cd01fcb824","head":{"label":"nishantmonu51:null-handling-4","ref":"null-handling-4","sha":"c5fbbd532f859f48bc6583f92e8866cd01fcb824","user":{"login":"nishantmonu51","id":5023786,"avatar_url":"https://avatars3.githubusercontent.com/u/5023786?v=4","gravatar_id":"","url":"https://api.github.com/users/nishantmonu51","html_url":"https://github.com/nishantmonu51","followers_url":"https://api.github.com/users/nishantmonu51/followers","following_url":"https://api.github.com/users/nishantmonu51/following{/other_user}","gists_url":"https://api.github.com/users/nishantmonu51/gists{/gist_id}","starred_url":"https://api.github.com/users/nishantmonu51/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/nishantmonu51/subscriptions","organizations_url":"https://api.github.com/users/nishantmonu51/orgs","repos_url":"https://api.github.com/users/nishantmonu51/repos","events_url":"https://api.github.com/users/nishantmonu51/events{/privacy}","received_events_url":"https://api.github.com/users/nishantmonu51/received_events","type":"User","site_admin":false},"repo":{"id":30287395,"name":"druid","full_name":"nishantmonu51/druid","owner":{"login":"nishantmonu51","id":5023786,"avatar_url":"https://avatars3.githubusercontent.com/u/5023786?v=4","gravatar_id":"","url":"https://api.github.com/users/nishantmonu51","html_url":"https://github.com/nishantmonu51","followers_url":"https://api.github.com/users/nishantmonu51/followers","following_url":"https://api.github.com/users/nishantmonu51/following{/other_user}","gists_url":"https://api.github.com/users/nishantmonu51/gists{/gist_id}","starred_url":"https://api.github.com/users/nishantmonu51/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/nishantmonu51/subscriptions","organizations_url":"https://api.github.com/users/nishantmonu51/orgs","repos_url":"https://api.github.com/users/nishantmonu51/repos","events_url":"https://api.github.com/users/nishantmonu51/events{/privacy}","received_events_url":"https://api.github.com/users/nishantmonu51/received_events","type":"User","site_admin":false},"private":false,"html_url":"https://github.com/nishantmonu51/druid","description":"Real²time Exploratory Analytics on Large Datasets","fork":true,"url":"https://api.github.com/repos/nishantmonu51/druid","forks_url":"https://api.github.com/repos/nishantmonu51/druid/forks","keys_url":"https://api.github.com/repos/nishantmonu51/druid/keys{/key_id}","collaborators_url":"https://api.github.com/repos/nishantmonu51/druid/collaborators{/collaborator}","teams_url":"https://api.github.com/repos/nishantmonu51/druid/teams","hooks_url":"https://api.github.com/repos/nishantmonu51/druid/hooks","issue_events_url":"https://api.github.com/repos/nishantmonu51/druid/issues/events{/number}","events_url":"https://api.github.com/repos/nishantmonu51/druid/events","assignees_url":"https://api.github.com/repos/nishantmonu51/druid/assignees{/user}","branches_url":"https://api.github.com/repos/nishantmonu51/druid/branches{/branch}","tags_url":"https://api.github.com/repos/nishantmonu51/druid/tags","blobs_url":"https://api.github.com/repos/nishantmonu51/druid/git/blobs{/sha}","git_tags_url":"https://api.github.com/repos/nishantmonu51/druid/git/tags{/sha}","git_refs_url":"https://api.github.com/repos/nishantmonu51/druid/git/refs{/sha}","trees_url":"https://api.github.com/repos/nishantmonu51/druid/git/trees{/sha}","statuses_url":"https://api.github.com/repos/nishantmonu51/druid/statuses/{sha}","languages_url":"https://api.github.com/repos/nishantmonu51/druid/languages","stargazers_url":"https://api.github.com/repos/nishantmonu51/druid/stargazers","contributors_url":"https://api.github.com/repos/nishantmonu51/druid/contributors","subscribers_url":"https://api.github.com/repos/nishantmonu51/druid/subscribers","subscription_url":"https://api.github.com/repos/nishantmonu51/druid/subscription","commits_url":"https://api.github.com/repos/nishantmonu51/druid/commits{/sha}","git_commits_url":"https://api.github.com/repos/nishantmonu51/druid/git/commits{/sha}","comments_url":"https://api.github.com/repos/nishantmonu51/druid/comments{/number}","issue_comment_url":"https://api.github.com/repos/nishantmonu51/druid/issues/comments{/number}","contents_url":"https://api.github.com/repos/nishantmonu51/druid/contents/{+path}","compare_url":"https://api.github.com/repos/nishantmonu51/druid/compare/{base}...{head}","merges_url":"https://api.github.com/repos/nishantmonu51/druid/merges","archive_url":"https://api.github.com/repos/nishantmonu51/druid/{archive_format}{/ref}","downloads_url":"https://api.github.com/repos/nishantmonu51/druid/downloads","issues_url":"https://api.github.com/repos/nishantmonu51/druid/issues{/number}","pulls_url":"https://api.github.com/repos/nishantmonu51/druid/pulls{/number}","milestones_url":"https://api.github.com/repos/nishantmonu51/druid/milestones{/number}","notifications_url":"https://api.github.com/repos/nishantmonu51/druid/notifications{?since,all,participating}","labels_url":"https://api.github.com/repos/nishantmonu51/druid/labels{/name}","releases_url":"https://api.github.com/repos/nishantmonu51/druid/releases{/id}","deployments_url":"https://api.github.com/repos/nishantmonu51/druid/deployments","created_at":"2015-02-04T07:42:09Z","updated_at":"2016-05-30T13:28:55Z","pushed_at":"2017-10-10T20:58:39Z","git_url":"git://github.com/nishantmonu51/druid.git","ssh_url":"git@github.com:nishantmonu51/druid.git","clone_url":"https://github.com/nishantmonu51/druid.git","svn_url":"https://github.com/nishantmonu51/druid","homepage":"http://druid.io","size":125937,"stargazers_count":0,"watchers_count":0,"language":"Java","has_issues":false,"has_projects":true,"has_downloads":true,"has_wiki":true,"has_pages":false,"forks_count":0,"mirror_url":null,"open_issues_count":2,"forks":0,"open_issues":2,"watchers":0,"default_branch":"master"}},"base":{"label":"druid-io:master","ref":"master","sha":"797b54d283a618ead581aa8cc100eeaeb39e4338","user":{"login":"druid-io","id":4203474,"avatar_url":"https://avatars0.githubusercontent.com/u/4203474?v=4","gravatar_id":"","url":"https://api.github.com/users/druid-io","html_url":"https://github.com/druid-io","followers_url":"https://api.github.com/users/druid-io/followers","following_url":"https://api.github.com/users/druid-io/following{/other_user}","gists_url":"https://api.github.com/users/druid-io/gists{/gist_id}","starred_url":"https://api.github.com/users/druid-io/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/druid-io/subscriptions","organizations_url":"https://api.github.com/users/druid-io/orgs","repos_url":"https://api.github.com/users/druid-io/repos","events_url":"https://api.github.com/users/druid-io/events{/privacy}","received_events_url":"https://api.github.com/users/druid-io/received_events","type":"Organization","site_admin":false},"repo":{"id":6358188,"name":"druid","full_name":"druid-io/druid","owner":{"login":"druid-io","id":4203474,"avatar_url":"https://avatars0.githubusercontent.com/u/4203474?v=4","gravatar_id":"","url":"https://api.github.com/users/druid-io","html_url":"https://github.com/druid-io","followers_url":"https://api.github.com/users/druid-io/followers","following_url":"https://api.github.com/users/druid-io/following{/other_user}","gists_url":"https://api.github.com/users/druid-io/gists{/gist_id}","starred_url":"https://api.github.com/users/druid-io/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/druid-io/subscriptions","organizations_url":"https://api.github.com/users/druid-io/orgs","repos_url":"https://api.github.com/users/druid-io/repos","events_url":"https://api.github.com/users/druid-io/events{/privacy}","received_events_url":"https://api.github.com/users/druid-io/received_events","type":"Organization","site_admin":false},"private":false,"html_url":"https://github.com/druid-io/druid","description":"Column oriented distributed data store ideal for powering interactive applications","fork":false,"url":"https://api.github.com/repos/druid-io/druid","forks_url":"https://api.github.com/repos/druid-io/druid/forks","keys_url":"https://api.github.com/repos/druid-io/druid/keys{/key_id}","collaborators_url":"https://api.github.com/repos/druid-io/druid/collaborators{/collaborator}","teams_url":"https://api.github.com/repos/druid-io/druid/teams","hooks_url":"https://api.github.com/repos/druid-io/druid/hooks","issue_events_url":"https://api.github.com/repos/druid-io/druid/issues/events{/number}","events_url":"https://api.github.com/repos/druid-io/druid/events","assignees_url":"https://api.github.com/repos/druid-io/druid/assignees{/user}","branches_url":"https://api.github.com/repos/druid-io/druid/branches{/branch}","tags_url":"https://api.github.com/repos/druid-io/druid/tags","blobs_url":"https://api.github.com/repos/druid-io/druid/git/blobs{/sha}","git_tags_url":"https://api.github.com/repos/druid-io/druid/git/tags{/sha}","git_refs_url":"https://api.github.com/repos/druid-io/druid/git/refs{/sha}","trees_url":"https://api.github.com/repos/druid-io/druid/git/trees{/sha}","statuses_url":"https://api.github.com/repos/druid-io/druid/statuses/{sha}","languages_url":"https://api.github.com/repos/druid-io/druid/languages","stargazers_url":"https://api.github.com/repos/druid-io/druid/stargazers","contributors_url":"https://api.github.com/repos/druid-io/druid/contributors","subscribers_url":"https://api.github.com/repos/druid-io/druid/subscribers","subscription_url":"https://api.github.com/repos/druid-io/druid/subscription","commits_url":"https://api.github.com/repos/druid-io/druid/commits{/sha}","git_commits_url":"https://api.github.com/repos/druid-io/druid/git/commits{/sha}","comments_url":"https://api.github.com/repos/druid-io/druid/comments{/number}","issue_comment_url":"https://api.github.com/repos/druid-io/druid/issues/comments{/number}","contents_url":"https://api.github.com/repos/druid-io/druid/contents/{+path}","compare_url":"https://api.github.com/repos/druid-io/druid/compare/{base}...{head}","merges_url":"https://api.github.com/repos/druid-io/druid/merges","archive_url":"https://api.github.com/repos/druid-io/druid/{archive_format}{/ref}","downloads_url":"https://api.github.com/repos/druid-io/druid/downloads","issues_url":"https://api.github.com/repos/druid-io/druid/issues{/number}","pulls_url":"https://api.github.com/repos/druid-io/druid/pulls{/number}","milestones_url":"https://api.github.com/repos/druid-io/druid/milestones{/number}","notifications_url":"https://api.github.com/repos/druid-io/druid/notifications{?since,all,participating}","labels_url":"https://api.github.com/repos/druid-io/druid/labels{/name}","releases_url":"https://api.github.com/repos/druid-io/druid/releases{/id}","deployments_url":"https://api.github.com/repos/druid-io/druid/deployments","created_at":"2012-10-23T19:08:07Z","updated_at":"2017-10-10T21:11:47Z","pushed_at":"2017-10-10T20:58:42Z","git_url":"git://github.com/druid-io/druid.git","ssh_url":"git@github.com:druid-io/druid.git","clone_url":"https://github.com/druid-io/druid.git","svn_url":"https://github.com/druid-io/druid","homepage":"http://druid.io","size":126701,"stargazers_count":5524,"watchers_count":5524,"language":"Java","has_issues":true,"has_projects":true,"has_downloads":true,"has_wiki":true,"has_pages":false,"forks_count":1344,"mirror_url":null,"open_issues_count":832,"forks":1344,"open_issues":832,"watchers":5524,"default_branch":"master"}},"_links":{"self":{"href":"https://api.github.com/repos/druid-io/druid/pulls/4754"},"html":{"href":"https://github.com/druid-io/druid/pull/4754"},"issue":{"href":"https://api.github.com/repos/druid-io/druid/issues/4754"},"comments":{"href":"https://api.github.com/repos/druid-io/druid/issues/4754/comments"},"review_comments":{"href":"https://api.github.com/repos/druid-io/druid/pulls/4754/comments"},"review_comment":{"href":"https://api.github.com/repos/druid-io/druid/pulls/comments{/number}"},"commits":{"href":"https://api.github.com/repos/druid-io/druid/pulls/4754/commits"},"statuses":{"href":"https://api.github.com/repos/druid-io/druid/statuses/c5fbbd532f859f48bc6583f92e8866cd01fcb824"}},"author_association":"OWNER"}}
{"actor":{"display_login":"leventov"}}