scarlin-cloudera commented on code in PR #4442: URL: https://github.com/apache/hive/pull/4442#discussion_r1286168676
########## ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/translator/PlanModifierForASTConv.java: ########## @@ -382,6 +387,11 @@ private static boolean validExchangeChild(HiveSortExchange sortNode) { return sortNode.getInput() instanceof Project; } + private static boolean validTableFunctionScanChild(HiveTableFunctionScan htfsNode) { + return htfsNode.getInputs().size() == 1 && + (htfsNode.getInput(0) instanceof Project || htfsNode.getInput(0) instanceof HiveTableScan); Review Comment: I found this code kinda weird as I went through it. My observation, and perhaps it is wrong, is that this module makes up for some defects in the ASTConverter. The ASTConverter requires that the level underneath it be a Project or HiveTableFunctionScan. If it isn't, the caller "introduces" a Project so that the ASTConverter can handle it properly. I potentially misunderstood this code, but while I was testing, I hit this issue in the ASTConverter and this seems to follow the trend of already existing code. I do think ASTConverter should be rewritten, but if we went that far, we should perhaps go to the physical operator directly rather than convert to AST and then convert to the physical operator. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: gitbox-unsubscr...@hive.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org --------------------------------------------------------------------- To unsubscribe, e-mail: gitbox-unsubscr...@hive.apache.org For additional commands, e-mail: gitbox-h...@hive.apache.org