[CALCITE-3153] Improve testing in TpcdsTest using assertEqual instead of printing results#1283
Conversation
| // foo(with, "WEB_PAGE", 60); | ||
| // foo(with, "WEB_RETURNS", 71_763); | ||
| // foo(with, "WEB_SALES", 719_384); | ||
| // foo(with, "WEB_SITE", 30); |
There was a problem hiding this comment.
Why to open up these tests ? which seems has no relationship with the fix title.
There was a problem hiding this comment.
Thanks for @danny0405 's replies. I just doubt why this should be commented。If it's not ok, i will keep original instead.
There was a problem hiding this comment.
I see @julianhyde did the modification in 93b8349, maybe he can answer you!
There was a problem hiding this comment.
I would assume to avoid generating data for those tables just for the sake of count queries but I may be wrong.
There was a problem hiding this comment.
Thanks for @zabetak 's replies. Those tables' rowCounts are added in org.apache.calcite.adapter.tpcds.TpcdsSchema, it uses TABLE_ROW_COUNTS mapping to maintaince tables' rowCounts. So I think there's no cost to generate data?
Maybe it's better to open up these tests again?
There was a problem hiding this comment.
I see that foo method executes a query to the table passed as a parameter. If I remember well TpcdsQueryableTable which I think is involved on this query generates the respective data every time it is referenced.
There was a problem hiding this comment.
If those codes are useless, so just remove them any more? I wonder what I should do, maybe just keep it unchanged?
|
Hey @LiShuMing, it seems that you have checkstyle violations. In most cases before submitting a PR it is highly recommended to test that the build passes locally |
Sorry, it's my first merge request. I'll fix it right now. |
plus/src/test/java/org/apache/calcite/adapter/tpcds/TpcdsTest.java
Outdated
Show resolved
Hide resolved
hsyuan
left a comment
hsyuan
left a comment
There was a problem hiding this comment.
Now the changes look good to me.
hsyuan
added
the
LGTM-will-merge-soon
|
@LiShuMing Can you squash the commits? I will merge this PR after that. |
|
Thanks for @hsyuan 's replies. I just resolve the comments. |
… of printing results
|
retrigger tests |
4 participants
It's a little improve to use
assertEqualinstead of just printing result in TpcdsTest#testQuery27Builder.