8000 [Dev] Make the various mappings in `MultiFileReaderData` typesafe by Tishj · Pull Request #16596 · duckdb/duckdb · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

[Dev] Make the various mappings in MultiFileReaderData typesafe #16596

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 16 commits into from
Mar 13, 2025
Merged

[Dev] Make the various mappings in MultiFileReaderData typesafe #16596

merged 16 commits into from
Mar 13, 2025

Conversation

Tishj
Copy link
Contributor
@Tishj Tishj commented Mar 11, 2025 •
edited
Loading

This PR does not change anything functionality-wise

In the MultiFileReaderData we have many mappings, used in various contexts, and many of these mappings are tied to each other - if one changes, the other should also change.

All of these were using idx_t, which made it very confusing and error-prone to use and understand what type of index is required and produced.

This PR introduces the following structs as typesafe replacements:

  • MultiFileLocalColumnId
  • MultiFileLocalIndex
  • MultiFileGlobalIndex
  • MultiFileConstantMapIndex
  • MultiFileLocalColumnIds
  • MultiFileColumnMapping
  • MultiFileFilterMap
  • MultiFileConstantMap
  • MultiFileCastMap

column_ids, column_mapping, filter_map, constant_map and cast_map are indexed with and produce the typed indexes
MultiFileFilterEntry has been changed to produce MultiFileConstantMapIndex or MultiFileLocalIndex depending on the way it was set.

Tishj added 11 commits March 7, 2025 11:07
@Tishj
use an unordered_map instead of a vector for 'constant_map'
141c302
@Tishj
also remove 'MultiFileFilterEntry', not needed
a46d254
@Tishj
fix the tidy issues
c5f3221
@Tishj
added '[global|local]_idx_t' and '[global|local]_column_id_t' typedef…
29d5ecc 
…s to help understand the various contexts that idx_ts are used in, mostly to figure out what is impacted and how when we rework this to create BoundExpressions instead and apply those in FinalizeChunk
@Tishj
undo changes, replace loops with std::find_if
9d91d45
@Tishj
undo everything, std::find_if is not much better looking than the alt…
2b37b7f 
…ernative
@Tishj
Merge branch 'multi_file_reader_constant_map_actually_a_map' into mul…
61c4c75 
…ti_file_reader_mapping_demystification
@Tishj
undo stuff
8000
b5c934d
@Tishj
add the typedefs in more places, clean up a bit
9e786b6
@Tishj
more clean up
517a1e5
@Tishj
realize global_column_id_t is entirely unused for now..
3d8948b
@Tishj
Copy link
Contributor Author
Tishj commented Mar 11, 2025 •
edited
Loading

In the future perhaps we want to look into making these actual types, so the type system will actually help in disallowing indexes to not be used in the wrong contexts, but for now I think the semantic changes should help with development / reasoning about the code.

This PR now also does this proposed next step of making it actually typesafe

For example the logic in ParquetReader::ScanInternal is a lot clearer now, how given the filter's (global) filter_idx we look up the MultiFileFilterEntry, which gives us the local idx, which we use to find the column_id from the column_ids and also the (global) result_idx (which we can now see is redundant because that should be the same as filter_idx).

@Tishj Tishj mentioned this pull request Mar 11, 2025
Closed
@duckdb-draftbot duckdb-draftbot marked this pull request as draft March 12, 2025 14:10
@Tishj
make the filter entry also typesafe, it can be set with either a cons…
055d6d8 
…tant index or a local index, and retrieved in the same way
@Tishj Tishj changed the title [Dev] Add some typedefs to distinguish between the various indexes used in the MultiFileReaderData [Dev] Make the various mappings in MultiFileReaderData typesafe Mar 12, 2025
@Tishj Tishj marked this pull request as ready for review March 12, 2025 14:45
Mytherin
Mytherin approved these changes Mar 12, 2025
View reviewed changes
Copy link
Collaborator
@Mytherin Mytherin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Looks good to me (after CI passes).

@Tishj Tishj marked this pull request as draft March 12, 2025 15:46
@Tishj Tishj marked this pull request as ready for review March 12, 2025 15:46
@Mytherin Mytherin merged commit 81a9e52 into duckdb:main Mar 13, 2025
49 of 50 checks passed
@Mytherin
Copy link
Collaborator

Thanks!

krlmlr added a commit to duckdb/duckdb-r that referenced this pull request May 15, 2025
@krlmlr
vendor: Update vendored sources to duckdb/duckdb@81a9e52
73f353b 
[Dev] Make the various mappings in `MultiFileReaderData` typesafe (duckdb/duckdb#16596)
Issue template: direct UI issues to the UI repository (duckdb/duckdb#16619)
krlmlr added a commit to duckdb/duckdb-r that referenced this pull request May 15, 2025
@krlmlr
vendor: Update vendored sources to duckdb/duckdb@81a9e52
ddfd8d6 
[Dev] Make the various mappings in `MultiFileReaderData` typesafe (duckdb/duckdb#16596)
Issue template: direct UI issues to the UI repository (duckdb/duckdb#16619)
krlmlr added a commit to duckdb/duckdb-r that referenced this pull request May 16, 2025
@krlmlr
vendor: Update vendored sources to duckdb/duckdb@81a9e52
24d6ee1 
[Dev] Make the various mappings in `MultiFileReaderData` typesafe (duckdb/duckdb#16596)
Issue template: direct UI issues to the UI repository (duckdb/duckdb#16619)
krlmlr added a commit to duckdb/duckdb-r that referenced this pull request May 17, 2025
@krlmlr
vendor: Update vendored sources to duckdb/duckdb@81a9e52
bf84929 
[Dev] Make the various mappings in `MultiFileReaderData` typesafe (duckdb/duckdb#16596)
Issue template: direct UI issues to the UI repository (duckdb/duckdb#16619)
krlmlr added a commit to duckdb/duckdb-r that referenced this pull request May 18, 2025
@krlmlr
vendor: Update vendored sources to duckdb/duckdb@81a9e52
296b038 
[Dev] Make the various mappings in `MultiFileReaderData` typesafe (duckdb/duckdb#16596)
Issue template: direct UI issues to the UI repository (duckdb/duckdb#16619)
krlmlr added a commit to duckdb/duckdb-r that referenced this pull request May 18, 2025
@krlmlr
vendor: Update vendored sources to duckdb/duckdb@81a9e52
c6fd869 
[Dev] Make the various mappings in `MultiFileReaderData` typesafe (duckdb/duckdb#16596)
Issue template: direct UI issues to the UI repository (duckdb/duckdb#16619)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Mytherin Mytherin Mytherin approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@Tishj @Mytherin
0