Closed gmorenz closed 1 year ago
Thanks for the PR! I’ll take a closer look in a few hours, but yes, an MSRV bump to 1.70 is fine.
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...
This fixes selecting for
Option<primitive type>
andVec<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 oncontent
, one small snippet for how the iterator over rows is handled depending oncontainer
, 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
Vec<OutputType>
instead of first allocating an intermediateVec
withResults
un-flattened.I uncommented some existing tests that checked these for correctness, as well as adding two additional tests for
Option<CustomStruct>
andVec<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 :)