Skip to content

Add test for CBO decisions on TPC-H, TPC-DS queries#11665

Merged
kokosing merged 2 commits intoprestodb:masterfrom
kokosing:origin/master/035_cbo_tests
Oct 16, 2018
Merged

Add test for CBO decisions on TPC-H, TPC-DS queries#11665
kokosing merged 2 commits intoprestodb:masterfrom
kokosing:origin/master/035_cbo_tests

Conversation

@kokosing
Copy link
Contributor

@kokosing kokosing commented Oct 9, 2018

Add test for CBO decisions on TPC-H, TPC-DS queries

@kokosing
Copy link
Contributor Author

kokosing commented Oct 9, 2018

Applied comments from #11267

@kokosing kokosing closed this Oct 9, 2018
@kokosing kokosing reopened this Oct 9, 2018
Copy link
Contributor Author

Choose a reason for hiding this comment

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

In a follow up I would like to extract this concept and use it (replace the usage) instead of com.facebook.presto.sql.planner.assertions.PlanMatchPattern. This is much more easier to debug and maintain.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

cc: @atris

Copy link
Member

@arhimondr arhimondr left a comment

Choose a reason for hiding this comment

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

Some comments

Copy link
Member

Choose a reason for hiding this comment

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

I think this readme file should remain in presto-benchto-benchmarks

Copy link
Member

Choose a reason for hiding this comment

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

presto-benchto-queries

pom.xml Outdated
Copy link
Member

Choose a reason for hiding this comment

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

Move it to the previous commit

pom.xml Outdated
Copy link
Member

Choose a reason for hiding this comment

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

Move it to the previous commit

Copy link
Member

Choose a reason for hiding this comment

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

Move it to the previous commit

Copy link
Member

Choose a reason for hiding this comment

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

You don't need this. Use Resources.toString()

Copy link
Member

Choose a reason for hiding this comment

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

Ditto. Use Resources

Copy link
Member

Choose a reason for hiding this comment

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

Scan queries in the classpath instead.

Reflections reflections = new Reflections("some.package", new ResourcesScanner());
Set<String> fileNames = reflections.getResources(Pattern.compile("q(\\d+)\.sql"));

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would prefer not to drag new dependency just for this

Copy link
Member

Choose a reason for hiding this comment

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

Sorry, i thought this is the class from Guava. Nevermind than.

Copy link
Member

Choose a reason for hiding this comment

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

Extract ID from query file Pattern.compile("q(\\d+)\.sql")

Copy link
Member

Choose a reason for hiding this comment

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

Scan resources from the resource path, so you don't need to do this hack

Thanks to this extraction queries could be use from other modules,
without copying query files or dragging any not needed dependencies.
@kokosing
Copy link
Contributor Author

Comments applied.

Copy link
Member

@arhimondr arhimondr left a comment

Choose a reason for hiding this comment

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

LGTM % few comments

Copy link
Member

Choose a reason for hiding this comment

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

Make it String. Path object implies a reference to a real file system.

Copy link
Member

Choose a reason for hiding this comment

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

How about

private String getQueryPlanResourcePath(String queryResourcePath)
    {
        return queryResourcePath.replace("\.sql$", ".plan");
    }

Copy link
Member

Choose a reason for hiding this comment

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

static import

Copy link
Member

Choose a reason for hiding this comment

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

static import

Copy link
Member

Choose a reason for hiding this comment

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

static import getResource. Also you can use getResource(string). Context class loader should be fine here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Context class loader should be fine here.

It does not work.

Copy link
Member

Choose a reason for hiding this comment

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

Move it before the JoinOrderPrinter. Class definition come last.

Copy link
Contributor Author

@kokosing kokosing left a comment

Choose a reason for hiding this comment

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

Comments addressed. Thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Context class loader should be fine here.

It does not work.

@kokosing kokosing merged commit 3022c0e into prestodb:master Oct 16, 2018
@kokosing kokosing deleted the origin/master/035_cbo_tests branch October 16, 2018 08:20
@findepi
Copy link
Contributor

findepi commented Oct 18, 2018

Was it an intentional change that i can no longer just run TestTpcdsCostBasedPlan$UpdateTestFiles?

Exception in thread "main" java.lang.IllegalStateException: java.lang.IllegalStateException: This class must be executed from presto-main or presto source directory
	at sun.reflect.NativeConstructorAccessorImpl.newInstance0(Native Method)
	at sun.reflect.NativeConstructorAccessorImpl.newInstance(NativeConstructorAccessorImpl.java:62)
	at sun.reflect.DelegatingConstructorAccessorImpl.newInstance(DelegatingConstructorAccessorImpl.java:45)
	at java.lang.reflect.Constructor.newInstance(Constructor.java:423)
	at java.util.concurrent.ForkJoinTask.getThrowableException(ForkJoinTask.java:593)
	at java.util.concurrent.ForkJoinTask.reportException(ForkJoinTask.java:677)
	at java.util.concurrent.ForkJoinTask.invoke(ForkJoinTask.java:735)
	at java.util.stream.ForEachOps$ForEachOp.evaluateParallel(ForEachOps.java:160)
	at java.util.stream.ForEachOps$ForEachOp$OfRef.evaluateParallel(ForEachOps.java:174)
	at java.util.stream.AbstractPipeline.evaluate(AbstractPipeline.java:233)
	at java.util.stream.ReferencePipeline.forEach(ReferencePipeline.java:418)
	at com.facebook.presto.sql.planner.optimizations.AbstractCostBasedPlanTest.generate(AbstractCostBasedPlanTest.java:90)
	at com.facebook.presto.sql.planner.optimizations.TestTpcdsCostBasedPlan$UpdateTestFiles.main(TestTpcdsCostBasedPlan.java:91)
Caused by: java.lang.IllegalStateException: This class must be executed from presto-main or presto source directory
	at com.facebook.presto.sql.planner.optimizations.AbstractCostBasedPlanTest.getPrestoMainSourcePath(AbstractCostBasedPlanTest.java:143)
	at com.facebook.presto.sql.planner.optimizations.AbstractCostBasedPlanTest.lambda$generate$0(AbstractCostBasedPlanTest.java:93)
...

@findepi
Copy link
Contributor

findepi commented Oct 18, 2018

Also, the exception is misleading. IntelliJ was setting presto source dir as working directory by default.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants