[
https://issues.apache.org/jira/browse/CALCITE-889?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14847342#comment-14847342
]
Jesus Camacho Rodriguez commented on CALCITE-889:
-------------------------------------------------
[~pxiong], some comments on the patch.
- Please, check the style of the patch, as checkstyle does not pass (using
tabs, incorrect order of imports, etc). If you compile with mvn, it will give
you info about exact problems.
- Concerning SortUnionTransposeRule, you are not adding all the union inputs to
the list e.g. if Sort is not added, you skip that part.
- Further, I'm not convinced about bailing out if the condition in line 75 is
not met; although it might not be a valid construct in SQL, a Sort below the
Union is a valid plan construct. In order to bail out properly, I think we
could check if the input to the Union is a Sort operator. If it is, we would
replace it with a copy of the top Sort operator. If it is not, we introduce the
copy of the top Sort operator between the Union and its input.
- In Line 87, you should not create a LogicalUnion, but instead use Union.copy
(or a factory).
Btw, for reviewing the patch, I think it is easier if you create a GitHub pull
request.
Thanks
> Implement SortUnionTransposeRule
> --------------------------------
>
> Key: CALCITE-889
> URL: https://issues.apache.org/jira/browse/CALCITE-889
> Project: Calcite
> Issue Type: Sub-task
> Reporter: Pengcheng Xiong
> Assignee: Julian Hyde
> Attachments: CALCITE-889.01.patch
>
>
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
