refactor: Prepare TableFunction SPI for C++#27365
Merged
aditi-pandit merged 1 commit intoprestodb:masterfrom Mar 19, 2026
Merged
refactor: Prepare TableFunction SPI for C++#27365aditi-pandit merged 1 commit intoprestodb:masterfrom
aditi-pandit merged 1 commit intoprestodb:masterfrom
Conversation
aditi-pandit
requested review from
a team,
feilong-liu and
jaystarshot
as code owners
prestodb-ci
requested review from
a team,
ScrapCodes and
infvg
and removed request for
a team
Contributor
Reviewer's GuideRefactors the table function SPI to use Jackson-serializable polymorphic types for arguments and return type specifications, introduces dedicated return type specification classes, and wires them through analyzers, registry, and tests in preparation for C++ interop. Class diagram for new ReturnTypeSpecification hierarchyclassDiagram
class ReturnTypeSpecification {
<<abstract>>
+String getReturnType()
}
class GenericTableReturnTypeSpecification {
+GenericTableReturnTypeSpecification GENERIC_TABLE
-String returnType
+GenericTableReturnTypeSpecification(String returnType)
+boolean equals(Object o)
+int hashCode()
+String getReturnType()
}
class DescribedTableReturnTypeSpecification {
-Descriptor descriptor
-String returnType
+DescribedTableReturnTypeSpecification(Descriptor descriptor)
+Descriptor getDescriptor()
+String getReturnType()
}
class OnlyPassThroughReturnTypeSpecification {
+OnlyPassThroughReturnTypeSpecification ONLY_PASS_THROUGH
-String returnType
+OnlyPassThroughReturnTypeSpecification(String returnType)
+String getReturnType()
}
ReturnTypeSpecification <|-- GenericTableReturnTypeSpecification
ReturnTypeSpecification <|-- DescribedTableReturnTypeSpecification
ReturnTypeSpecification <|-- OnlyPassThroughReturnTypeSpecification
Class diagram for ArgumentSpecification polymorphic hierarchyclassDiagram
class ArgumentSpecification {
<<abstract>>
-String name
-boolean required
-Object defaultValue
+ArgumentSpecification(String name, boolean required, Object defaultValue)
+String getName()
+boolean isRequired()
+Object getDefaultValue()
}
class TableArgumentSpecification {
-boolean rowSemantics
-boolean pruneWhenEmpty
-boolean passThroughColumns
+TableArgumentSpecification(String name, boolean rowSemantics, boolean pruneWhenEmpty, boolean passThroughColumns)
+boolean isRowSemantics()
+boolean isPruneWhenEmpty()
+boolean isPassThroughColumns()
}
class DescriptorArgumentSpecification {
+DescriptorArgumentSpecification(String name, boolean required, Descriptor defaultValue)
}
class ScalarArgumentSpecification {
-Type type
+ScalarArgumentSpecification(String name, Type type, boolean required, Object defaultValue)
+Type getType()
}
ArgumentSpecification <|-- TableArgumentSpecification
ArgumentSpecification <|-- DescriptorArgumentSpecification
ArgumentSpecification <|-- ScalarArgumentSpecification
Class diagram for table function SPI integration pointsclassDiagram
class AbstractConnectorTableFunction {
-String schema
-String name
-List~ArgumentSpecification~ arguments
-ReturnTypeSpecification returnTypeSpecification
+AbstractConnectorTableFunction(String schema, String name, List~ArgumentSpecification~ arguments, ReturnTypeSpecification returnTypeSpecification)
+String getSchema()
+String getName()
+List~ArgumentSpecification~ getArguments()
+ReturnTypeSpecification getReturnTypeSpecification()
}
class ConnectorTableFunctionHandle {
<<interface>>
+ConnectorSplitSource getSplits(ConnectorTransactionHandle transaction, ConnectorSession session, NodeManager nodeManager, Object functionAndTypeManager)
}
class TableArgument {
-RowType rowType
+RowType getRowType()
+List~RowType.Field~ getFields()
+List~String~ getPartitionBy()
}
class DescriptorArgument {
-Optional~Descriptor~ descriptor
+DescriptorArgument(Optional~Descriptor~ descriptor)
+Optional~Descriptor~ getDescriptor()
}
AbstractConnectorTableFunction --> ReturnTypeSpecification
AbstractConnectorTableFunction --> ArgumentSpecification
TableArgument --> RowType
DescriptorArgument --> Descriptor
ConnectorTableFunctionHandle ..> ConnectorSplitSource
ConnectorTableFunctionHandle ..> ConnectorTransactionHandle
ConnectorTableFunctionHandle ..> ConnectorSession
ConnectorTableFunctionHandle ..> NodeManager
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- The
OnlyPassThroughReturnTypeSpecificationreturn type string is spelled"PASSTRHOUGH"and the switch inStatementAnalyzer.verifyProperColumnsDescriptormatches that value; if this is intended to be"PASSTHROUGH"(or to match any existing external contract), both the constant and switch cases should be corrected consistently. - The
GenericTableReturnTypeSpecificationandOnlyPassThroughReturnTypeSpecification@JsonCreatorconstructors accept areturnTypeparameter that is ignored; consider either removing the parameter or validating it to avoid confusing, unused input in the serialized form.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `OnlyPassThroughReturnTypeSpecification` return type string is spelled `"PASSTRHOUGH"` and the switch in `StatementAnalyzer.verifyProperColumnsDescriptor` matches that value; if this is intended to be `"PASSTHROUGH"` (or to match any existing external contract), both the constant and switch cases should be corrected consistently.
- The `GenericTableReturnTypeSpecification` and `OnlyPassThroughReturnTypeSpecification` `@JsonCreator` constructors accept a `returnType` parameter that is ignored; consider either removing the parameter or validating it to avoid confusing, unused input in the serialized form.
## Individual Comments
### Comment 1
<location path="presto-spi/src/main/java/com/facebook/presto/spi/function/table/OnlyPassThroughReturnTypeSpecification.java" line_range="26" />
<code_context>
+ extends ReturnTypeSpecification
+{
+ public static final OnlyPassThroughReturnTypeSpecification ONLY_PASS_THROUGH = new OnlyPassThroughReturnTypeSpecification("");
+ private static final String returnType = "PASSTRHOUGH";
+
+ @JsonCreator
</code_context>
<issue_to_address>
**issue (typo):** The return type string value appears to contain a typo that may leak into external behavior or configuration.
The constant value is spelled `"PASSTRHOUGH"` but should likely be `"PASSTHROUGH"`. Because this value is used in `StatementAnalyzer.verifyProperColumnsDescriptor` and may be persisted/serialized, the misspelling becomes part of the external contract. Unless this is intentional for backward compatibility, please correct it now to avoid locking in the typo.
```suggestion
private static final String returnType = "PASSTHROUGH";
```
</issue_to_address>
### Comment 2
<location path="presto-spi/src/main/java/com/facebook/presto/spi/function/table/GenericTableReturnTypeSpecification.java" line_range="31-32" />
<code_context>
private final ReturnTypeSpecification returnTypeSpecification;
- public AbstractConnectorTableFunction(String schema, String name, List<ArgumentSpecification> arguments, ReturnTypeSpecification returnTypeSpecification)
+ @JsonCreator
+ public AbstractConnectorTableFunction(
+ @JsonProperty("schema") String schema,
</code_context>
<issue_to_address>
**suggestion (bug_risk):** The JsonCreator argument is unused, which is confusing for both readers and Jackson configuration.
The constructor declares `@JsonProperty("returnType") String returnType` but ignores it and instead uses a static constant. This means the JSON field is effectively a no-op. If you want a singleton, consider a no-arg `@JsonCreator`; otherwise, either remove the parameter or validate that its value matches the expected constant to avoid surprising deserialization behavior.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
...main/java/com/facebook/presto/spi/function/table/OnlyPassThroughReturnTypeSpecification.java
Outdated
Show resolved
Hide resolved
...rc/main/java/com/facebook/presto/spi/function/table/GenericTableReturnTypeSpecification.java
Outdated
Show resolved
Hide resolved
aditi-pandit
force-pushed
the
tvf_update_api
branch
2 times, most recently
from
1342620 to
b92e6ba
Compare
aditi-pandit
force-pushed
the
tvf_update_api
branch
from
b92e6ba to
94e7406
Compare
jaystarshot
approved these changes
Mar 18, 2026
This was referenced Mar 31, 2026
15 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Add the JSON annotations to prepare the TableFunction SPI classes for C++ serialization
Summary by Sourcery
Annotate table function SPI types for JSON serialization and split table return type variants into dedicated classes to support polymorphic handling, updating callers and validation logic accordingly.
Enhancements:
Tests: