You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "ming08108 (via GitHub)" <gi...@apache.org> on 2023/06/14 02:00:15 UTC
[GitHub] [arrow-rs] ming08108 opened a new pull request, #4412: Fix bad cast in create_primitive_array
ming08108 opened a new pull request, #4412:
URL: https://github.com/apache/arrow-rs/pull/4412
# Which issue does this PR close?
N/A
# Rationale for this change
Fixes a bad cast in create_primitive_array.
# What changes are included in this PR?
Removes bad casts in create_primitive_array.
# Are there any user-facing changes?
N/A
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] ming08108 commented on a diff in pull request #4412: Remove 64-bit to 32-bit Cast from IPC Reader
Posted by "ming08108 (via GitHub)" <gi...@apache.org>.
ming08108 commented on code in PR #4412:
URL: https://github.com/apache/arrow-rs/pull/4412#discussion_r1230300405
##########
arrow-ipc/src/reader.rs:
##########
@@ -250,24 +242,23 @@ fn create_primitive_array(
| UInt32
| Time32(_)
| Date32
- | Interval(IntervalUnit::YearMonth) => {
- if buffers[1].len() / 8 == length && length != 1 {
Review Comment:
Encountered this when reading a zero copy sliced RecordBatch from c++. If an int32 array was sliced to half its original size it would trigger this since the original backing buffer remained at its original size in the ipc message.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] tustvold merged pull request #4412: Remove 64-bit to 32-bit Cast from IPC Reader
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold merged PR #4412:
URL: https://github.com/apache/arrow-rs/pull/4412
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] tustvold commented on pull request #4412: Remove 64-bit to 32-bit Cast from IPC Reader
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on PR #4412:
URL: https://github.com/apache/arrow-rs/pull/4412#issuecomment-1592435647
Can you also remove the Float32 case, if all the tests and integration tests still pass I'll be happy to merge this
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] ming08108 commented on a diff in pull request #4412: Fix bad cast in create_primitive_array
Posted by "ming08108 (via GitHub)" <gi...@apache.org>.
ming08108 commented on code in PR #4412:
URL: https://github.com/apache/arrow-rs/pull/4412#discussion_r1228887450
##########
arrow-ipc/src/reader.rs:
##########
@@ -250,24 +242,23 @@ fn create_primitive_array(
| UInt32
| Time32(_)
| Date32
- | Interval(IntervalUnit::YearMonth) => {
- if buffers[1].len() / 8 == length && length != 1 {
Review Comment:
I think this logic is incorrect. https://arrow.apache.org/docs/format/Columnar.html#fixed-size-primitive-layout suggests that fixed sized types Int32 are always 4 bytes.
I also checked against arrow2 https://github.com/jorgecarleitao/arrow2/blob/2d2e7053f9a50810bfe9cecff25ab39089aef98e/src/array/primitive/mod.rs and didn't see any similar logic.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] tustvold commented on a diff in pull request #4412: Fix bad cast in create_primitive_array
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on code in PR #4412:
URL: https://github.com/apache/arrow-rs/pull/4412#discussion_r1229264615
##########
arrow-ipc/src/reader.rs:
##########
@@ -250,24 +242,23 @@ fn create_primitive_array(
| UInt32
| Time32(_)
| Date32
- | Interval(IntervalUnit::YearMonth) => {
- if buffers[1].len() / 8 == length && length != 1 {
Review Comment:
This dates from the earliest version of the IPC reader added in https://github.com/apache/arrow/pull/4167/files#diff-c9e901cbd035c1eefca19e8a63765c428e11c4a2e732d9075368477f96348d31R220 by @nevi-me
My recollection is that some of the arrow implementations only support 64-bit primitives, perhaps Go or Javascript?
I would like to understand why this code was added before we go about removing it, and if we do we should also remove the similar case for Float32. Is this code causing you issues?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] tustvold commented on a diff in pull request #4412: Remove 64-bit to 32-bit Cast from IPC Reader
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on code in PR #4412:
URL: https://github.com/apache/arrow-rs/pull/4412#discussion_r1230489562
##########
arrow-ipc/src/reader.rs:
##########
@@ -250,24 +242,23 @@ fn create_primitive_array(
| UInt32
| Time32(_)
| Date32
- | Interval(IntervalUnit::YearMonth) => {
- if buffers[1].len() / 8 == length && length != 1 {
Review Comment:
I'm surprised that the C++ IPC isn't truncating slices, but if it isn't I can see how that would cause issues
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] ming08108 commented on pull request #4412: Remove 64-bit to 32-bit Cast from IPC Reader
Posted by "ming08108 (via GitHub)" <gi...@apache.org>.
ming08108 commented on PR #4412:
URL: https://github.com/apache/arrow-rs/pull/4412#issuecomment-1593843810
> Can you also remove the Float32 case, if all the tests and integration tests still pass I'll be happy to merge this
Sorry didn't see this before this got merged. Shall I open a new PR to do this?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org