DrillWindowQueryTest: use proper way to decide if the query is ordered #15118
Conversation
This reverts commit baddb89.
|
|
||
| public QueryResults( | ||
| final Map<String, Object> queryContext, | ||
| final String vectorizeOption, | ||
| final RelDataType sqlSignature, | ||
| final List<Object[]> results, | ||
| final List<Query<?>> recordedQueries, | ||
| final PlannerCaptureHook capture | ||
| final PlannerHook capture |
There was a problem hiding this comment.
is there a reason to use PlannerHook instead of PlannerCaptureHook here?
There was a problem hiding this comment.
I changed it to have the interface instead ; so that later other implementations can be used
...however it can be interpreted like the system captures a set of things for you and here they are in the QueryResults
I would rather see the results of this hook like a map of values or something instead of the hook itself...
I find this whole thing oddly designed as the interface has only captureX methods ; meanwhile the PlannerCaptureHook has the getX-s
should I change it back?
There was a problem hiding this comment.
Yes, I agree that the interface is a bit odd in terms of using it practically. But given that we specifically know how we want to use it in the tests, I think we can use PlannerCaptureHook itself - since that provides the interface + extra capabilities/bridge methods that we need.
…to windowing-fixes-isorderby
| sql = sql.toLowerCase(Locale.ENGLISH).replace('\n', ' '); | ||
| sql = sql.substring(sql.lastIndexOf(')')); | ||
| return sql.contains("order"); | ||
| SqlNode sqlNode = ((PlannerCaptureHook) queryResults.capture).getSqlNode(); |
There was a problem hiding this comment.
| SqlNode sqlNode = ((PlannerCaptureHook) queryResults.capture).getSqlNode(); | |
| SqlNode sqlNode = queryResults.capture.getSqlNode(); |
this can be done in any later change, no need to run CI for this!
There was a problem hiding this comment.
sure; I think my IDE will remove this cast right away when I next change that file
|
Thank you @rohangarg for reviewing and merging the changes! |
|
Removing the milestone since it is a test change and shouldn't affect the behavior, therefore backporting is not needed. |
SqlNodeduring planningisOrderedbased onSqlToRelConverter#isOrdered