feat: move the output schema from Substrait's RelRoot into an extension #11033
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- Removed output_schema field from RelRoot in algebra.proto - Added advanced_extension field to RelRoot - Created gluten_extensions.proto with RelRootOutputSchema extension - Updated ClickHouse parser to read output_schema from extension - Updated Java PlanNode to pack output_schema into extension - Updated documentation to reflect the change The output_schema field was only used by the ClickHouse backend to preserve nullability information. By moving it to an extension, we follow Substrait best practices for backend-specific functionality.
- Moved proto file from substrait/proto/substrait/extensions/ to io/glutenproject/proto/ - Changed package from 'substrait.extensions' to 'gluten' - Updated Java package to 'io.glutenproject.proto' - Updated C++ includes from substrait/extensions/gluten_extensions.pb.h to gluten_extensions.pb.h - Updated namespace references from substrait::extensions to gluten in C++ code - Updated Java imports to use io.glutenproject.proto.GlutenExtensions This keeps Gluten-specific extensions separate from upstream Substrait proto files and follows the existing Gluten proto conventions.
Renamed RelRootOutputSchema to CHExpectedOutputSchema and improved documentation to make it clear this is a ClickHouse-specific workaround, not a general output schema mechanism. Key clarifications: - The output schema can already be computed from any Substrait plan - This extension exists because ClickHouse's plan conversion doesn't preserve nullability correctly (issue-1874) - Rather than fix ClickHouse's complex conversion logic, we provide the expected schema so ClickHouse can insert casts to correct nullability mismatches - Not needed by backends like Velox that preserve types correctly Changes: - Renamed message from RelRootOutputSchema to CHExpectedOutputSchema - Renamed field from output_schema to expected_schema - Added detailed comments explaining the workaround nature - Updated all Java and C++ code references - Enhanced documentation to explain this is not a general feature - Improved error messages and comments in C++ code
|
Run Gluten Clickhouse CI on x86 |
Contributor
Author
|
Will solve this by removing output_schema instead. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Moved the output schema used to solve the ClickHouse output schema nullability mismatch into an extension instead of being a core feature of Substrait. This helps align the Gluten fork of Substrait with that of the core project.