Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add WorkProcessorOperatorAdapter and use it for TopNOperator #889

Merged

Conversation

3 participants
@sopel39
Copy link
Member

commented Jun 2, 2019

some benchmarks:

Before best
Benchmark                   (positionsPerPage)  (topN)   Mode  Cnt   Score   Error  Units
BenchmarkTopNOperator.topN                  32       1  thrpt   60  23,024 ± 0,292  ops/s
BenchmarkTopNOperator.topN                  32     100  thrpt   60  22,101 ± 0,249  ops/s
BenchmarkTopNOperator.topN                  32   10000  thrpt   60   6,634 ± 0,178  ops/s
BenchmarkTopNOperator.topN                1024       1  thrpt   60  32,031 ± 0,227  ops/s
BenchmarkTopNOperator.topN                1024     100  thrpt   60  29,742 ± 0,180  ops/s
BenchmarkTopNOperator.topN                1024   10000  thrpt   60   9,917 ± 0,091  ops/s

After 1
Benchmark                   (positionsPerPage)  (topN)   Mode  Cnt   Score   Error  Units
BenchmarkTopNOperator.topN                  32       1  thrpt   60  21,778 ± 0,293  ops/s
BenchmarkTopNOperator.topN                  32     100  thrpt   60  20,971 ± 0,463  ops/s
BenchmarkTopNOperator.topN                  32   10000  thrpt   60   6,601 ± 0,193  ops/s
BenchmarkTopNOperator.topN                1024       1  thrpt   60  31,952 ± 0,199  ops/s
BenchmarkTopNOperator.topN                1024     100  thrpt   60  29,581 ± 0,389  ops/s
BenchmarkTopNOperator.topN                1024   10000  thrpt   60   9,756 ± 0,089  ops/s

After 2
Benchmark                   (positionsPerPage)  (topN)   Mode  Cnt   Score   Error  Units
BenchmarkTopNOperator.topN                  32       1  thrpt   60  22,921 ± 0,333  ops/s
BenchmarkTopNOperator.topN                  32     100  thrpt   60  21,170 ± 0,249  ops/s
BenchmarkTopNOperator.topN                  32   10000  thrpt   60   6,637 ± 0,186  ops/s
BenchmarkTopNOperator.topN                1024       1  thrpt   60  31,663 ± 0,267  ops/s
BenchmarkTopNOperator.topN                1024     100  thrpt   60  30,293 ± 0,217  ops/s
BenchmarkTopNOperator.topN                1024   10000  thrpt   60   9,694 ± 0,243  ops/s

@sopel39 sopel39 requested a review from dain Jun 2, 2019

@cla-bot cla-bot bot added the cla-signed label Jun 2, 2019

@sopel39 sopel39 force-pushed the starburstdata:ks/work_processor_non_source_adapter branch 3 times, most recently from 6b5828b to 0fe7dd3 Jun 2, 2019

@dain

dain approved these changes Jun 2, 2019

Copy link
Member

left a comment

Some minor comments but otherwise looks good

Show resolved Hide resolved ...in/src/main/java/io/prestosql/operator/WorkProcessorOperatorAdapter.java Outdated
Show resolved Hide resolved presto-main/src/main/java/io/prestosql/operator/TopNOperator.java
}
}

private void finish()

This comment has been minimized.

Copy link
@dain

dain Jun 2, 2019

Member

I may have missed it, but I think there is only one caller of this, so I would inline this. I think it might be clear to readers if this is not something to be called (since this is an inner class).

This comment has been minimized.

Copy link
@sopel39

sopel39 Jun 3, 2019

Author Member

I think it might be clear to readers if this is not something to be called (since this is an inner class).

Isn't private keyword sufficient for that? This method is similar to add method directly above in this regard. Anyway in this case it seems cleaner that private PageBuffer field is not directly modified from wrapping class (as none of other fields).

This comment has been minimized.

Copy link
@dain

dain Jun 3, 2019

Member

The class containing the inner class is allowed access to any method or field regardless of access modifier.

Show resolved Hide resolved ...in/src/main/java/io/prestosql/operator/WorkProcessorOperatorAdapter.java Outdated

@sopel39 sopel39 force-pushed the starburstdata:ks/work_processor_non_source_adapter branch from 0fe7dd3 to 005b7b7 Jun 3, 2019

@sopel39 sopel39 merged commit 6e1ac20 into prestosql:master Jun 3, 2019

1 of 2 checks passed

Travis CI - Pull Request Build Errored
Details
verification/cla-signed
Details

@sopel39 sopel39 deleted the starburstdata:ks/work_processor_non_source_adapter branch Jun 3, 2019

@findepi findepi added this to the 314 milestone Jun 6, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.