Support ingestion of long/float dimensions by jon-wei · Pull Request #3966 · apache/druid

jon-wei · 2017-02-23T02:12:35Z

This PR adds long and float implementations of DimensionHandler/DimensionIndexer/DimensionMerger, allowing ingestion of long/float typed dimensions.

This also changes the EncodedTypeArray type parameter in the interfaces above to EncodedKeyComponentType, removing the restriction that the individual fields of row keys during ingestion must be arrays (to allow for single numeric values in the keys, since long/floats don't support multivalue rows)

gianm

@jon-wei, the patch looks good other than the comments, although the usage of makeColumnValueSelector seems a little sketchy to me.

It looks like what IncrementalIndexStorageAdapter does is that if you ask for a selector for a dimension, then based on what kind of selector you ask for, it casts the result of makeColumnValueSelector to match (so makeFloatColumnSelector casts it to FloatColumnSelector).

This seems sketchy because it should be allowable to ask for a long selector on a float column, and then you should get a selector that casts each float to a long.

Questions for you:

Is there some reason (elsewhere in the code) that this is ok?
Do you have tests covering this? (e.g. asking for a long selector on a float column)
Do those tests include aggregations? (e.g. longSum on a float column of an incremental index)
Do those tests include grouping? (e.g. grouping with outputType LONG on a float column of an incremental index)

gianm · 2017-02-24T23:09:40Z

      "dimensionsSpec" : {
-        "dimensions": ["page","language","user","unpatrolled","newPage","robot","anonymous","namespace","continent","country","region","city"],
+        "dimensions": [
+          "page","language","user","unpatrolled","newPage","robot","anonymous","namespace","continent","country","region","city",


Given the new additions, placing each of these on its own line would be more readable.

Moved these to new lines

gianm · 2017-02-24T23:09:58Z

+        "dimensions": [
+          "page","language","user","unpatrolled","newPage","robot","anonymous","namespace","continent","country","region","city",
+          {
+            "type": "LONG",


Do these need to be uppercase? Lowercase is more typical for Druid options in JSON.

They can be lowercase, made them lowercase in the docs now

gianm · 2017-02-24T23:10:27Z

 | Field | Type | Description | Required |
 |-------|------|-------------|----------|
-| dimensions | JSON String array | The names of the dimensions. If this is an empty array, Druid will treat all columns that are not timestamp or metric columns as dimension columns. | yes |
+| dimensions | JSON Object array | A list of [dimension schema](#dimension-schema) objects or dimension names. Providing a name is equivalent to providing a String-typed dimension schema with the given name. If this is an empty array, Druid will treat all columns that are not timestamp or metric columns as String-typed dimension columns. | yes |


It's just a JSON array, since it can mix json strings and json objects.

Changed to "JSON array"

gianm · 2017-02-24T23:11:08Z

+```json
+"dimensionsSpec" : {
+  "dimensions": [
+    "page","language","user","unpatrolled","newPage","robot","anonymous","namespace","continent","country","region","city",


Similar comments to above on formatting.

Moved dimensions to new lines

gianm · 2017-02-27T17:58:17Z

+  @Override
+  public Indexed<Long> getSortedIndexedValues()
+  {
+    return null;


Is this expected to never be called? If so, throw UnsupportedOperationException rather than return null.

Changed this and the bitmap-related method to throw UnsupportedOperationException

gianm · 2017-02-27T17:58:44Z

+  @Override
+  public Long getMinValue()
+  {
+    return 0L;


Long.MIN_VALUE and Long.MAX_VALUE may work better for these two, since people may use the min/max values reported by metadata for pruning segment lists.

Changed these to use MIN_VALUE and MAX_VALUE

gianm · 2017-02-27T18:03:05Z

+  @Override
+  public Object convertUnsortedEncodedKeyComponentToActualArrayOrList(Long key, boolean asList)
+  {
+    return Lists.newArrayList(key);


ImmutableList.of may be cheaper. It just stores the key as a field.

Changed to use ImmutableList.of

gianm · 2017-02-27T18:06:41Z

+  @Override
+  public Float getMinValue()
+  {
+    return 0.0f;


Similar comment to min/max in the Long handler.

Changed to use min/max here as well

gianm · 2017-02-27T18:06:53Z

+  @Override
+  public Indexed<Float> getSortedIndexedValues()
+  {
+    return null;


Similar comment to getSortedIndexedValues in the Long handler.

Changed this and the bitmap-related method to throw UnsupportedOperationException

gianm · 2017-02-27T18:07:08Z

+  @Override
+  public Object convertUnsortedEncodedKeyComponentToActualArrayOrList(Float key, boolean asList)
+  {
+    return Lists.newArrayList(key);


Similar comment to convertUnsortedEncodedKeyComponentToActualArrayOrList in the Long handler.

Changed to use ImmutableList.of

jon-wei · 2017-02-28T00:29:34Z

@gianm
Re: your comment on makeColumnValueSelector

thanks, I missed that, I added a couple of tests, one in GroupBy and one in TopN that have aggregations on numeric columns ingested as dimensions, these trigger the float->long and long->float cases you mentioned in IncrementalIndexStorageAdapter

I added something to makeLong/FloatColumnSelector to create wrapping selectors that cast the values returned by the base selector from the indexer if necessary

gianm · 2017-02-28T17:54:55Z

@jon-wei The wrapping stuff in IISA feels a bit weird -- I think it'd be nicer for the DimensionIndexers to handle that on their own. So, that means replacing makeColumnValueSelector with makeObjectColumnSelector, makeLongColumnSelector, makeFloatColumnSelector, and makeDimensionSelector, giving full control over behavior to the type handler.

The long/float handlers should probably return NullDimensionSelector for makeDimensionSelector since QISA behaves like that too.

The string handler should probably return Zero-selectors for long/float since, again, QISA behaves like that.

gianm · 2017-02-28T17:56:51Z

Also, the current code in your patch throws UnsupportedOperationExceptions in some cases, but I think we should avoid those and return null/zero selectors instead (like QISA).

…t in inc index storage adapter

gianm

👍 latest changes look good to me

fjy · 2017-03-01T03:04:38Z

👍

leventov · 2017-07-04T16:42:00Z

+      IncrementalIndexStorageAdapter.EntryHolder currEntry, IncrementalIndex.DimensionDesc desc
+  )
+  {
+     return ZeroLongColumnSelector.instance();


How valid is it to return zero selector here? Maybe try to convert from String to number?

I believe QueryableIndexStorageAdapter does the same thing, so at least it's consistent with that. However, it'd be nice (for schema evolution of a column from string -> numeric) for the behavior to change to try to convert the string to a number.

leventov · 2017-07-04T16:42:05Z

+      IncrementalIndexStorageAdapter.EntryHolder currEntry, IncrementalIndex.DimensionDesc desc
+  )
+  {
+    return ZeroFloatColumnSelector.instance();


Same as #3966 (comment).

licl2014 · 2017-11-30T08:39:03Z

@jon-wei @gianm i have a doubt that long/float dimensions will cause high Cardinality easily when we have a group by query.so have we consider this scenario?

gianm · 2017-11-30T22:16:43Z

@licl2014 it could happen, although in that case Druid will do the best it can. If cardinality is not super high it should be fine and go quickly. If cardinality is super high then Druid will spill to disk as needed (assuming disk spilling is enabled).

jon-wei added 3 commits February 22, 2017 15:15

Support ingestion for long/float dimensions

e85330f

Allow non-arrays for key components in indexing type strategy interfaces

1a0ba2d

Add numeric index merge test, fixes

9a54839

jon-wei added the Feature label Feb 23, 2017

jon-wei requested a review from gianm February 23, 2017 02:12

Docs for numeric dims at ingestion

1a69a20

jon-wei force-pushed the ingestion_types branch from cf90c60 to 1a69a20 Compare February 23, 2017 02:14

Remove unused import

4d54625

leventov self-assigned this Feb 25, 2017

gianm reviewed Feb 27, 2017

View reviewed changes

jon-wei added 4 commits February 27, 2017 16:05

Adjust docs, add aggregate on numeric dims tests

21790e4

Merge with master

2c48b16

remove unused imports

3ad56db

Throw exception for bitmap method on numerics

e782d29

jon-wei closed this Feb 28, 2017

jon-wei reopened this Feb 28, 2017

jon-wei added 3 commits February 28, 2017 11:31

Move typed selector creation to DimensionIndexer interface

a21ca01

Merge with master

0ddaa75

unused imports

d225d87

gianm mentioned this pull request Feb 28, 2017

Druid 0.10.0 release notes #3944

Closed

Fix

2f19449

gianm closed this Feb 28, 2017

gianm reopened this Feb 28, 2017

gianm closed this Feb 28, 2017

gianm reopened this Feb 28, 2017

jon-wei added 2 commits February 28, 2017 14:33

Remove unused DimensionSpec from indexer methods, check for dims firs…

d3b5c78

…t in inc index storage adapter

Remove spaces

2505271

jon-wei closed this Mar 1, 2017

jon-wei reopened this Mar 1, 2017

gianm closed this Mar 1, 2017

gianm reopened this Mar 1, 2017

gianm approved these changes Mar 1, 2017

View reviewed changes

fjy merged commit a08660a into apache:master Mar 1, 2017

fjy added this to the 0.10.0 milestone Mar 1, 2017

leventov reviewed Jul 4, 2017

View reviewed changes

jon-wei deleted the ingestion_types branch October 6, 2017 22:22

clambertus unassigned leventov Jul 6, 2018

Conversation

jon-wei commented Feb 23, 2017

Uh oh!

gianm left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jon-wei commented Feb 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gianm commented Feb 28, 2017

Uh oh!

gianm commented Feb 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gianm left a comment

Choose a reason for hiding this comment

Uh oh!

fjy commented Mar 1, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

licl2014 commented Nov 30, 2017

Uh oh!

gianm commented Nov 30, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

jon-wei commented Feb 28, 2017 •

edited

Loading

gianm commented Feb 28, 2017 •

edited

Loading