feat(core)!: bump substrait to v0.85.0, drop URI support#740
feat(core)!: bump substrait to v0.85.0, drop URI support#740benbellick wants to merge 12 commits intomainfrom
Conversation
There was a problem hiding this comment.
I think these roundtrip tests are still valuable, even if we aren't testing the migration.
Add test showing plans with unknown URNs are accepted when those functions aren't referenced by relations.
benbellick
changed the title
| } | ||
|
|
||
| public static Builder builder() { | ||
| return builder(DefaultExtensionCatalog.DEFAULT_COLLECTION); |
There was a problem hiding this comment.
We used to need this to be able to access the URI<->URN mapping. It was a bit of a hack before but was necessary during the migration.
| Map<Integer, String> uriMap = new HashMap<>(); | ||
|
|
||
| // Handle URN format | ||
| // TODO: consider validating URN format (e.g. must start with "extension:") |
There was a problem hiding this comment.
I think it could be reasonable to validate at least extension:<namespace>:<name> here, but I figure that can be left for another PR where it can be properly discussed.
There was a problem hiding this comment.
if we want to validate the URN format we may want to add a regexp pattern to the extension YAML schema in the spec:
- https://github.com/substrait-io/substrait/blob/2aaae7c0d369a23b5b469badce0644097f1769a1/text/simple_extensions_schema.yaml#L8-L9
- https://json-schema.org/understanding-json-schema/reference/regular_expressions#example
I would agree that is a separate change. Do you want to create an issue for this?
There was a problem hiding this comment.
I have this PR open where there is some discussion about this. It seemed to stall a bit and it was relatively low priority on my end, so I decided to prioritize other things. However, I would love to see some kind of resolution there :)
| * | ||
| * <p>Used by {@link AbstractExtensionLookup#getScalarFunction} and similar methods to resolve a | ||
| * {@link FunctionAnchor} into a complete {@link Function} with signature metadata. | ||
| */ |
There was a problem hiding this comment.
This wasn't strictly necessary for this PR, but I get a bit confused in this codebase on the differences between SimpleExtension, ExtensionCollector, ExtensionCollection, and ImmutableExtensionLookup. I wanted to move in the direction of clearer documentation on these differences.
There was a problem hiding this comment.
This test had to do with the management of the URI<->URN mapping when there was an implied discrepancy of the mapping from the YAML files. Since there is no more mapping, these tests are useless now.
There was a problem hiding this comment.
This is a new test but testing normally expected behavior, which is just that when we look things up in the ImmutableExtensionLookup, they are indeed there.
There was a problem hiding this comment.
Yeah just added this roundtrip tests (as mentioned in another comment) because we already had those nice plans, and I think this is a good category of tests to have in general.
2 participants
Completes the URI-to-URN migration started in #522.
Substrait spec v0.85.0 removed the deprecated URI fields from the proto definitions (
SimpleExtensionURI,extension_uris,extension_uri_reference) in favor ofSimpleExtensionURN,extension_urns, andextension_urn_reference(see substrait#971). This PR bumps the submodule and drops all URI handling from substrait-java to match.Previously,
SimpleExtensionmaintained a bidirectional URI-to-URN map so that plans could accept either format on input and emit both on output.ExtensionCollectorandImmutableExtensionLookupboth dealt with URI fields alongside URN fields during serialization and deserialization. With the upstream removal, all of that is unnecessary. The extension system now works exclusively with URNs.