justaparth opened a new pull request, #41108: URL: https://github.com/apache/spark/pull/41108
https://issues.apache.org/jira/browse/SPARK-43427 <!-- Thanks for sending a pull request! Here are some tips for you: 1. If this is your first time, please read our contributor guidelines: https://spark.apache.org/contributing.html 2. Ensure you have added or run the appropriate tests for your PR: https://spark.apache.org/developer-tools.html 3. If the PR is unfinished, add '[WIP]' in your PR title, e.g., '[WIP][SPARK-XXXX] Your PR title ...'. 4. Be sure to keep the PR description updated to reflect all changes. 5. Please write your PR title to summarize what this PR proposes. 6. If possible, provide a concise example to reproduce the issue for a faster review. 7. If you want to add a new configuration, please read the guideline first for naming configurations in 'core/src/main/scala/org/apache/spark/internal/config/ConfigEntry.scala'. 8. If you want to add or modify an error type or message, please read the guideline first in 'core/src/main/resources/error/README.md'. --> ### What changes were proposed in this pull request? **Explanation** Protobuf supports unsigned integer types, including `uint32` and `uint64`. When deserializing protobuf values with fields of these types, uint32 is converted to `IntegerType` and uint64 is converted to `LongType` in the resulting spark struct. `IntegerType` and `LongType` are [signed](https://spark.apache.org/docs/latest/sql-ref-datatypes.html) integer types, so this can lead to confusing results. Namely, if a uint32 value in a stored proto is above 2^31 or a uint64 value is above 2^63, their representation in binary will contain a 1 in the highest bit, which when interpreted as a signed integer will come out as negative (I.e. overflow). I propose that we deserialize unsigned integer types into a type that can contain them correctly, e.g. uint32 => `LongType` uint64 => `Decimal(20, 0)` **Backwards Compatibility / Default Behavior** **Should we maintain backwards compatibility and we add an option that allows deserializing these types differently? OR should we change change the default behavior (with an option to go back to the old way)?** I think by default it makes more sense to deserialize them as the larger types so that it's semantically more correct. However, there may be existing users of this library that would be affected by this behavior change. Though, maybe we can justify the change since the function is tagged as `Experimental` (and spark 3.4.0 was only released very recently). **Precedent** I believe that unsigned integer types in parquet are deserialized in a similar manner, i.e. put into a larger type so that the unsigned representation natively fits. https://issues.apache.org/jira/browse/SPARK-34817 and https://github.com/apache/spark/pull/31921 ### Why are the changes needed? Improve unsigned integer deserialization behavior. ### Does this PR introduce _any_ user-facing change? Yes, as written it would change the deserialization behavior of unsigned integer field types. However, ### How was this patch tested? Unit Testing -- 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] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
