[CALCITE-5192] CodeGenerationBenchmark throws IllegalStateException #2834
Conversation
|
@chunweilei Thanks for your review.
I'm not sure about this, as existing benchmarks all do not have tests, I think maybe we do not need it? |
How do you find the bug if there is no test? We'd better add a test case to reproduce the error. |
I spotted it because I was running the benchmark manually to verify some performance issues, the bug do not leads to failoure/exception, the benchmark will catch it, and print error logs. From what I can see, the benchmarks are always run manually, and the result are used by hand, not by code (such as UT). |
| ICompilerFactory compilerFactory; | ||
| try { | ||
| compilerFactory = CompilerFactoryFactory.getDefaultCompilerFactory( | ||
| CodeGenerationBenchmark.class.getClassLoader()); | ||
| } catch (Exception e) { | ||
| throw new IllegalStateException( | ||
| "Unable to instantiate java compiler", e); | ||
| } |
There was a problem hiding this comment.
Do we also need to include the compilerFactory in the Supplier? Whatever we include in the supplier becomes part of the benchmark (and not part of the setup) so we should reduce it to the minimal.
| IClassBodyEvaluator cbe = compilerFactory.newClassBodyEvaluator(); | ||
| cbe.setClassName(info.classExpr.name); | ||
| cbe.setExtendedClass(Utilities.class); | ||
| cbe.setImplementedInterfaces( | ||
| plan.getRowType().getFieldCount() == 1 | ||
| ? new Class[]{Bindable.class, Typed.class} | ||
| : new Class[]{ArrayBindable.class}); | ||
| cbe.setParentClassLoader(EnumerableInterpretable.class.getClassLoader()); |
There was a problem hiding this comment.
Since the setup of the CBE is now becoming part of the benchmark maybe it would make more sense to extract it to a method and call it directly in the benchmark methods but I don't have strong feelings about this. Keeping it in a supplier should be fine as well.
|
@zabetak Thanks for your review comments, they are helpful, I've addressed both of them. |
|
merging |
No description provided.