HIVE-29612: LATERAL VIEW OUTER doesn't work with CBO enabled#6482
HIVE-29612: LATERAL VIEW OUTER doesn't work with CBO enabled#6482soumyakanti3578 wants to merge 3 commits into
Conversation
asf-ci-hive
added
tests pending
tests passed
and removed
tests passed
tests pending
labels
|
rubenada
left a comment
rubenada
left a comment
There was a problem hiding this comment.
Not super familiar with this code (yet), but changes LGTM.
The only detail is the new boolean flag HiveTableFunctionScan#outer, which seems a bit ad-hoc. I wonder if in the future we'd need for it to be something more "generic" (like an enum), although atm this seems a bit overengineering if we only need to represent one value.
| return lateralView.getToken().getType() != HiveParser.TOK_LATERAL_VIEW_OUTER; | ||
| boolean isCBOSupportedLateralView() { | ||
| // Both LATERAL VIEW and LATERAL VIEW OUTER are supported in CBO. | ||
| return !this.conf.getBoolVar(HiveConf.ConfVars.HIVE_CBO_RETPATH_HIVEOP); |
There was a problem hiding this comment.
Previously the result didn't depend on the HIVE_CBO_RETPATH_HIVEOP conf option. Maybe it should still return true if lateralView.getToken().getType() == HiveParser.TOK_LATERAL_VIEW?
Could you please explain why using that conf option here? It's description is Flag to control calcite plan to hive operator conversion, but I don't understand its purpose.
There was a problem hiding this comment.
@thomasrebele
CBO return path is a different implementation of transforming the logical plan to Hive operators. It skips the AST conversion and converts Calcite operators directly to Hive operators. Maybe I'm missing something but I haven't found any change regarding this conversion in this patch.
@soumyakanti3578
Does lateral views in general is supported in CBO return path?
If yes could you please implement the lateral view outer too. I assume it would affect only 1-2 files and it would fit into the scope of this patch.
There was a problem hiding this comment.
Neither LATERAL VIEW nor LATERAL VIEW OUTER work with CBO return path. I'll create a separate ticket for them, as I don't think there's a ticket for them here: https://issues.apache.org/jira/browse/HIVE-9132
So the current condition in isCBOSupportedLateralView() is true for both types of LATERAL VIEW when CBO is enabled (If CBO is not enabled we take a different path), and it is false only when HIVE_CBO_RETPATH_HIVEOP is true (non default path for CBO). So I think the condition correctly reflects the current state.
| * @param node AST node | ||
| * @return true if node is of lateral view or lateral view outer; false otherwise. | ||
| */ | ||
| public static boolean isASTNodeLateralViewOrOuter(ASTNode node) { |
There was a problem hiding this comment.
nit.: I think we can name this just simply isASTNodeLateralView because IIUC lateral view outer is just a variation of lateral views. WDYT?
There was a problem hiding this comment.
Yes, I can rename this method in the next commit. 👍🏼
5 participants
What changes were proposed in this pull request?
Support
LATERAL VIEW OUTERwith CBO enabledWhy are the changes needed?
https://issues.apache.org/jira/browse/HIVE-29612
Does this PR introduce any user-facing change?
No
How was this patch tested?