Fix bug in field level metadata matching code #8286
Merged
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
commented
Nov 20, 2023
| .ok() | ||
| .map(|f| f.metadata().clone()) | ||
| // Look up field by index in schema (not NAME) | ||
| e.as_any() |
There was a problem hiding this comment.
This is the fix -- look up metadata by column index rather than name
alamb
commented
Nov 20, 2023
| @@ -299,3 +299,31 @@ fn table_with_many_types() -> Arc<dyn TableProvider> { | |||
| let provider = MemTable::try_new(Arc::new(schema), vec![vec![batch]]).unwrap(); | |||
| Arc::new(provider) | |||
| } | |||
|
|
|||
| /// Registers a table_with_metadata that contains both field level and Table level metadata | |||
| pub async fn register_metadata_tables(ctx: &SessionContext) { | |||
There was a problem hiding this comment.
I intend to port several existing metadata tests to sqllogictest as well
comphead
approved these changes
Nov 20, 2023
comphead
reviewed
Nov 20, 2023
| e.as_any() | ||
| .downcast_ref::<Column>() | ||
| .map(|column| column.index()) | ||
| .map(|idx| input_schema.field(idx).metadata()) |
There was a problem hiding this comment.
this can likely be done within a single map?
There was a problem hiding this comment.
good idea -- done in f5c557e
|
Thank you for the review @comphead |
alamb
added a commit
to alamb/datafusion
that referenced
this pull request
Nov 21, 2023
* Fix bug in field level metadata matching code * improve comment * single map
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 #8285
Rationale for this change
Bug fix
What changes are included in this PR?
What was happening is that the input to a join has metadata on one side, but not the other. When the inputs were reordered then metadata was not properly reordered as well
Specifically:
So the output schema looks like:
When the join reorders the inputs, the result had the metadata incorrectly set:
There is code in the join reordering that tries to remap the output so the schema is the same, however
it had a bug (was looking up field by name, not column index), so it picked the wrong column.
Are these changes tested?
Are there any user-facing changes?