mkzung commented on PR #50475: URL: https://github.com/apache/arrow/pull/50475#issuecomment-5071370165
Thanks for the review. I folded the index-type tests down to the distinct code paths - type and finish preservation now run in one loop over signed and unsigned, and I dropped the ordered and fixed-size-binary ones since they don't exercise a separate path. Switched to ASSERT_OK_AND_ASSIGN and reworded the context-dependent comments. Moved MaybeUnsignedIndexType into builder.cc and renamed the flag to use_unsigned_index. On uint64 to pandas: I added support for it instead of keeping the error. uint64 indices map to int64 codes, which is safe because the indices are bounds-checked below the dictionary length, so they never reach the int64 limit. WriteIndicesUniform handles it the same way as uint32, and I updated test_dictionary_with_pandas to check the conversion instead of the raise. On the missing top bit: you're right, the adaptive builder is signed so a uint8 index widens after 128 rather than 256. I left that as-is and noted it in the builder.cc comment - happy to change the builder to track the unsigned range if you'd prefer. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
