Skip to content
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.

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

Break down select output into composable components #33

Merged
merged 2 commits into from
Sep 29, 2023

Conversation

gmorenz
Copy link
Contributor

@gmorenz gmorenz commented Sep 29, 2023

This fixes selecting for Option<primitive type> and Vec<primitive type>. It does so by breaking down the code gen for select into three components, one small snippet for how each row is handled depending on content, one small snippet for how the iterator over rows is handled depending on container, and one shared snippet that puts it all together.

The code it generates isn't identical to the original code, but I believe it's logically equivalent. Except for

  • The two cases mentioned above that weren't handled correctly before
  • Properly classifying u8 as a primitive type (it was missing from the list).
  • Collecting the rows iterator directly into the Vec<OutputType> instead of first allocating an intermediate Vec with Results un-flattened.

I uncommented some existing tests that checked these for correctness, as well as adding two additional tests for Option<CustomStruct> and Vec<CustomStruct> to complete the grid of possibilities (those tests pass before the changes as well).

I believe this bumps MSRV to 1.70, and it probably requires a CI change as a result, but before figuring that out I wanted to make sure you were ok with bumping the MSRV. I'm using it for Option::is_some_and (that I know about, I'm generally not very careful about not using new features so it wouldn't surprise me to find out I was using something else that bumped MSRV as well). If you're worried about that I can easily rewrite it to use older and only slightly clunkier syntax.

Thanks for a cool library :)

Fixes selecting for Option<primitive type> and Vec<primitive type>.
@trevyn
Copy link
Owner

trevyn commented Sep 29, 2023

Thanks for the PR! I’ll take a closer look in a few hours, but yes, an MSRV bump to 1.70 is fine.

@trevyn
Copy link
Owner

trevyn commented Sep 29, 2023

This looks clean, thank you! I believe the previous behavior of Vec<u8> was awkwardly special-cased to interpret as a single row blob-like field (hence u8 missing from the primitive types list), but it looks like I didn't have a test in there and might never have mentioned its behavior, so that's on me! Hopefully your motivation for this PR wasn't using Vec<u8> as a multi-row select? 😟 In which case, I'm all ears on how we should handle this...

@trevyn trevyn merged commit 9826618 into trevyn:main Sep 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants