DRILL-6475: Unnest: Null fieldId Pointer. #1381
Conversation
| } | ||
|
|
||
| private void register(Prel toRegister) { | ||
| this.sourceOperatorRegistry.put(toRegister.getClass().getSimpleName(), toRegister); |
There was a problem hiding this comment.
It is odd to register the mapping of class name to the Prel. The association of a LateralJoin to Unnest should ideally be done through instances of the class. i.e a particular Unnest instance is associated with a particular Lateral instance (each Lateral can be associated with multiple Unnest instances), otherwise there is potential for bugs due to incorrect association. Thoughts ?
There was a problem hiding this comment.
Yes, I agree that each lateral can be associated to many unnest. I think the current design itself doesn't take into account the correct association with unnest and lateral. It traverses the right of the lateral looking for unnest and associates them. This can cause potential issues when we support binary operators like union, join etc. I think ideally we should have an id with each lateral and then associate that id to an unnest. This id can be used in the map while traversal etc. But for now as we don't support binary operators I have used the classname instead of id's to associate the lateral and unnest. Let me know if you have a better way to fix this issue.
There was a problem hiding this comment.
One option is to have a specialized PrelVisitor that does the traversal (similar to what we do for a few other things in physical planning) and for the right child keeps track of the nearest ancestor Lateral and if it encounters an Unnest, it ties up the two together. In fact the Materalizer already does this for the POPs but for your use case this is needed earlier in the planning phase.
|
@amansinha100 As discussed I have modified the code. Can you please take a look and let me know if there any other changes required. |
amansinha100
left a comment
amansinha100
left a comment
There was a problem hiding this comment.
Couple of minor follow up comments. Overall +1.
| } | ||
|
|
||
| private void register(Prel prel) { | ||
| this.currentLateralJoin = prel; |
There was a problem hiding this comment.
The registerPrel and unregisterPrel imply that they are working with any arbitrary physical rel, not specifically with lateral join. Why not use a more generic name instead of currentLateralJoin because in future another operator may need to register a different physical rel.
| * the children of the join has same field names. Whereas in case of lateral/unnest operators it changes the correlated | ||
| * field and also the unnest operator's output row type. | ||
| */ | ||
| public class AdjustOperatorsRowTypeVisitor extends BasePrelVisitor<Prel, Void, RuntimeException>{ |
There was a problem hiding this comment.
One minor comment about this..I think having 'RowType' in the visitor class name is slightly misleading because the visitor for joins column renaming is inserting a project below to do the renaming. Also 'RowType' is a Calcite terminology and it is possible (logically speaking) to implement this without actually using RowType. Perhaps use 'Schema' instead of 'RowType' ?
|
@amansinha100 , @HanumathRao since it has an overall +1, I added the ready-to-commit label on the JIRA |
3 participants
@amansinha100 Can you please review this PR.
This PR includes changes related to updating the row type and also correlated column for the unnest prel.