Repository navigation
feat!: support plan extensions - #70
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (11)
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe PR adds plan-level metadata for advanced extensions and expected type URLs. Plans expose this metadata, ChangesPlan extension metadata
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~25 minutes Change: Feature Sequence Diagram(s)sequenceDiagram
participant ProtobufPlan
participant ProtoToPlanConverter
participant Plan
participant PlanToProtoConverter
participant SerializedPlan
ProtobufPlan->>ProtoToPlanConverter: advanced extensions and expected type URLs
ProtoToPlanConverter->>Plan: construct with captured PlanMetadata
Plan->>PlanToProtoConverter: provide plan metadata
PlanToProtoConverter->>SerializedPlan: write advanced extensions and expected type URLs
Suggested reviewers: Merge Risk: ⚪ Minimal · up to Plan-level extensions and expected type URLs are preserved through editing and protobuf conversion, with no actionable regression identified. The change is ready to merge after normal checks. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The reviewed paths preserve extension data without interpreting or executing it, and metadata edits protect previously built plans. Remaining risk is concentrated in compatibility with custom plan implementations and how downstream consumers handle newly preserved payloads. Retained concerns Security review detailsSecurity Blast Radius
Security Findings and Attack Paths
Trust Boundaries and Controls
Resilience and Maintainability Implications
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Full details: Out of Scope Changes checkExplanation The incremental changes add custom relation support through Full details: Docstring CoverageExplanation Docstring coverage is 54.10% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 61 functions across 11 files. (4 skipped: 4 unsupported.)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Warning Some tools did not complete. Review the errors below. 🔧 LanguageToolLanguageTool checks are incomplete after three consecutive availability failures. Remaining chunks and files were skipped; findings from completed checks are retained. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Preserve opaque plan-level advanced extensions and expected type URLs, with immutable snapshots and focused builder editing methods. BREAKING CHANGE: Custom IPlan implementations must provide a non-null PlanMetadata Metadata property and recompile. Implementations without plan extensions can return PlanMetadata.Empty. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
03ac695 to
c4407f6
Compare
2 participants
Thank you for submitting a pull request to the Substrait project. Please keep
your description clear and concise for reviewers.
Change Summary [REQUIRED]
Support plan-level advanced extensions and expected type URLs through immutable
metadata, round-trip conversion, and focused editing methods on
PlanBuilder.Motivation [REQUIRED]
Fixes #69.
Plan conversion currently drops these fields. Callers also need to set and edit
extensions during composition without constructing immutable metadata upfront
or changing previously built plans.
Reviewer Context [OPTIONAL]
ReadOnlyAdvancedExtensionfacade. Opaquepayloads, unknown fields, absent versus empty messages, and ordered duplicate
type URLs survive conversion without interpretation.
unchanged, and repeated builds reuse unchanged metadata. Failed edits leave
the builder's metadata intact.
ReadOnlyAnyorIMessage: ordinary messages arepacked immediately, while existing
Anymessages are copied without repacking.Whole-message replacement supports both read-only and protobuf extensions.
ClearAdvancedExtensions()removes that message; expected type URLs are editedindependently and are never inferred from payloads.
payload descriptors. Simple function/type extension declarations are unchanged.
Validation [REQUIRED]
dotnet format Substrait.sln --verify-no-changes --no-restore— Passed.dotnet build Substrait.sln --configuration Release --no-restore— Passed on all targets, includingnetstandard2.0.dotnet test --solution Substrait.sln --configuration Release --no-build— Passed: 3,888 tests, zero failures or skips, covering .NET 8 and .NET 10.Public API and compatibility [REQUIRED]
Adds
PlanMetadata,IPlan.Metadata, metadata-first plan construction, andbuilder methods for setting, adding, replacing, removing, and clearing payloads
and expected type URLs.
PlanBuilder.Metadataexposes an immutable snapshot forinspection. Existing concrete plan and builder constructors remain available,
including
new PlanBuilder(null).Plan equality and hashing include metadata. No specification, dependency, or
generated-facade changes are required. The public API baseline and documentation
include the additions and migration guidance.
Breaking changes [REQUIRED]
BREAKING CHANGE: Custom
IPlanimplementations must provide a non-nullPlanMetadata Metadataproperty and recompile. Implementations without plan-levelextensions can return
PlanMetadata.Empty. Existing concretePlanconstructorsdefault to empty metadata; serializers explicitly reject null metadata.
Summary by CodeRabbit