Improve BenchmarkPartitionedOutputOperator#12234
Conversation
lukasz-stec
force-pushed
the
ls/014-poo-benchmark-docs
branch
from
2c46d6a to
6f9558c
Compare
lukasz-stec
changed the title
There was a problem hiding this comment.
nit: that should be separate commit
There was a problem hiding this comment.
nit: it would be better to use composition instead of overriding createPage. Right now you pass some arguments to TestType, but you also optionally override a method. With composition, you could reuse createRandomDictionaryPage rather than override it every time. Composition is also cleaner and more consistent than inheritance
There was a problem hiding this comment.
makes sense. I refactored it to use new PageGenerator interface
There was a problem hiding this comment.
this should go to commit that adds row-wise benchmark. BTW: do you know if it works?
There was a problem hiding this comment.
there is no explicit row-wise benchmark. It has to be set up manually by choosing positionCount ~= partitionCount.
And yes, row-wise code path pollution works, it has visible influence on benchmark results:
arm
### without row-wise pollution
Benchmark (channelCount) (enableCompression) (nullRate) (partitionCount) (positionCount) (type) Mode Cnt Score Error Units
BenchmarkPartitionedOutputOperator.addPage 2 false 0 512 512 BIGINT avgt 10 276.219 ± 3.432 ms/op
### with row-wise pollution
Benchmark (channelCount) (enableCompression) (nullRate) (partitionCount) (positionCount) (type) Mode Cnt Score Error Units
BenchmarkPartitionedOutputOperator.addPage 2 false 0 512 512 BIGINT avgt 10 352.991 ± 9.117 ms/op```
Use PageGenerator composition inside TestType instead of overriding createPage method. It makes the intent clearer, especially if the PageGenerators are re-used.
Extend BenchmarkPartitionedOutputOperator.pollute with row-wise code path pollution
Introduce a consistent TestType naming convention. Add descriptions to the TestType cases.
lukasz-stec
force-pushed
the
ls/014-poo-benchmark-docs
branch
from
6f9558c to
8e75abd
Compare
lukasz-stec
left a comment
lukasz-stec
left a comment
There was a problem hiding this comment.
as suggested I refactored the benchmark TestType to use page generation via composition over overriding the createPage method
There was a problem hiding this comment.
makes sense. I refactored it to use new PageGenerator interface
There was a problem hiding this comment.
there is no explicit row-wise benchmark. It has to be set up manually by choosing positionCount ~= partitionCount.
And yes, row-wise code path pollution works, it has visible influence on benchmark results:
arm
### without row-wise pollution
Benchmark (channelCount) (enableCompression) (nullRate) (partitionCount) (positionCount) (type) Mode Cnt Score Error Units
BenchmarkPartitionedOutputOperator.addPage 2 false 0 512 512 BIGINT avgt 10 276.219 ± 3.432 ms/op
### with row-wise pollution
Benchmark (channelCount) (enableCompression) (nullRate) (partitionCount) (positionCount) (type) Mode Cnt Score Error Units
BenchmarkPartitionedOutputOperator.addPage 2 false 0 512 512 BIGINT avgt 10 352.991 ± 9.117 ms/op```
2 participants
Description
Introduce a consistent TestType naming convention.
Add descriptions to the TestType cases.
Add more Dictionary test cases.
Add RowType with RLE field case.
Extracted (except for the docs commit) from #11289.
benchmark refactoring + documentation
only BenchmarkPartitionedOutputOperator
Improved code readibility
Related issues, pull requests, and links
Documentation
( ) No documentation is needed.
( ) Sufficient documentation is included in this PR.
( ) Documentation PR is available with #prnumber.
( ) Documentation issue #issuenumber is filed, and can be handled later.
Release notes
( ) No release notes entries required.
( ) Release notes entries required with the following suggested text: