@zabetak you contributed a lot on the original implementation of this (experimental) feature. If you have a bit of time, could you please take a look at this patch to address some existing issues on the current implementation? Thanks.

Hey @rubenada , I don't have much time the following 1-2 weeks to dive into this :/ I had a quick look but cannot have a clear picture in 10-15 min. I am afraid that putting the burden of adding/removing the transient table to the EnumerableRepeatUnion operator will make the latter less generic since it implies that we will not be able to use it with other sub-plans.

About CALCITE-3673, can't we remove the table while close the enumerator of the ListTransientTable?

The repeat union closes the enumerator of the iteration enumerable after every iteration. So this would also close the enumerator of the ListTransientTable, therefore making the table unavailable for the iterations after the first one. Alternatively we could introduce a TableSpoolScope operator that initializes the table and removes it when it is closed:

TableSpoolScope(delta) // <- creates the ListTransientScan and removes the table from the schema at the end
  RepeatUnion(iterations=..., all=...)
     TableSpool(delta)
        TableScan

The table parameter of EnumerableRepeatUnion could be made optional, allowing to keep the same behavior of the operator as before. @zabetak, could you be more specific about the uses with other sub-plans that you have in mind?

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).

It's true that the RelBuilder API around RepeatUnion is tight to a table (it may not even be a TransientTable but let's not focus on this right now) but the javadoc of RepeatUnion and its implementation (EnumerableRepeatUnion) do not mention at all tables, spools, or other operators. If you want to change the contract maybe it would be better to send an email to the dev list to see what people think and if there are others using RepeatUnion in some other way.

Apart from that, it might worth brainstorming a bit if the problem could be solved by handling the addition/removal of transient tables at another level and not during the execution of some specific operator; I don't have ideas at the moment 😆

@zabetak @thomasrebele I would like to get these issues fixed in the next release 1.30.
The proposed PR might not be perfect, but I'm not sure we will have much time for a deeper refactoring. On the positive side, this patch fixes 2 problems, and maintains RelBuilder API (so it is backwards compatible), so I think it is an improvement compared to the current status (and it is no hindrance to potential further work / re-designs around this feature in future versions). Wdyt?

It would be nice if the SchemaPlus.removeTable(String) could be avoided. As there is no easy way around this: LGTM.

Thanks for the feedback. I have addressed the suggestions and rebased.
If no new remarks come up, I plan to merge the PR in the coming days.