fix: report deprecated URI extension fields clearly - #99
Conversation
3596c67 to
6e9856b
Compare
6e9856b to
d96893a
Compare
d96893a to
a20acf9
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d96893ac11
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| errors.push(e); | ||
| } | ||
| } | ||
| Some(ExtractedMapping::DeprecatedUriOnly) => {} |
There was a problem hiding this comment.
Preserve URI-only mappings for function-name lookup
Dropping DeprecatedUriOnly declarations in from_extensions removes anchor-to-name entries entirely, so deprecated URI-based plans that actually call those functions now lose best-effort textification (NamedAnchor::lookup falls back to MissingAnchor, producing error tokens instead of function names). Before this change, add_extension still inserted the mapping even when it emitted MissingUrn, which preserved readable function names; this regression specifically affects older plans using extensionUriReference plus function calls.
Useful? React with 👍 / 👎.
Description
Closes #96 by replacing the misleading
Missing URN anchor 0failure mode for deprecated URI-based extension fields with explicit formatter diagnostics.Older hard-coded Substrait JSON can still contain the deprecated
extensionUrisandextensionUriReferencefields. This crate does not normalize those fields into the current URN-based model; instead, textification reports them through the existing formatting-issues channel so callers can fix the JSON input. Relation text still renders best-effort when possible.This PR also avoids building a fake
@0extension mapping from URI-only declarations, so the old confusing error does not get emitted alongside the clearer deprecated-field diagnostics.Testing
RUSTC_WRAPPER= cargo test deprecated_extension_uris -- --nocaptureRUSTC_WRAPPER= cargo test test_from_extensions_skips_deprecated_uri_only_mappings -- --nocaptureRUSTC_WRAPPER= just checkRUSTC_WRAPPER= cargo test --docRUSTC_WRAPPER= just testRelated Issues
Closes #96