Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: factor out advanced_extension logic and add it to other ops #65

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

ingomueller-net
Copy link
Collaborator

@ingomueller-net ingomueller-net commented Jan 21, 2025

This PR factors out the handling of the shared_extension field from the plan op and adds that logic to the project op. This mainly consisted of moving the import and export logic from the functions related to the plan op to dedicated functions. The PR also introduces the new ExtensibleOpInterface that enforces an attribute called advanced_extension on the op that implement it and allows to deal with all such ops transparently. Since that interface depends on an attribute, the include order of the generated code of interfaces and attributes also had to be adapted. Unfortunately, the field names in the Substrait spec also vary (singular or plural), so the PR also introduces some template magic to be able to deal with protobuf message types with both spellings. With this PR, message types with an advanced_extension field should be able to support it by (1) adding the ExtensibleOpInterface to their traits, (2) adding an advanced_extension parameter, and (3) adding that parameter to their assembly format (although that's technically optional; otherwise, the attribute is set through the attributes dictionary).

This PR factors out the handling of the `shared_extension` field from
the `plan` op and adds that logic to the `project` op. This mainly
consisted of moving the import and export logic from the functions
related to the `plan` op to dedicated functions. The PR also introduces
the new `ExtensibleOpInterface` that enforces an attribute called
`advanced_extension` on the op that implement it and allows to deal with
all such ops transparently. Since that interface depends on an
attribute, the include order of the generated code of interfaces and
attributes also had to be adapted. Unfortunately, the field names in the
Substrait spec also vary (singular or plural), so the PR also introduces
some template magic to be able to deal with protobuf message types with
both spellings. With this PR, message types with an `advanced_extension`
field should be able to support it by (1) adding the
`ExtensibleOpInterface` to their traits, (2) adding an
`advanced_extension` parameter, and (3) adding that parameter to their
assembly format (although that's technically optional; otherwise, the
attribute is set through the `attributes` dictionary).
@ingomueller-net ingomueller-net changed the title feat: factor out advanced_extension logic and add it to other ops [WIP] feat: factor out advanced_extension logic and add it to other ops Jan 22, 2025
@ingomueller-net ingomueller-net force-pushed the advanced-extension-in-rels branch from 5539f62 to 8c74e7b Compare January 22, 2025 14:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant