[Calcite 1758] push order and limit to druid #433
Conversation
| .multiplyBy(getQueryTypeCostMultiplier()); | ||
| .multiplyBy(getQueryTypeCostMultiplier()) | ||
| // a plan with sort pushed to druid is better than doing sort outside of druid | ||
| .multiplyBy(Util.last(rels) instanceof Bindables.BindableSort ? 0.1 : 0.2); |
There was a problem hiding this comment.
We should use the superclass in case other rules do not generate a Sort with bindable convention, i.e., use instanceof Sort instead of BindableSort.
Further, if last operator is not a SortLimit, we should just leave cost as it is, i.e., multiply by 1 instead of 0.2.
| } | ||
| if (originalFieldName.equals(druidTable.timestampFieldName)) { | ||
| finalGranularity = Granularity.ALL; | ||
| String extractColumnName = SqlValidatorUtil.uniquify(EXTRACT_COLUMN_NAME_PREFIX |
| @@ -590,32 +589,21 @@ private static boolean validSortLimit(Sort sort, DruidQuery query) { | |||
| int metricsRefs = 0; | |||
| for (RelFieldCollation col : sort.collation.getFieldCollations()) { | |||
| int idx = col.getFieldIndex(); | |||
| //computes the number of metrics in the sort | |||
| if (idx >= topAgg.getGroupCount()) { | |||
| metricsRefs++; | |||
There was a problem hiding this comment.
metricsRefs is not used anymore (and it seems it is not needed). If it became useless, could we get rid of it?
| + "EnumerableInterpreter\n" | ||
| + " BindableSort(sort0=[$1], sort1=[$0], dir0=[ASC], dir1=[DESC], offset=[2], fetch=[3])\n" | ||
| + " DruidQuery(table=[[foodmart, foodmart]], intervals=[[1900-01-09T00:00:00.000/2992-01-10T00:00:00.000]], projects=[[$39, $30]], groups=[{0, 1}], aggs=[[]])"; | ||
| final String explain = "PLAN=EnumerableLimit(offset=[2], fetch=[3])\n" |
There was a problem hiding this comment.
It seems we do not push the limit because of the offset? In that case, we could just push the fetch part and keep the offset/limit on top of the DruidQuery as it is right now, I think that would still be beneficial. Since this is not a blocker, we could tackle that in a follow-up. Could you create a JIRA case to keep track of the issue?
There was a problem hiding this comment.
i guess this can be tricky, since druid does not support offsets -> i need to alter the limit and return let say 5 instead of 3 and let calcite do the offset ?
There was a problem hiding this comment.
Exactly, that is what I meant. But in any case, it can be part of a follow-up as this patch is almost ready; we can create the issue and tackle later.
|
@jcamachor fixed all the comments thanks ! |
…ion and additional columns (Slim Bouguerra) Close apache/calcite#433
…ion and additional columns (Slim Bouguerra) Close apache/calcite#433
…ion and additional columns (Slim Bouguerra) Close apache#433
No description provided.