rubenada commented on pull request #2690: URL: https://github.com/apache/calcite/pull/2690#issuecomment-1014324016
Thanks for your feedback @zabetak . No worries, there is no rush for this PR (as long as it makes it for 1.30 :P ) > About CALCITE-3673, can't we remove the table while close the enumerator of the ListTransientTable? Yes, that could be a possible solution. However, there is a small problem, this `close` method can be called multiple times in the life time of the RepeatUnion (e.g. multiple iterations of the iterativeRel; or in case this iterativeRel contains a correlation, etc.). So the `removeTable` would be called multiple times, only the first one being effective. This is a minor issue though, in fact from the two tickets involved in this PR, CALCITE-3673 is the "minor" one (the transient table left behind in the schema, but the application does not crash). On the contrary, CALCITE-4054 is much more serious because we have a NPE. The problem here is when the transient table is **added** to the schema; and as I see it, the only possible solution to guarantee that the table will be added on time, and will be accessible for all the operators inside RepeatUnion's inputs is putting the addTable logic inside the RepeatUnion itself (this guarantees fixing CALCITE-4054). Following this logic, my understanding is that analogously the removeTable logic should be put in the RepeatUnion's `close` (i.e. the operator will take responsibility for adding/removing its temporary table). This solution guarantees a proper (and singly executed) add/remove table. It is true that this change puts some extra burden on the RepeatUnion, and it makes it less generic and potentially reusable on other use cases (and this was one of our main concerns when we first designed this feature). However, I think it is more important that the primary RepeatUnion use case (recursive union) is able to perform flawless. In any case, at this point this main (and only?) RepeatUnion use case is very tight to its transient table (see `RelBuilder#repeatUnion`, without the valid table the RepeatUnion cannot be created). If in the future this changes, and we see another use case for the RepeatUnion that does not require the transient table, we could re-work the operator, e.g. we could make the new RepeatUnion's transientTable nullable (to be used in the recursive union scenario; and null in this theoretical new scenario), and adapt the code generation in `EnumerableRepeatUnion#implement` (if transientTable != null generate the addTable / removeTable code, otherwise not); but at this point IMO it is more important to fix the current use case than thinking about operator re-usage in potential new use cases (bearing in mind that nothing is set on stone, all the operators involved in this feature are `@Experimental`, so they could be re-worked in the future). -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
