feat: graceful URI -> URN migration #114

benbellick · 2025-10-23T22:13:28Z

Introduces a graceful migration from the previous usage of URIs to URNs

requires extensions to present a URI explicitly and a URN inside of the yaml file Migrate extensions from URIs to URNs #95
removes the usage of uri/urn aliases in the ExtensionRegistry
introduce action to ensure examples run Add running examples to PR checks #121
introduce action to ensure generated code up to date Add code generation and verification to PR builds #120

github-actions · 2025-10-23T22:13:53Z

ACTION NEEDED

Substrait follows the Conventional Commits
specification for
release automation.

The PR title and description are used as the merge commit message. Please update your PR title and description to match the specification.

tokoko · 2025-10-24T04:26:08Z

Looks like the problem is in duckdb extension's json to substrait conversion, it is too strict and fails on unknown json fields. you can get around it by switching to use from_substrait instead of from_substrait_json in test_sql_to_substrait.py:

sql = f"CALL from_substrait(?)"
substrait_out = conn.sql(sql, params=[plan.SerializeToString()])

that being said, we should probably open a ticket on duckdb extension as well to loosen json conversion checks.

benbellick · 2025-10-24T15:30:02Z

Looks like the problem is in duckdb extension's json to substrait conversion, it is too strict and fails on unknown json fields. you can get around it by switching to use from_substrait instead of from_substrait_json in test_sql_to_substrait.py:
sql = f"CALL from_substrait(?)"
substrait_out = conn.sql(sql, params=[plan.SerializeToString()])
that being said, we should probably open a ticket on duckdb extension as well to loosen json conversion checks.

Opened up an issue here: substrait-io/duckdb-substrait-extension#166

tokoko · 2025-10-24T17:46:58Z

@benbellick Can you introduce make codegen-extensions changes also?

src/substrait/builders/extended_expression.py

tokoko

looks good.

P.S. it'd be good to run make antlr after substrait upgrades as well, but it needs some setting up (like in devcontainer) so we can do that separately.

nielspardon · 2025-10-28T09:06:21Z

P.S. it'd be good to run make antlr after substrait upgrades as well, but it needs some setting up (like in devcontainer) so we can do that separately.

I guess this step is still missing in this PR before we can merge?

Note, this is not passing any of the tests, and more work is necessary.

Had to temporarily disable the duckdb extension tests until the dependency on the duckdb-substrait-extension can handle URNs.

This PR introduces handling of migrating from the usage of URI to URN for extension references. As an intermediate step, both URI and URN and emitted from produced plans. Closes #95

the duckdb extension throws an error when unexpected fields are present in JSON on invocation of `from_substrait_json`. So we instead switch to using `from_substrait`.

It is not actually set anywhere and isn't in the substrait spec.

also added tests

benbellick · 2025-10-28T19:03:44Z

I have rebased off of main and run make antlr.

nielspardon · 2025-10-29T06:45:21Z

src/substrait/gen/proto/algebra_pb2.py

 from google.protobuf import symbol_database as _symbol_database
-from google.protobuf.internal import builder as _builder
-_runtime_version.ValidateProtobufRuntimeVersion(_runtime_version.Domain.PUBLIC, 5, 29, 5, '', 'proto/algebra.proto')


since this line got removed it seems you did not using the same protoc version as the devcontainer has been configured to use. we had the same observation in this closed PR: #113 (review)

the devcontainer uses protoc v29.5 which is it build for protobuf v5.29.5 which is the most recent compatible version with the protoletariat tool used in the proto code generation

I guess we could add a Github Actions check that runs the codegen and tests whether the git workspace does not contain any uncommitted changes to help prevent missing to run codegen with the right versions

There's too much code generation going on, we'll probably have hard time keeping track of all the versions in two places unless we build/run devcontainer in a gh action.

yes, makes sense to do it via the devcontainer by building that using the Dockerfile during Github Actions and run the codegen and verification inside the container

I created this issue for updating the github actions build: #120

if we can improve the build in this PR that would be amazing. otherwise we do it in a follow-up

nielspardon · 2025-10-29T07:03:12Z

src/substrait/gen/json/simple_extensions.py

 # generated by datamodel-codegen:
 #   filename:  simple_extensions_schema.yaml
-#   timestamp: 2025-06-06T08:43:35+00:00
+#   timestamp: 2025-10-24T17:55:30+00:00


I guess we could disable adding the timestamp in Makefile by adding the --disable-timestamp flag to make repeated make codegen-extensions runs produce reproducible outputs

Closes #120

benbellick · 2025-10-29T20:21:51Z

gen_proto.sh

 # Generate the new python protobuf files
 buf generate
-protol --in-place --create-package --python-out "$dest_dir" buf
+uv run protol --in-place --create-package --python-out "$dest_dir" buf


