ability to not rollup at index time, make pre aggregation an option #3020

kaijianding · 2016-05-26T03:11:33Z

here is the Proposal https://groups.google.com/forum/#!msg/druid-development/GE8QtSMdoFo/BhEkLIOjBAAJ

In our case, we need to keep all details instead of rollup duplicate rows, so here is the patch.
We believe there should be no impact to query part if not rollup

In this patch

we add an "rollup" option in IncrementalIndexSchema and a new NoRollupGranularitySpec
add "rollup" option to mergeQueryableIndex() and merge() in IndexMerger. and do not use CombiningIterable in rowMergerFn if not rollup, then all rows will be kept without combine duplicate rows to one row. If not rollup, it is ordered by timestamp instead of timeAndDims
changes to related classes and doc due to the 1,2
in IncrementalIndex, do not directly use facts map, use FactsHolder instead. if rollup, use RollupFactsHolder which just delegates to the original facts map. if no rollup, use PlainFactsHolder, which keeps all duplicate rows in a list as ConcurrentMap < Long, Queue < Pair < TimeAndDims, Integer > > >, the key is timestamp, the value is the list of < timeAndDims,rowNum > pair.
update IncrementalIndexStorageAdapter to use FactsHolder instead of the facts map
add rollup info to SegmentMetadataQuery, also in metadata.drd when persist
benchmark

vogievetsky · 2016-05-26T20:30:21Z

Could you please add the ability to find out if there is rollup in a data source from the segmentMetadata query. This would be very helpful for tools that interface with Druid. Like https://github.com/implydata/pivot and https://github.com/airbnb/caravel

vogievetsky · 2016-05-26T20:31:35Z

Could you please add docs for how to use this feature?

vogievetsky · 2016-05-27T16:07:11Z

Kickass! Thank you for incorporating my comments!

vogievetsky · 2016-05-27T16:17:26Z

docs/content/ingestion/index.md

@@ -71,6 +72,7 @@ An example dataSchema is shown below:
 | dataSource | String | The name of the ingested datasource. Datasources can be thought of as tables. | yes |
 | parser | JSON Object | Specifies how ingested data can be parsed. | yes |
 | metricsSpec | JSON Object array | A list of [aggregators](../querying/aggregations.html). | yes |
+| rollup | Boolean | rollup or not | no (default == true) |


From that I know of the Druid commiter people they will not like the "default to true" settings. Things should always default to false or so they say :-p. Now the 'simple fix' is to change this setting to (say) disableRollup and have it default to false. But I think we should have a conversation about having no rollup be the default in Druid.

'No rollup' is how most DBs out there operate. It would make Druid much easier to evaluate because you would not need to have to think about rollup from day 0.

I would love to get the Druid people @xvrl , @gianm , @cheddar , @fjy , @himanshug , @drcrallen and anyone I am forgetting to comment on possibly making Druid have no rollup by default.

This will be a fairly large change to have no rollup as default and require updating the version number. FWIW, I think we should keep rollup on by default and re-evaluate for 0.10.0

agree with @fjy, I am in favor of keeping rollup on by default at least for 0.9

nishantmonu51 · 2016-05-27T18:45:47Z

@kaijianding: Not a blocker for this PR, just out of curiousity, did you ran any benchmarks with rollup disabled ?
would be great to see performance comparison for ingestion, persisting & merging with and without rollup. you can probably use IndexMargeBenchmark & OnheapIncrementalIndexBenchmark.

kaijianding · 2016-05-28T19:02:44Z

Will post results here when I done the benchmarks @nishantmonu51

fjy · 2016-06-14T17:13:36Z

@kaijianding did you get a chance to get the benchmarks?

fjy · 2016-06-22T22:13:36Z

@kaijianding

kaijianding · 2016-06-23T16:47:14Z

Will get benchmark by end of this week, @fjy

jon-wei · 2016-06-25T01:08:34Z

@kaijianding I looked at the core changes in IncrementalIndex today, seems good so far, will review the rest as well

kaijianding · 2016-06-26T15:17:50Z

master

