Add more sql tests for groupby queries#11454
Conversation
suneet-s
left a comment
suneet-s
left a comment
There was a problem hiding this comment.
Thanks for the tests - LGTM
Just a comment about deciding where the tests should live. Reading this PR I can't tell why some tests should live in 1 class vs another
| import org.junit.Test; | ||
| import org.junit.runner.RunWith; | ||
|
|
||
| @RunWith(JUnitParamsRunner.class) |
There was a problem hiding this comment.
I don't see any parameterized tests in this class
| import org.junit.runner.RunWith; | ||
|
|
||
| @RunWith(JUnitParamsRunner.class) | ||
| public class CalciteSimpleQueryTest extends BaseCalciteQueryTest |
There was a problem hiding this comment.
Can you add a doc somewhere that talks about the difference between CalciteQueryTest and CalciteSimpleQueryTest.
As a naive developer - which tests should live in which class and how would I know where to put my new tests
There was a problem hiding this comment.
I added some simple javadocs. The reason I added this new class is CalciteQueryTest is getting bigger and bigger and now it hits almost 20000 lines. This makes it hard to tell what SQL patterns are covered by our tests. I haven't moved existing tests in this PR because moving them will make hard to see what are new tests and what are not. I will create a follow-up PR to move them and split CalciteQueryTest further.
suneet-s
left a comment
suneet-s
left a comment
There was a problem hiding this comment.
Thanks @jihoonson ! LGTM after CI
|
checkstyle failure looks legit |
|
@suneet-s thanks for the review! |
3 participants
Description
#11379 makes group by queries faster by making a better query plan. This PR adds more unit tests to cover some basic cases where the optimization in #11379 can affect.
This PR has: