Allow IncrementalIndex to store Long/Float dimensions #2263

jon-wei · 2016-01-14T00:59:28Z

This PR updates IncrementalIndex such that it can ingest and store Long/Float dimensions in addition to Strings. It is intended as a step towards the larger goals being discussed in the following topic:

https://groups.google.com/forum/#!topic/druid-development/obtfNJnXPDg/discussion

DimDim is now a generic class instead of assuming String values. The TimeAndDims grouping key class now stores dimension values as Comparable[] instead of String[].
The PR does not change the DimensionSelector or *Adapter interfaces to support non-String types, so query/index persist behavior is unchanged, nor does this PR provide a way for the user to specify non-String dimension types in the ingestion schema.
Schema-less dimension type discovery is not enabled in the PR (newly discovered dims are assumed to be Strings).
The PR includes a JMH benchmark that adds rows with Long/Float/String columns to an IncrementalIndex.

I ran the row-adding benchmark with type discovery enabled for testing purposes and the numbers are shown below:

java -jar target/benchmarks.jar IncrementalIndexAddRowsBenchmark -f 1 -i 20 -wi 10

With patch, non-String dims:

Run complete. Total time: 00:08:05

Benchmark                                       Mode  Cnt   Score   Error  Units
IncrementalIndexAddRowsBenchmark.normalFloats   avgt   20  18.918 ± 3.063  us/op
IncrementalIndexAddRowsBenchmark.normalLongs    avgt   20  10.783 ± 0.394  us/op
IncrementalIndexAddRowsBenchmark.normalStrings  avgt   20  28.218 ± 2.513  us/op

Without patch, String dims only:

Run complete. Total time: 00:09:34

Benchmark                                       Mode  Cnt   Score   Error  Units
IncrementalIndexAddRowsBenchmark.normalFloats   avgt   20  19.977 ± 2.803  us/op
IncrementalIndexAddRowsBenchmark.normalLongs    avgt   20  20.520 ± 3.298  us/op
IncrementalIndexAddRowsBenchmark.normalStrings  avgt   20  29.656 ± 3.261  us/op

nishantmonu51 · 2016-01-14T07:51:26Z

processing/src/main/java/io/druid/segment/incremental/IncrementalIndex.java

