Make ConnectorExpression JSON serializable

[assaf2]

Since TupleDomain is JSON serializable, IMO ConnectorExpression should be as well

[findepi]

Don't you need to enumerate known subclasses as in

trino/core/trino-main/src/main/java/io/trino/sql/planner/plan/PlanNode.java

Lines 28 to 69 in 36ff8b5

	@JsonSubTypes({
	@JsonSubTypes.Type(value = OutputNode.class, name = "output"),
	@JsonSubTypes.Type(value = ProjectNode.class, name = "project"),
	@JsonSubTypes.Type(value = TableScanNode.class, name = "tablescan"),
	@JsonSubTypes.Type(value = ValuesNode.class, name = "values"),
	@JsonSubTypes.Type(value = AggregationNode.class, name = "aggregation"),
	@JsonSubTypes.Type(value = MarkDistinctNode.class, name = "markDistinct"),
	@JsonSubTypes.Type(value = FilterNode.class, name = "filter"),
	@JsonSubTypes.Type(value = WindowNode.class, name = "window"),
	@JsonSubTypes.Type(value = RowNumberNode.class, name = "rowNumber"),
	@JsonSubTypes.Type(value = TopNRankingNode.class, name = "topnRanking"),
	@JsonSubTypes.Type(value = LimitNode.class, name = "limit"),
	@JsonSubTypes.Type(value = DistinctLimitNode.class, name = "distinctlimit"),
	@JsonSubTypes.Type(value = TopNNode.class, name = "topn"),
	@JsonSubTypes.Type(value = SampleNode.class, name = "sample"),
	@JsonSubTypes.Type(value = SortNode.class, name = "sort"),
	@JsonSubTypes.Type(value = RemoteSourceNode.class, name = "remoteSource"),
	@JsonSubTypes.Type(value = JoinNode.class, name = "join"),
	@JsonSubTypes.Type(value = SemiJoinNode.class, name = "semijoin"),
	@JsonSubTypes.Type(value = SpatialJoinNode.class, name = "spatialjoin"),
	@JsonSubTypes.Type(value = IndexJoinNode.class, name = "indexjoin"),
	@JsonSubTypes.Type(value = IndexSourceNode.class, name = "indexsource"),
	@JsonSubTypes.Type(value = RefreshMaterializedViewNode.class, name = "refreshmaterializedview"),
	@JsonSubTypes.Type(value = TableWriterNode.class, name = "tablewriter"),
	@JsonSubTypes.Type(value = DeleteNode.class, name = "delete"),
	@JsonSubTypes.Type(value = UpdateNode.class, name = "update"),
	@JsonSubTypes.Type(value = TableExecuteNode.class, name = "tableExecute"),
	@JsonSubTypes.Type(value = TableDeleteNode.class, name = "tableDelete"),
	@JsonSubTypes.Type(value = TableFinishNode.class, name = "tablecommit"),
	@JsonSubTypes.Type(value = UnnestNode.class, name = "unnest"),
	@JsonSubTypes.Type(value = ExchangeNode.class, name = "exchange"),
	@JsonSubTypes.Type(value = UnionNode.class, name = "union"),
	@JsonSubTypes.Type(value = IntersectNode.class, name = "intersect"),
	@JsonSubTypes.Type(value = EnforceSingleRowNode.class, name = "scalar"),
	@JsonSubTypes.Type(value = GroupIdNode.class, name = "groupid"),
	@JsonSubTypes.Type(value = ExplainAnalyzeNode.class, name = "explainAnalyze"),
	@JsonSubTypes.Type(value = ApplyNode.class, name = "apply"),
	@JsonSubTypes.Type(value = AssignUniqueId.class, name = "assignUniqueId"),
	@JsonSubTypes.Type(value = CorrelatedJoinNode.class, name = "correlatedJoin"),
	@JsonSubTypes.Type(value = StatisticsWriterNode.class, name = "statisticsWriterNode"),
	@JsonSubTypes.Type(value = PatternRecognitionNode.class, name = "patternRecognition"),
	})

?

[assaf2]

no, see #11447 (comment)

[findepi]

I had some thoughts in #11045, however same concern as in #1720 (comment) applies -- if SPI knows all subclasses, it would be convenient to add a visitor pattern;

also, JSON-serialization may be taken as implying serialized form backward- and forward-compatibility, but we make no guarantees about that, otherwise we should have test coverage for that.

@assaf2 what problem are you trying to solve?

cc @martint @losipiuk @hashhar @wendigo

[assaf2]

I had some thoughts in #11045, however same concern as in #1720 (comment) applies -- if SPI knows all subclasses, it would be convenient to add a visitor pattern;

The SPI won't need to be aware of all the subclasses. When @JsonTypeInfo is used, @JsonSubTypes is not mandatory (the opposite might be true though):

com.fasterxml.jackson.annotation.JsonTypeInfo.Id#CLASS:

       /**
         * Means that fully-qualified Java class name is used as the type identifier.
         */
        CLASS("@class"),

com.fasterxml.jackson.annotation.JsonSubTypes:

/**
 * Annotation used with {@link JsonTypeInfo} to indicate sub-types of serializable
 * polymorphic types, and to associate logical names used within JSON content
 * (which is more portable than using physical Java class names).
 *<p>
 * Note that just annotating a property or base type with this annotation does
 * NOT enable polymorphic type handling: in addition, {@link JsonTypeInfo}
 * or equivalent (such as enabling of so-called "default typing") annotation
 * is needed, and only in such case is subtype information used.
 */

We're already using @JsonTypeInfo(use = JsonTypeInfo.Id.CLASS) at io.trino.spi.metrics.Metric.
In addition, TupleDomain is JSON-serializable and it contains ColumnHandle which is extended by the connectors.

also, JSON-serialization may be taken as implying serialized form backward- and forward-compatibility, but we make no guarantees about that, otherwise we should have test coverage for that.

Why JSON-serialization backward- and forward-compatibility is taken more "seriously" than the SPI class itself?

@assaf2 what problem are you trying to solve?

I want to pass the connectorExpression from coordinator to worker without having to create a new class hierarchy.

[findepi]

In addition, TupleDomain is JSON-serializable and it contains ColumnHandle which is extended by the connectors.

TupleDomain is a concrete class (not extensible) and ColumnHandles have special treatment around serialization.

I want to pass the connectorExpression from coordinator to worker without having to create a new class hierarchy.

That was my intuition in #11045, but i realized not needing that is actually simple.
Why is it not simple in your case?

[assaf2]

That was my intuition in #11045, but i realized not needing that is actually simple. Why is it not simple in your case?

In my case, each worker dynamically supports different predicates.
In my case and in general, since connectorExpression represents the predicate and since the workers are the ones who use it, I think it would make sense to allow passing the predicate to the workers as-is.

[findepi]

I discussed this with @martint and per my understanding we don't want these expressions to be serializable.
Let's wait for @martint to chime in here directly.

[bitsondatadev]

👋 @assaf2 - this PR has become inactive. If you're still interested in working on it, please let us know.

We're working on closing out old and inactive PRs, so if you're too busy or this has too many merge conflicts to be worth picking back up, we'll be making another pass to close it out in a few weeks.