Support correct output column names and struct field names when consuming/producing Substrait #10829
Conversation
This file contains 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
alamb
approved these changes
Jun 10, 2024
| \n TableScan: data projection=[a]", | ||
| false // NULL vs Int64(NULL) | ||
| true |
| false, // "List(..)" vs "make_array(..)" | ||
| ) | ||
| .await | ||
| roundtrip("SELECT [[1,2,3], [], NULL, [NULL]] FROM data").await |
There was a problem hiding this comment.
I verified that roundtrip actually is more stringent than assert_expected_plan as it checks the plan before and after roundtripping 👍
Blizzara
force-pushed
the
avo/substrait-consumer-fix-output-column-names
branch
from
e4b4b3d
to
6c1bb88
Compare
(assert_eq gives nicer error messages than assert)
|
🚀 -- thanks again @Blizzara |
5 tasks
findepi
pushed a commit
to findepi/datafusion
that referenced
this pull request
Jul 16, 2024
…ming/producing Substrait (apache#10829) * produce flattened list of names including inner struct fields * add a (failing) test * rename output columns (incl. inner struct fields) according to the given list of names * fix a test * add column names project to the new TPC-H test and fix case (assert_eq gives nicer error messages than assert)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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?
Closes #10817
Rationale for this change
What changes are included in this PR?
nameslistAre these changes tested?
Tested by a new test specifically for testing Alias (" AS "), as well as tightening existing tests.
Are there any user-facing changes?
Plans produced by DataFusion before this change are not compatible with plans produced after this change, if the plan contains inner struct fields, since the length of the "names" field does not match. The previous behavior was wrong, but still it's a breaking change. If that's considered important, it is possible to add an extra handling for those cases, but I'd prefer not to unless necessary.
Also some output column names might change if people are depending on the old behavior that did not rename them.