Use correct output column id list for query processing #195
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
JelteF
requested changes
Sep 19, 2024
JelteF
reviewed
Sep 19, 2024
b1ebe29 to
49f9c50
Compare
618277c to
7303512
Compare
JelteF
previously approved these changes
Sep 20, 2024
src/pgduckdb_types.cpp
Outdated
|
|
||
| bool valid_tuple = true; | ||
|
|
||
| for (auto const &[columnIdx, valueIdx] : scan_global_state->m_columns) { | ||
| /* First we are fetching all required columns oredered by column id |
There was a problem hiding this comment.
Suggested change
| /* First we are fetching all required columns oredered by column id | |
| /* First we are fetching all required columns ordered by column id |
Comment on lines
+62
to
+63
| -- Column ids list used because both of fetched column are used after scan | ||
| SELECT a, b FROM query_filter_output_column WHERE b = 't1'; |
There was a problem hiding this comment.
From your comments and my closer reading of the relevant duckdb code, we are expected to change the handle the column order changes. So let's include that in one of these tests
Suggested change
| -- Column ids list used because both of fetched column are used after scan | |
| SELECT a, b FROM query_filter_output_column WHERE b = 't1'; | |
| -- Column ids list used because both of fetched column are used after scan, we also should swap column order correctly | |
| SELECT b, a FROM query_filter_output_column WHERE b = 't1'; |
There was a problem hiding this comment.
Let's also add a test where we only re-order columns, but still output all of them.
* When reading postgres tables we need to know which columns need to to written to output vector. Based on filtering and query we need to either get this information from `projection_ids` or `columns_ids` list. Projection ids list will be used when that are fetched columns from table but those columns are not needed in further query processing. Otherwise columns_ids list will be used.
7303512 to
689cec9
Compare
JelteF
approved these changes
Sep 20, 2024
|
I get this crash on |
|
e4b914a (the parent of this PR) does not have this issue |
This was referenced Sep 24, 2024
Tishj
added a commit
that referenced
this pull request
Sep 26, 2024
This PR fixes the issue mentioned in #195 (comment) Fix found by Mario. I don't think we quite understand why this caused a crash/bad access violation on Mac but Linux was unaffected. either way this is now fixed Some debugging: ``` (lldb) p scan_global_state->m_output_columns_ids.size() (std::map<unsigned long long, unsigned long long>::size_type) 2 (lldb) p scan_global_state->m_read_columns_ids.size() (std::map<unsigned long long, unsigned long long>::size_type) 3 ``` I imagine this wrote to memory we do own, and is managed by duckdb, containing a pointer value that got overwritten, when it gets dereferenced it segfaults because the address is bogus The difference in system allocators likely caused this problem to fly under the radar on Linux
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.
written to output vector. Based on filtering and query we need to
either get this information from
projection_idsorcolumns_idslist. Projection ids list will be used when that are fetched columns
from table but those columns are not needed in further query processing.
Otherwise columns_ids list will be used.