Fix classloading issue with OutputNode PlanNode

[jainxrohit]

OutputNode plan class is used for creating logical plan. Now logical plan creation is part of the pluggable analyzer interface, respective analyzer plugins are expected to use OutputNode class.

Previously, the OutputNode class was extended from InternalPlanNode, which required it to be available with AppClassLoader. However, since the analyzer plugins now instantiate the OutputNode class, it and its dependencies are loaded by PluginClassLoader, making it unavailable for the rest of the AppClassLoader.

To fix this issue, we are extending OutputNode from PlanNode instead of InternalPlanNode and also moving it to the spi package, as it is anyways expected to be leveraged by plugins. This also whitelists this class for PluginClassLoader.

The other alternative would have been to add com.facebook.presto.sql.planner package to the whitelists for PluginClassLoader, however that would have added to many classes to the whitelist which is not necessary.

== NO RELEASE NOTE ==

[mlyublena]

why did this change from Immutable to unmodifiable?

[jainxrohit]

Presto SPI packages do not use guava. Since we are moving this class to SPI, we need to find alternatives for Guava.

[elharo]

It's also a good idea to replace Guava and similar libraries with JDK methods where available. A lot of Guava, Apache commons collections, and the like exist primarily to provide utilities that are available in JDK 8+ for earlier JDK versions.

[rschlussel]

why is this specific to output node and not all plan nodes?

[jainxrohit]

why is this specific to output node and not all plan nodes?

For all plan nodes that we anticipate a pluggable analyzer to use, this would hold true. However, for internal plan nodes (such as ExchangeNode), which are not utilized by the analyzer, this would not be the case. In the present range of query shapes that Crux supports, the only missing class was the OutputNode.

[rschlussel]

why is this specific to output node and not all plan nodes?

For all plan nodes that we anticipate a pluggable analyzer to use, this would hold true. However, for internal plan nodes (such as ExchangeNode), which are not utilized by the analyzer, this would not be the case. In the present range of query shapes that Crux supports, the only missing class was the OutputNode.

That would include most plan nodes then. Currently very few plan nodes are in the spi. we'll need to add almost all of them over.

[jainxrohit]

Not all, only those which are required while constructing logical plan nodes. Nodes added during optimization phase are not required.
So either we can move these one by one as we construct shapes in the CRUX (which I feel is cleaner approach, as classes used by plugins should be in SPI), or we can add current main class planner package to whitelist.

At the moment following nodes are defined nodes and related classes are used by inbuilt analyzer. Which potentially we would need to migrate to SPI.

com.facebook.presto.sql.planner.plan.DeleteNode;
com.facebook.presto.sql.planner.plan.ExplainAnalyzeNode;
com.facebook.presto.sql.planner.plan.StatisticAggregations;
com.facebook.presto.sql.planner.plan.StatisticsWriterNode;
com.facebook.presto.sql.planner.plan.TableFinishNode;
com.facebook.presto.sql.planner.plan.TableWriterNode;
com.facebook.presto.sql.planner.plan.TableWriterNode.DeleteHandle;

We have following options:

1. Migrate these later when we use these in CRUX analyzer.
2. Migrate them now.
3. Instead of doing this migration, we can whitelist planner package. But this seems odd to me, as why are these nodes defined as internalNode if these are being used by plugins.

At this point I have taken the approach 1.

[jainxrohit]

I discussed offline with @rschlussel and we have agreed to go with approach 2. I will update the PR by fixing more plan nodes, which would be used by analyzers plugins.

[elharo]

com.facebook.presto.sql.planner.plan.OutputNode appears to be public API. Who's affected by removing it?

[jainxrohit]

com.facebook.presto.sql.planner.plan.OutputNode appears to be public API. Who's affected by removing it?

@elharo we are not removing it, we have moved to spi package, which makes it even more official. SPI package in general is very well understood by plugins owner.

[rschlussel]

We discussed, and not all nodes can be easily moved to the spi, so some work is needed there. We'll start with just the OutputNode to unblock testing an end to end flow for a simple query with a plugin analyzer. We'll migrate the rest of the nodes afterwards.

[mbasmanova]

@jainxrohit Rohit, it seem like this change broken Sapphire-Native. CC: @miaoever @tanjialiang

[jainxrohit]

@jainxrohit Rohit, it seem like this change broken Sapphire-Native. CC: @miaoever @tanjialiang

Fixed in the #19284