feat(extensions): support deprecation info in extensions#846
feat(extensions): support deprecation info in extensions#846nielspardon wants to merge 1 commit into
Conversation
Substrait v0.86.0 added the ability to mark simple-extension entries as deprecated (substrait-io/substrait#1014). An optional `deprecated` object (`since`, optional `reason`, optional `metadata`) can appear on types, scalar/aggregate/window functions, and individual function implementations. Parse and expose this information so tooling can surface deprecation warnings, mirroring the existing `metadata` support. Function-level deprecation propagates to each resolved variant; an impl-level deprecation takes precedence over the parent function's when both are present. Type variations are not modeled in substrait-java's extension schema, so type-variation deprecation is out of scope. Closes substrait-io#811 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
nielspardon
requested review from
andrew-coleman,
benbellick,
bestbeforetoday and
vbarua
vbarua
left a comment
vbarua
left a comment
There was a problem hiding this comment.
Left some minor comments but overall seems reasonable.
|
|
||
| /** | ||
| * Deprecation information for an extension entry (type, function, or function implementation). | ||
| * See <a href="https://github.com/substrait-io/substrait/pull/1014">substrait#1014</a>. |
There was a problem hiding this comment.
I don't think it's worth linking to the PR where this was introduced. The Javadoc should stand on it's own, and it already does.
| String name, | ||
| String description, | ||
| Optional<Map<String, Object>> metadata, | ||
| Optional<DeprecationStatus> deprecated) { |
There was a problem hiding this comment.
As this is a public API, we should note the breaking change in the description.
| .args(args()) | ||
| .options(options()) | ||
| .metadata(metadata) | ||
| .deprecated(deprecated().isPresent() ? deprecated() : deprecated) |
There was a problem hiding this comment.
If I understand what we're doing here, resolve is ScalarFunction#resolve which gets called with the function-level deprecation status. When we create the ScalarFunctionVariant, we first check if it has an impl level deprecation status, and if not just use the function-level one.
This is definitely a little lossy in terms of representation, but works reasonable well. Is the reason to inject the deprecation status like this to make it easy to check if ScalarFunctionVariant is deprecated directly without having to access the ScalarFunction holding it? If so, could we do something like inject the function-level deprecation status and store both on the ScalarFunctionImp? That way we still check the deprecation status easily with a method like isDeprecated that looks at the function-level and impl-level deprecation, but when we serialize ScalarFunctionImpl we can ignore the function-level deprecation to avoid setting it for every impl.
There was a problem hiding this comment.
This is definitely a little lossy in terms of representation, but works reasonable well. Is the reason to inject the deprecation status like this to make it easy to check if ScalarFunctionVariant is deprecated directly without having to access the ScalarFunction holding it?
I think the AI wanted to follow the existing pattern where we already copy the function name, description and metadata over from the function (ScalarFunction) to the impl (ScalarFunctionVariant). We could store a pointer to the ScalarFunction inside the ScalarFunctionVariant in general for accessing the function level metadata from the impl more easily.
There was a problem hiding this comment.
The current behavior is already lossy since it overwrites the impl description with the function description.
2 participants
Summary
Implements support for deprecation information in simple extensions, introduced in Substrait v0.86.0 (substrait-io/substrait#1014).
An optional
deprecatedobject can now be parsed and exposed on:The
deprecatedobject carries a requiredsince(core semantic version string), an optionalreason, and optionalmetadata. As with the existingmetadatasupport, consumers are not required to validate these fields — the information is surfaced so tooling can produce deprecation warnings.Type variations are not modeled in substrait-java's extension schema (
ExtensionSignaturesonly coverstypes,scalar_functions,aggregate_functions,window_functions), so type-variation deprecation is out of scope. I opened #847 to track adding type-variation support to the extension model (which would then also cover their deprecation info).Details
SimpleExtension.DeprecationStatusimmutable value type (since,reason,metadata), following the existingOption/VariadicBehaviorpattern.Optional<DeprecationStatus> deprecated()toType, theScalarFunction/AggregateFunction/WindowFunctionparent classes, and theFunctionbase class (per impl).Testing
DeprecationExtensionTest(7 tests) covering type, function-level, and impl-level deprecation, deprecationmetadata, an overload-specific deprecation where a sibling overload is not deprecated, and carry-over to aggregate-as-window functions.deprecation_extensions.yamltest resource.Closes #811
🤖 Generated with Claude Code