Benchmark                           (rowsPerSegment)  (schema)  Mode  Cnt       Score       Error  Units
IncrementalIndexReadBenchmark.read            750000     basic  avgt   25  546006.587 ± 38764.187  us/op
IndexIngestionBenchmark.addRows               75000      basic  avgt   25  710651.752 ± 66651.048  us/op
IndexPersistBenchmark.persist                 75000      basic  avgt   25  2093374.955 ± 146172.738  us/op
IndexPersistBenchmark.persistV9               75000      basic  avgt   25  1757484.706 ±  83394.681  us/op
Benchmark                    (numSegments)  (rowsPerSegment)  (schema)  Mode  Cnt        Score        Error  Units
IndexMergeBenchmark.merge                5             75000     basic  avgt   25  8662055.099 ± 178077.777  us/op
IndexMergeBenchmark.mergeV9              5             75000     basic  avgt   25  7122331.471 ±  83495.704  us/op


rollup with patch

Benchmark                           (rowsPerSegment)  (schema)  Mode  Cnt       Score       Error  Units
IncrementalIndexReadBenchmark.read            750000     basic  avgt   25  557579.110 ± 20328.083  us/op
IndexIngestionBenchmark.addRows               75000      basic  avgt   25  714533.366 ± 74060.007  us/op
IndexPersistBenchmark.persist                 75000      basic  avgt   25  2160473.511 ± 181774.284  us/op
IndexPersistBenchmark.persistV9               75000      basic  avgt   25  1821372.750 ±  90223.928  us/op
Benchmark                    (numSegments)  (rowsPerSegment)  (schema)  Mode  Cnt        Score        Error  Units
IndexMergeBenchmark.merge                5             75000     basic  avgt   25  8922968.176 ± 243350.019  us/op
IndexMergeBenchmark.mergeV9              5             75000     basic  avgt   25  7418237.936 ± 119332.012  us/op

no-rollup with patch

Benchmark                           (rowsPerSegment)  (schema)  Mode  Cnt       Score       Error  Units
IncrementalIndexReadBenchmark.read            750000     basic  avgt   25  612151.425 ± 15653.577  us/op
IndexIngestionBenchmark.addRows               75000      basic  avgt   25  734294.010 ± 46828.218  us/op
IndexPersistBenchmark.persist                 75000      basic  avgt   25  2111290.873 ± 197970.723  us/op
IndexPersistBenchmark.persistV9               75000      basic  avgt   25  1807539.558 ± 115874.940  us/op
Benchmark                    (numSegments)  (rowsPerSegment)  (schema)  Mode  Cnt        Score        Error  Units
IndexMergeBenchmark.merge                5             75000     basic  avgt   25  8823597.932 ± 259610.525  us/op
IndexMergeBenchmark.mergeV9              5             75000     basic  avgt   25  7212103.929 ± 144478.531  us/op

here is the benchmark, @fjy @nishantmonu51

fjy · 2016-06-27T17:55:22Z

@kaijianding

How many iterations did you run? There seems to be a 5-6% slowdown for ingestion, which is significant. There is also a wide error range. Does running more iterations help reduce this error range? If there is actually a 5-6% slowdown in performance, I think it is worth investigating why the slowdown is occurring as ideally there should be no performance impact when rollup is turned on.

kaijianding · 2016-06-28T17:02:07Z

master

Benchmark                           (rowsPerSegment)  (schema)  Mode  Cnt       Score       Error  Units
IncrementalIndexReadBenchmark.read            750000     basic  avgt  500  451675.864 ± 1083.005  us/op
IndexIngestionBenchmark.addRows               75000      basic  avgt  200  564279.494 ± 6781.083  us/op
IndexPersistBenchmark.persist                 75000      basic  avgt  100  1490063.885 ± 32357.405  us/op
IndexPersistBenchmark.persistV9               75000      basic  avgt  100  1298518.894 ± 22939.044  us/op
Benchmark                    (numSegments)  (rowsPerSegment)  (schema)  Mode  Cnt        Score        Error  Units
IndexMergeBenchmark.merge                5             75000     basic  avgt  100  6203247.612 ± 47913.348  us/op
IndexMergeBenchmark.mergeV9              5             75000     basic  avgt  100  5243835.387 ± 29471.834  us/op

rollup with patch

