Skip to content

Add HashVectorGrouper based on MemoryOpenHashTable. - #9314

Merged
jihoonson merged 6 commits into
apache:masterfrom
gianm:groupby-hvg
Feb 6, 2020
Merged

jihoonson merged 6 commits into
apache:masterfrom
gianm:groupby-hvg

Conversation

@gianm

@gianm gianm commented Feb 5, 2020 •

Copy link
Copy Markdown
Contributor

This causes vectorized hash-based groupBys to use MemoryOpenHashTable
instead of ByteBufferHashTable (see #9308).

Additional supporting changes:

  1. Modifies VectorGrouper interface to use Memory instead of ByteBuffers.
  2. Modifies BufferArrayGrouper to match the new VectorGrouper interface.
  3. Removes "implements VectorGrouper" from BufferHashGrouper.

Benchmarks:

master @ a085685182d62e5dd1b716f1bbb9bcbbaeb1c661

Benchmark              (query)  (rowsPerSegment)  (vectorize)  Mode  Cnt    Score   Error  Units
SqlBenchmark.querySql       10           5000000        force  avgt   25  683.263 ± 8.382  ms/op
SqlBenchmark.querySql       11           5000000        force  avgt   25  691.823 ± 7.979  ms/op
SqlBenchmark.querySql       12           5000000        force  avgt   25   34.977 ± 0.842  ms/op
SqlBenchmark.querySql       13           5000000        force  avgt   25   42.408 ± 0.984  ms/op
SqlBenchmark.querySql       14           5000000        force  avgt   25   98.015 ± 0.887  ms/op
SqlBenchmark.querySql       15           5000000        force  avgt   25  449.379 ± 6.541  ms/op
SqlBenchmark.querySql       16           5000000        force  avgt   25  519.083 ± 5.115  ms/op
SqlBenchmark.querySql       17           5000000        force  avgt   25  456.992 ± 4.015  ms/op
SqlBenchmark.querySql       18           5000000        force  avgt   25  534.258 ± 8.832  ms/op

groupby-hvg @ 9fdc4bcf8e6dd6896a5fe687145e5ea0f1c6a709

Benchmark              (query)  (rowsPerSegment)  (vectorize)  Mode  Cnt    Score   Error  Units
SqlBenchmark.querySql       10           5000000        force  avgt   25  450.345 ± 6.428  ms/op
SqlBenchmark.querySql       11           5000000        force  avgt   25  473.163 ± 4.918  ms/op
SqlBenchmark.querySql       12           5000000        force  avgt   25   32.281 ± 0.566  ms/op
SqlBenchmark.querySql       13           5000000        force  avgt   25   39.259 ± 1.300  ms/op
SqlBenchmark.querySql       14           5000000        force  avgt   25   91.553 ± 0.892  ms/op
SqlBenchmark.querySql       15           5000000        force  avgt   25   85.417 ± 0.670  ms/op
SqlBenchmark.querySql       16           5000000        force  avgt   25  158.197 ± 1.458  ms/op
SqlBenchmark.querySql       17           5000000        force  avgt   25  103.445 ± 0.872  ms/op
SqlBenchmark.querySql       18           5000000        force  avgt   25  175.661 ± 1.859  ms/op

The largest improvements are on queries 10 and 11 (~50%) and queries 15–18 (3–5x). Queries 12–14 look very slightly faster or maybe unchanged.

Queries 10 & 11 are groupBys on two strings and would use hash-based grouping.

Queries 12–14 are groupBys on one string and would use array-based grouping. It makes sense that they haven't changed much, since they wouldn't use hash-based grouping. The changes to the array-based grouping code were more minimal.

Queries 15–18 are groupBys on one long column and would use hash-based grouping.

Additional supporting changes:

1) Modifies VectorGrouper interface to use Memory instead of ByteBuffers.
2) Modifies BufferArrayGrouper to match the new VectorGrouper interface.
3) Removes "implements VectorGrouper" from BufferHashGrouper.
@lgtm-com

lgtm-com Bot commented Feb 5, 2020

Copy link
Copy Markdown

This pull request introduces 1 alert when merging 672fa2d into 54bc4dc - view on LGTM.com

new alerts:

  • 1 for Result of multiplication cast to wider type

@jihoonson jihoonson left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 after CI.

@lgtm-com

lgtm-com Bot commented Feb 5, 2020

Copy link
Copy Markdown

This pull request introduces 1 alert when merging 2ce825f into 54bc4dc - view on LGTM.com

new alerts:

  • 1 for Result of multiplication cast to wider type

@jihoonson

Copy link
Copy Markdown
Contributor

Please check the LGTM warning.

@gianm

gianm commented Feb 6, 2020

Copy link
Copy Markdown
Contributor Author

I just looked at it. It's complaining that keySpace.getInt(i * Integer.BYTES) accepts a long and in theory i * Integer.BYTES could overflow. It's not very plausible: it would only happen if the vector size was crazy huge. I added a check to the start of the method that will detect this and block it. I'm not sure if LGTM will be smart enough to understand it, but it means the issue isn't real.

@lgtm-com

lgtm-com Bot commented Feb 6, 2020

Copy link
Copy Markdown

This pull request introduces 1 alert when merging 78bc768 into 2e1dbe5 - view on LGTM.com

new alerts:

  • 1 for Result of multiplication cast to wider type

@jihoonson jihoonson left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The latest change is enough to prevent the overflow.

@jihoonson
jihoonson merged commit 0aa7a2a into apache:master Feb 6, 2020
@gianm
gianm deleted the groupby-hvg branch February 7, 2020 00:52
@jihoonson jihoonson added this to the 0.18.0 milestone Mar 26, 2020
riovic918data pushed a commit to riovic918data/druid that referenced this pull request Jun 12, 2026
* Add HashVectorGrouper based on MemoryOpenHashTable.

Additional supporting changes:

1) Modifies VectorGrouper interface to use Memory instead of ByteBuffers.
2) Modifies BufferArrayGrouper to match the new VectorGrouper interface.
3) Removes "implements VectorGrouper" from BufferHashGrouper.

* Fix comment.

* Fix another comment.

* Remove unused stuff.

* Include hoisted bounds checks.

* Checks against too-large keySpaces.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants