feat: support Placeholder <-> DynamicParameter in Substrait producer/consumer#20976
Closed
bvolpato-dd wants to merge 1 commit intoapache:mainfrom
Closed
feat: support Placeholder <-> DynamicParameter in Substrait producer/consumer#20976bvolpato-dd wants to merge 1 commit intoapache:mainfrom
bvolpato-dd wants to merge 1 commit intoapache:mainfrom
Conversation
…consumer Add support for converting DataFusion's Expr::Placeholder to Substrait's DynamicParameter and back. This enables roundtripping query plans that contain parameterized expressions (e.g. $1, $2) through the Substrait serialization layer. Producer: new from_placeholder function maps one-based $N placeholder ids to zero-based Substrait parameter_reference values with optional type info. Consumer: consume_dynamic_parameter converts back from Substrait DynamicParameter to DataFusion Placeholder expressions. Tests cover SQL-based roundtrips (untyped placeholders in filter and projection), typed placeholders (Int64, Utf8, Decimal128), multiple placeholders, and Substrait proto-level DynamicParameter verification.
Contributor
Author
|
Superseded by #20977, my bad! |
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
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.
Which issue does this PR close?
Rationale for this change
The Substrait producer previously returned
not_impl_err!forExpr::Placeholder, meaning any query plan containing parameterized expressions (e.g.$1,$2) could not be serialized to Substrait. This gap prevented roundtripping prepared statement plans through the Substrait layer.What changes are included in this PR?
Producer:
placeholder.rsmodule withfrom_placeholderthat convertsExpr::PlaceholdertoDynamicParameter, mapping one-based$NDataFusion ids to zero-based Substraitparameter_referencevalues with optional type information.handle_placeholderadded to theSubstraitProducertrait.Consumer:
consume_dynamic_parameterconverts SubstraitDynamicParameterback toExpr::Placeholder, reversing the index mapping and type conversion.Are these changes tested?
Yes. Five new integration tests and one unit test:
roundtrip_placeholder_sql_filter— SQL-based,WHERE a > $1roundtrip_placeholder_sql_projection— SQL-based,$1in SELECT +$2in WHEREroundtrip_placeholder_typed_int64— typed Int64 placeholder with proto-levelDynamicParameterverificationroundtrip_placeholder_multiple_typed— two typed placeholders (Int64 + Decimal128)roundtrip_placeholder_typed_utf8— Utf8 typed placeholdertest_parse_placeholder_index— unit test for index parsing edge casesAll 190 integration tests, 28 unit tests, and 3 doc-tests pass. Clippy and fmt are clean.
Are there any user-facing changes?
Query plans containing
Placeholderexpressions can now be serialized to and deserialized from Substrait format. Previously this would return an error.