Benchmark                           (rowsPerSegment)  (schema)  Mode  Cnt       Score       Error  Units
IncrementalIndexReadBenchmark.read            750000     basic  avgt  500  454189.336 ± 855.910  us/op
IndexIngestionBenchmark.addRows               75000      basic  avgt  200  560886.776 ± 7041.057  us/op
IndexPersistBenchmark.persist                 75000      basic  avgt  100  1508987.804 ± 34927.774  us/op
IndexPersistBenchmark.persistV9               75000      basic  avgt  100  1301953.483 ± 24832.039  us/op
Benchmark                    (numSegments)  (rowsPerSegment)  (schema)  Mode  Cnt        Score        Error  Units
IndexMergeBenchmark.merge                5             75000     basic  avgt  100  6334795.617 ± 46532.740  us/op
IndexMergeBenchmark.mergeV9              5             75000     basic  avgt  100  5256236.805 ± 31898.007  us/op

no-rollup with patch

Benchmark                           (rowsPerSegment)  (schema)  Mode  Cnt       Score       Error  Units
NoRollupIncrementalIndexReadBenchmark.read           750000     basic  avgt  500  534176.049 ± 1379.058  us/op
NoRollupIndexIngestionBenchmark.addRows              75000      basic  avgt  200  570859.946 ± 3950.365  us/op
NoRollupIndexPersistBenchmark.persist                75000      basic  avgt  100  1507740.670 ± 32116.040  us/op
NoRollupIndexPersistBenchmark.persistV9              75000      basic  avgt  100  1315232.369 ± 25098.084  us/op
Benchmark                    (numSegments)  (rowsPerSegment)  (schema)  Mode  Cnt        Score        Error  Units
NoRollupIndexMergeBenchmark.merge                5             75000     basic  avgt  100  6055157.853 ± 47004.241  us/op
NoRollupIndexMergeBenchmark.mergeV9              5             75000     basic  avgt  100  5068509.238 ± 32577.070  us/op

here is the latest benchmark after running with more iterations on a more powerful machine and with slightly code modification, @fjy

fjy · 2016-06-29T22:08:56Z

@kaijianding cool! looks much better

fjy · 2016-06-29T22:14:26Z

docs/content/ingestion/index.md