@@ -74,6 +76,80 @@
 {
  private volatile DateTime maxIngestedEventTime;

+  private static final Map<String, ValueType> typeMap = ImmutableMap.<String, ValueType>builder()


static variable name in UPPERCASE.

@nishantmonu51 addressed

pjain1 · 2016-01-19T16:29:07Z

processing/src/main/java/io/druid/segment/incremental/IncrementalIndex.java

@@ -74,6 +77,76 @@
 {
  private volatile DateTime maxIngestedEventTime;

+  private static final Map<String, ValueType> TYPE_MAP = ImmutableMap.<String, ValueType>builder()
+      .put("Long[]", ValueType.LONG)


Just curious, do we have any parser in Druid currently that returns [] type for multi-valued dimension ?

@pjain1 Do you mean if the multi-value array can have subitems with an array[] type? If so, I don't think that will occur, the values would be single numerics or Strings for the JSON parsers and all single strings for the delimited parsers, I believe.

then why there is a need for [] types here as TYPE_MAP is used like TYPE_MAP.get(singleVal.getClass().getSimpleName()) where singleValue as you said "would be single numerics or Strings for the JSON parsers and all single strings for the delimited parsers, I believe."

@pjain1 It's not strictly necessary now, I think I had it in there from an older implementation where I used the TYPE_MAP to get a ValueType from a Comparable[].

I kept it there in case, but I can remove it if needed.

@jon-wei I was just curious to know why it was there. No need to remove if it does not hurt anything.

jon-wei · 2016-01-21T23:47:57Z

closed for now to rebase to master, will reopen

drcrallen · 2016-01-21T23:54:53Z

dont' force-push your branch until the PR is open or it will render this unable to re-open

jon-wei · 2016-01-22T21:08:13Z

@drcrallen yeah, forgot about that yesterday =\

I was able to re-open it though by doing a "git reset --hard" to the SHA at the time the PR was closed

fjy · 2016-02-05T23:33:35Z

@jon-wei we need to rebase this

jon-wei · 2016-02-06T00:53:16Z

Updated this to rebase on top of changes introduced by:

#2085

The PR above changed TimeAndDims to use a dictionary encoded int[] array instead of String[]; this patch uses dictionary encoding for Long/Float values as well (unnecessary).

I plan to change TimeAndDims to use Comparable[] instead of int[] in a later PR, still WIP in my fork:
https://github.com/jon-wei/druid/tree/flex_dims_pt2

Strings will store a dictionary encoding in TimeAndDims as in PR #2085, numerics will store their values directly in TimeAndDims.

fjy · 2016-02-08T23:52:46Z

benchmarks/src/main/java/io/druid/benchmark/IncrementalIndexAddRowsBenchmark.java

@@ -0,0 +1,178 @@
+package io.druid.benchmark;


@fjy added license header

fjy · 2016-02-09T00:02:44Z

👍

nishantmonu51 · 2016-02-09T15:29:13Z

processing/src/main/java/io/druid/segment/incremental/IncrementalIndexAdapter.java

-                    dims[dimIndex][i] = dimLookups[dimIndex].idToIndex(dimValues[dimIndex][i]);
+                    dims[dimIndex][i] = sortedDimLookups[dimIndex].getSortedIdFromUnsortedId(dimValues[dimIndex][i]);
+                    //TODO: in later PR, Rowboat will use Comparable[][] instead of int[][]
+                    // Can remove dictionary encoding for numeric dims then.


please file an issue for this todo.

nishantmonu51 · 2016-02-09T15:38:35Z

What is the behavior of null for Long/Float dimensions ?
Will it throw parseException or successfully ingest null values and treat them as null ?

can we also add tests for null values in Long/Float dimension to make sure it behaves as expected.

nishantmonu51 · 2016-02-09T15:39:13Z

Also need to add docs for the added functionality.

jon-wei · 2016-02-09T20:52:54Z

@nishantmonu51

RE: null handling, the nulls are being converted to 0. It's not in this PR, but a follow on PR that I am finishing up:

https://github.com/jon-wei/druid/blob/5bffacec5772718bcb948b81e6ace4192d940396/processing/src/main/java/io/druid/segment/IndexMerger.java#L1432

That PR reuses the existing single-value long/float column formats for the numeric dims, representing null without changes to the storage format (need some mechanism to distinguish null from normal values) or using dictionary encoding (have a dictionary entry for null) seems problematic. Throwing an exception sounds okay to me too (maybe let user configure? choice of "throw exception" or "convert null to 0")

RE: docs and tests, this open PR doesn't actually enable the use of long/float dimensions, it's just preparing IncrementalIndex for later changes:

It doesn't provide a way to specify dimension typing in the schema
No query/filter/extraction fn, etc. support for long/floats yet

I address those points and have tests in the follow-on patch (not ready yet), I think it would make more sense to include docs in the later PR when the numeric dims are actually usable

jon-wei · 2016-02-09T20:53:24Z

@fjy @nishantmonu51 will file issues for TODOs

nishantmonu51 · 2016-02-11T15:07:53Z

+1, I think this is ready to be merged. docs can be added in the follow up PR.

fjy · 2016-02-19T21:12:52Z

@jon-wei do you mind resolving merge conflicts?

jon-wei · 2016-02-24T22:40:01Z

@fjy rebased

Allow IncrementalIndex to store Long/Float dimensions

jon-wei force-pushed the flex_dims3 branch from 2d9bd77 to 9a260a3 Compare January 14, 2016 02:06

nishantmonu51 reviewed Jan 14, 2016
View reviewed changes

jon-wei force-pushed the flex_dims3 branch 4 times, most recently from 94c8b13 to 7de6bdc Compare January 18, 2016 21:29

pjain1 reviewed Jan 19, 2016
View reviewed changes

jon-wei mentioned this pull request Jan 19, 2016

[Proposal] More flexible dimension types #2292

Closed

jon-wei force-pushed the flex_dims3 branch from 7de6bdc to 542a7f8 Compare January 20, 2016 21:40

jon-wei closed this Jan 21, 2016

jon-wei reopened this Jan 21, 2016

jon-wei force-pushed the flex_dims3 branch from 542a7f8 to f5843fc Compare January 21, 2016 21:34

jon-wei closed this Jan 21, 2016

jon-wei reopened this Jan 22, 2016

jon-wei closed this Jan 22, 2016

jon-wei force-pushed the flex_dims3 branch from f5843fc to 3f99811 Compare January 22, 2016 00:16

jon-wei reopened this Jan 22, 2016

jon-wei force-pushed the flex_dims3 branch from 942996b to c3204a0 Compare January 22, 2016 01:25

jon-wei force-pushed the flex_dims3 branch 2 times, most recently from 6850331 to 7a92273 Compare February 6, 2016 00:14

fjy added this to the 0.9.1 milestone Feb 6, 2016

fjy added the Improvement label Feb 6, 2016

gianm closed this Feb 6, 2016

gianm reopened this Feb 6, 2016

jon-wei force-pushed the flex_dims3 branch from 7a92273 to 8f734b9 Compare February 6, 2016 00:44

jon-wei force-pushed the flex_dims3 branch 3 times, most recently from f2aa86d to 8e51849 Compare February 6, 2016 01:55

fjy reviewed Feb 8, 2016
View reviewed changes

nishantmonu51 reviewed Feb 9, 2016
View reviewed changes

jon-wei force-pushed the flex_dims3 branch from 8e51849 to 528533c Compare February 9, 2016 20:55

jon-wei mentioned this pull request Feb 11, 2016

Add support for Long and Float numeric dimensions (without indexing) #2442

Closed

jon-wei added the Discuss label Feb 11, 2016

navis mentioned this pull request Feb 15, 2016

Consolidate argument type of extraction function #2471

Closed

jon-wei force-pushed the flex_dims3 branch from 528533c to 1960634 Compare February 24, 2016 21:34

Allow IncrementalIndex to store Long/Float dimensions

c17ce02

jon-wei force-pushed the flex_dims3 branch from 1960634 to c17ce02 Compare February 24, 2016 21:52

fjy added a commit that referenced this pull request Feb 26, 2016

Merge pull request #2263 from jon-wei/flex_dims3

29d29ba

Allow IncrementalIndex to store Long/Float dimensions

fjy merged commit 29d29ba into apache:master Feb 26, 2016

navis mentioned this pull request Apr 1, 2016

Null string is encoded as "null" in incremental index #2765

Merged

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow IncrementalIndex to store Long/Float dimensions #2263

Allow IncrementalIndex to store Long/Float dimensions #2263

jon-wei commented Jan 14, 2016

nishantmonu51 Jan 14, 2016

jon-wei Jan 14, 2016

pjain1 Jan 19, 2016

jon-wei Jan 20, 2016

pjain1 Jan 20, 2016

jon-wei Jan 20, 2016

pjain1 Jan 24, 2016

jon-wei commented Jan 21, 2016

drcrallen commented Jan 21, 2016

jon-wei commented Jan 22, 2016

fjy commented Feb 5, 2016

jon-wei commented Feb 6, 2016

fjy Feb 8, 2016

jon-wei Feb 9, 2016

fjy commented Feb 9, 2016

nishantmonu51 Feb 9, 2016

nishantmonu51 commented Feb 9, 2016

nishantmonu51 commented Feb 9, 2016

jon-wei commented Feb 9, 2016

jon-wei commented Feb 9, 2016

nishantmonu51 commented Feb 11, 2016

fjy commented Feb 19, 2016

jon-wei commented Feb 24, 2016

Allow IncrementalIndex to store Long/Float dimensions #2263

Allow IncrementalIndex to store Long/Float dimensions #2263

Conversation

jon-wei commented Jan 14, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jon-wei commented Jan 21, 2016

drcrallen commented Jan 21, 2016

jon-wei commented Jan 22, 2016

fjy commented Feb 5, 2016

jon-wei commented Feb 6, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fjy commented Feb 9, 2016

Choose a reason for hiding this comment

nishantmonu51 commented Feb 9, 2016

nishantmonu51 commented Feb 9, 2016

jon-wei commented Feb 9, 2016

jon-wei commented Feb 9, 2016

nishantmonu51 commented Feb 11, 2016

fjy commented Feb 19, 2016

jon-wei commented Feb 24, 2016