This was to make CI/CD not complain about missing protol

.github/workflows/codegen-check.yml

nielspardon

LGTM, thank you for also helping with adding the codegen verification

tokoko

good to merge, thanks for bearing with us :)

benbellick mentioned this pull request Oct 24, 2025

Relax JSON substrait parsing substrait-io/duckdb-substrait-extension#166

Open

2 tasks

benbellick force-pushed the uri-urn-migration branch 4 times, most recently from e3fe8cd to 6c8cddb Compare October 24, 2025 17:22

benbellick force-pushed the uri-urn-migration branch 2 times, most recently from becf09b to 70b193f Compare October 24, 2025 18:06

benbellick changed the title ~~WIP: Uri urn migration~~ feat: graceful URI -> URN migration Oct 24, 2025

benbellick force-pushed the uri-urn-migration branch 3 times, most recently from 135e586 to e24c5a9 Compare October 24, 2025 19:34

benbellick marked this pull request as ready for review October 24, 2025 19:54

tokoko reviewed Oct 24, 2025

View reviewed changes

src/substrait/builders/extended_expression.py Show resolved Hide resolved

benbellick requested a review from tokoko October 24, 2025 21:41

benbellick force-pushed the uri-urn-migration branch 2 times, most recently from d145e28 to f6201bb Compare October 25, 2025 16:45

tokoko approved these changes Oct 25, 2025

View reviewed changes

benbellick added 8 commits October 28, 2025 14:58

feat: uri -> urn grep replace + regen protos from v0.77.0

6531aa3

Note, this is not passing any of the tests, and more work is necessary.

feat: fix get urn-based substrait tests passing

e5df044

Had to temporarily disable the duckdb extension tests until the dependency on the duckdb-substrait-extension can handle URNs.

feat: graceful migration from URI -> URN

0625bbb

This PR introduces handling of migrating from the usage of URI to URN for extension references. As an intermediate step, both URI and URN and emitted from produced plans. Closes #95

fix: use from_substrait in duckdb tests to get tests passing

87bea90

the duckdb extension throws an error when unexpected fields are present in JSON on invocation of `from_substrait_json`. So we instead switch to using `from_substrait`.

cleanup

1ccee76

tweak: drop _urn_aliases

a15ecd8

It is not actually set anywhere and isn't in the substrait spec.

make codegen-extensions

8557c0c

ruff

f63a9c0

benbellick added 4 commits October 28, 2025 14:58

test cleanup

85f3043

fix examples and add CI/CD job to ensure they work

66a11f1

readd extension_uris to extended expression + add test

9a13e6e

fix remaining extended expressions missing uris + fmt

975952d

also added tests

benbellick force-pushed the uri-urn-migration branch from dce4c13 to 975952d Compare October 28, 2025 18:59

chore: make antlr

0167820

benbellick requested review from nielspardon and tokoko and removed request for tokoko October 28, 2025 19:03

nielspardon reviewed Oct 29, 2025

View reviewed changes

nielspardon mentioned this pull request Oct 29, 2025

Add code generation and verification to PR builds #120

Closed

benbellick added 2 commits October 29, 2025 14:31

feat: CICD check for generated code

804655b

Closes #120

fix: correct codegen

9e7cb06

benbellick force-pushed the uri-urn-migration branch from b29f1d2 to 9e7cb06 Compare October 29, 2025 18:31

benbellick added 2 commits October 29, 2025 16:04

fix: update gen_proto.sh to use uv run protol

6c357ad

chore: regenerate generate code

d2f638a

benbellick force-pushed the uri-urn-migration branch from 251389e to d2f638a Compare October 29, 2025 20:07

benbellick commented Oct 29, 2025

View reviewed changes

.github/workflows/codegen-check.yml Outdated Show resolved Hide resolved

benbellick requested a review from nielspardon October 29, 2025 20:24

fix: drop timestamp from codegen

44b68f4

nielspardon approved these changes Oct 30, 2025

View reviewed changes

tokoko approved these changes Oct 30, 2025

View reviewed changes

benbellick merged commit 890f84b into main Oct 30, 2025
23 checks passed

feat: graceful URI -> URN migration #114

feat: graceful URI -> URN migration #114

Uh oh!

Conversation

benbellick commented Oct 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Oct 23, 2025

Uh oh!

tokoko commented Oct 24, 2025

Uh oh!

benbellick commented Oct 24, 2025

Uh oh!

tokoko commented Oct 24, 2025

Uh oh!

Uh oh!

tokoko left a comment

Choose a reason for hiding this comment

Uh oh!

nielspardon commented Oct 28, 2025

Uh oh!

benbellick commented Oct 28, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

nielspardon left a comment

Choose a reason for hiding this comment

Uh oh!

tokoko left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

benbellick commented Oct 23, 2025 •

edited

Loading