@@ -173,6 +175,11 @@ handle all formatting decisions on their own, without using the ParseSpec.
 | dimensionExclusions | JSON String array | The names of dimensions to exclude from ingestion. | no (default == [] |
 | spatialDimensions | JSON Object array | An array of [spatial dimensions](../development/geo.html) | no (default == [] |

+## Rollup


can we move this section and instead have this info in the table that first describes the rollup setting?

@kaijianding can we remove this section and have this info in the description of rollup in the table above?

kaijianding · 2016-07-22T06:27:03Z

I modified the code according to your comments, and rebased it, do you have more comments? @fjy

fjy · 2016-07-25T22:46:43Z

@kaijianding can you fix the merge conflicts?

kaijianding · 2016-07-26T03:22:53Z

fixed. @fjy

fjy · 2016-07-27T01:08:57Z

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

+    }
+
+    @Override
+    public Integer getRowIndexForRollup(TimeAndDims key)


can we call this getPriorIndex()?

fjy · 2016-07-27T01:20:52Z

@kaijianding mostly looks good, I have a few more minor comments

@jon-wei @nishantmonu51 can u guys take a look ?

kaijianding · 2016-07-27T02:10:56Z

renamed. @fjy

jon-wei · 2016-07-27T19:34:27Z

@fjy sure I'll take a look at this today

jon-wei · 2016-07-27T23:04:49Z

docs/content/ingestion/tasks.md

@@ -159,14 +159,18 @@ Append tasks append a list of segments together into a single segment (one after

 ### Merge Task

-Merge tasks merge a list of segments together. Any common timestamps are merged. The grammar is:
+Merge tasks merge a list of segments together. Any common timestamps are merged.
+If rollup is disabled as part of ingestion, common timestamps are not merged and rows are reorderded by their timestamp.


"reorderded" should be "reordered"

jon-wei · 2016-07-28T00:40:11Z

looks good to me aside from my last two comments

fjy · 2016-07-29T22:19:08Z

👍

fjy · 2016-07-31T01:46:48Z

@kaijianding can you run benchmarks again just to make sure that nothing has been impacted with all the changes during code review?

jon-wei · 2016-08-01T20:35:51Z

👍 if new benchmarks look good

kaijianding · 2016-08-02T09:18:23Z

master

Benchmark                                      (rowsPerSegment)  (schema)  Mode  Cnt       Score      Error  Units
IncrementalIndexReadBenchmark.read                     750000    basic  avgt  500  385330.680 ±  891.106  us/op
IncrementalIndexReadBenchmark.readWithFilters          750000    basic  avgt  500  502956.956 ± 1054.732  us/op
IndexIngestionBenchmark.addRows                        75000     basic  avgt  200  501676.186 ± 2953.806  us/op
IndexPersistBenchmark.persist                          75000     basic  avgt  100  1346216.134 ± 14633.695  us/op
IndexPersistBenchmark.persistV9                        75000     basic  avgt  100  1179403.111 ± 12030.273  us/op
Benchmark                    (numSegments)     (rowsPerSegment)  (schema)  Mode  Cnt       Score      Error  Units
IndexMergeBenchmark.merge                5             75000     basic  avgt  100  5835544.731 ± 22592.354  us/op
IndexMergeBenchmark.mergeV9              5             75000     basic  avgt  100  5033092.133 ± 13562.767  us/op

rollup with patch

Benchmark                                   (rollup)  (rowsPerSegment)  (schema)  Mode  Cnt       Score      Error  Units
IncrementalIndexReadBenchmark.read              true            750000     basic  avgt  500  379315.134 ± 885.494  us/op
IncrementalIndexReadBenchmark.readWithFilters   true            750000     basic  avgt  500  400021.287 ± 843.226  us/op
IndexIngestionBenchmark.addRows                 true             75000     basic  avgt  100  509449.271 ± 5378.047  us/op
IndexPersistBenchmark.persist                   true             75000     basic  avgt  100  1329498.943 ± 14548.436  us/op
IndexPersistBenchmark.persistV9                 true             75000     basic  avgt  100  1182184.226 ± 11937.399  us/op
Benchmark                    (numSegments)  (rollup)  (rowsPerSegment)  (schema)  Mode  Cnt        Score       Error  Units
IndexMergeBenchmark.merge                5      true             75000     basic  avgt  100  5739894.613 ± 15663.028  us/op
IndexMergeBenchmark.mergeV9              5      true             75000     basic  avgt  100  5019952.001 ± 10244.254  us/op

norollup

IncrementalIndexReadBenchmark.read             false            750000     basic  avgt  500  459174.614 ± 642.678  us/op
IncrementalIndexReadBenchmark.readWithFilters  false            750000     basic  avgt  500  465450.957 ± 452.000  us/op
IndexIngestionBenchmark.addRows                false             75000     basic  avgt  100  618939.096 ± 17863.195  us/op
IndexPersistBenchmark.persist                  false             75000     basic  avgt  100  1330675.790 ± 13748.631  us/op
IndexPersistBenchmark.persistV9                false             75000     basic  avgt  100  1176238.798 ± 12016.404  us/op
IndexMergeBenchmark.merge                5     false             75000     basic  avgt  100  5636242.340 ± 18749.393  us/op
IndexMergeBenchmark.mergeV9              5     false             75000     basic  avgt  100  4929684.534 ± 16980.737  us/op

here is the benchmark. @fjy @jon-wei
Basically, there is no change in rollup scenario. no rollup scenario is 20% slower in read and addRows.

BTW, I noticed that the jmh framework would call each benchmark method 1~2 times each iteration. In this way, the second call, it is not called in clean environment. Fg, IndexIngestionBenchmark.addRows test, the second call will cause incIndex to add more rows than expected.

fjy added the Feature label May 26, 2016

fjy added this to the 0.9.2 milestone May 26, 2016

vogievetsky reviewed May 27, 2016
View reviewed changes

kaijianding force-pushed the no-rollup branch 2 times, most recently from 0231651 to aff1f1b Compare May 29, 2016 12:23

kaijianding force-pushed the no-rollup branch from f946101 to 87c0a89 Compare June 26, 2016 16:50

kaijianding force-pushed the no-rollup branch 4 times, most recently from 5adf9ef to b941b38 Compare June 29, 2016 18:12

fjy reviewed Jun 29, 2016
View reviewed changes

kaijianding force-pushed the no-rollup branch 4 times, most recently from b5ec414 to 7e75988 Compare July 22, 2016 05:49

ability to not rollup at index time, make pre aggregation an option

b546ffd

kaijianding force-pushed the no-rollup branch from 7e75988 to b546ffd Compare July 26, 2016 02:55

fjy reviewed Jul 27, 2016
View reviewed changes

rename getRowIndexForRollup to getPriorIndex

94044d0

jon-wei reviewed Jul 27, 2016
View reviewed changes

fix doc misspelling

ca8f802

test query using no-rollup indexes

17ffaaf

fix benchmark fail due to jmh bug

2e82549

fjy merged commit 50d52a2 into apache:master Aug 2, 2016

kaijianding deleted the no-rollup branch August 3, 2016 02:21

gianm mentioned this pull request Sep 29, 2016

Druid 0.9.2 release notes #3503

Closed

seoeun25 pushed a commit to seoeun25/incubator-druid that referenced this pull request Jan 10, 2020

apache#3020 Support cuboid in index viewer

f4e6dd3

seoeun25 pushed a commit to seoeun25/incubator-druid that referenced this pull request Feb 25, 2022

apache#3020 Support cuboid in index viewer

5cfb989

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ability to not rollup at index time, make pre aggregation an option #3020

ability to not rollup at index time, make pre aggregation an option #3020

kaijianding commented May 26, 2016 •

edited

Loading

vogievetsky commented May 26, 2016

vogievetsky commented May 26, 2016

vogievetsky commented May 27, 2016

vogievetsky May 27, 2016

fjy May 27, 2016

nishantmonu51 May 27, 2016

nishantmonu51 commented May 27, 2016

kaijianding commented May 28, 2016

fjy commented Jun 14, 2016

fjy commented Jun 22, 2016

kaijianding commented Jun 23, 2016

jon-wei commented Jun 25, 2016

kaijianding commented Jun 26, 2016 •

edited

Loading

fjy commented Jun 27, 2016 •

edited

Loading

kaijianding commented Jun 28, 2016 •

edited

Loading

fjy commented Jun 29, 2016

fjy Jun 29, 2016

fjy Jul 1, 2016

kaijianding commented Jul 22, 2016

fjy commented Jul 25, 2016

kaijianding commented Jul 26, 2016

fjy Jul 27, 2016

fjy commented Jul 27, 2016

kaijianding commented Jul 27, 2016

jon-wei commented Jul 27, 2016

jon-wei Jul 27, 2016

jon-wei commented Jul 28, 2016

fjy commented Jul 29, 2016

fjy commented Jul 31, 2016

jon-wei commented Aug 1, 2016

kaijianding commented Aug 2, 2016

ability to not rollup at index time, make pre aggregation an option #3020

ability to not rollup at index time, make pre aggregation an option #3020

Conversation

kaijianding commented May 26, 2016 • edited Loading

vogievetsky commented May 26, 2016

vogievetsky commented May 26, 2016

vogievetsky commented May 27, 2016

vogievetsky May 27, 2016

Choose a reason for hiding this comment

fjy May 27, 2016

Choose a reason for hiding this comment

nishantmonu51 May 27, 2016

Choose a reason for hiding this comment

nishantmonu51 commented May 27, 2016

kaijianding commented May 28, 2016

fjy commented Jun 14, 2016

fjy commented Jun 22, 2016

kaijianding commented Jun 23, 2016

jon-wei commented Jun 25, 2016

kaijianding commented Jun 26, 2016 • edited Loading

fjy commented Jun 27, 2016 • edited Loading

kaijianding commented Jun 28, 2016 • edited Loading

fjy commented Jun 29, 2016

fjy Jun 29, 2016

Choose a reason for hiding this comment

fjy Jul 1, 2016

Choose a reason for hiding this comment

kaijianding commented Jul 22, 2016

fjy commented Jul 25, 2016

kaijianding commented Jul 26, 2016

fjy Jul 27, 2016

Choose a reason for hiding this comment

fjy commented Jul 27, 2016

kaijianding commented Jul 27, 2016

jon-wei commented Jul 27, 2016

jon-wei Jul 27, 2016

Choose a reason for hiding this comment

jon-wei commented Jul 28, 2016

fjy commented Jul 29, 2016

fjy commented Jul 31, 2016

jon-wei commented Aug 1, 2016

kaijianding commented Aug 2, 2016

kaijianding commented May 26, 2016 •

edited

Loading

kaijianding commented Jun 26, 2016 •

edited

Loading

fjy commented Jun 27, 2016 •

edited

Loading

kaijianding commented Jun 28, 2016 •

edited

Loading