fix `Vector{UInt8}` writing #419

Moelf · 2023-04-07T23:44:32Z

Setup:

julia> using Arrow

julia> df = (; x = [[0x01, 0x02], UInt8[], [0x03]])

julia> Arrow.write("/tmp/julia.feather", df)
"/tmp/julia.feather"

Before:

In [3]: pyarrow.feather.read_table("/tmp/julia.feather")["x"]
Out[3]:
<pyarrow.lib.ChunkedArray object at 0x7fb2994c86d0>
[
  [
    0102,
    ,
    03
  ]
]

Note: data = (; x = [b"12", b"", b"3"]) from Julia will give write out the exact same thing, which is IMO why this was wrong.

After:

In [39]: pyarrow.feather.read_table("/tmp/julia.feather")["x"]
Out[39]:
<pyarrow.lib.ChunkedArray object at 0x7f4568b376a0>
[
  [
    [
      1,
      2
    ],
    [],
    [
      3
    ]
  ]
]

Moelf · 2023-04-08T00:04:02Z

~~test wouldn't fully pass, could use some help and also~~ this is technically breaking but I believe this is the correct thing to do.

If user wants byte-string, they should use b"" (which still works now) and not Vector{UInt8} @quinnj

Moelf · 2023-04-11T16:14:04Z

CI waiting for #424

…t8_jagged

Moelf · 2023-04-13T03:58:15Z

src/eltypes.jl

+            Meta.largUtf8Start(b)
+            return Meta.LargeUtf8, Meta.largUtf8End(b), nothing
+        end
+    elseif eltype(A) == UInt8 && ArrowTypes.isstringtype(T)


the only additional type cathed here is Base.CodeUnits.

Ideally, we want the first (above) used for long string and this one for short-string, but that's not a well defined concept, so for now we just say, "use CodeUnits if you want output byte-string in Arrow"

What does "cathed" mean?

sorry, "caught" (I probably tried to type catched, isn't a word), as in caught by the condition

Moelf · 2023-04-13T04:03:35Z

The reason why this PR doesn't need to touch test is that our implementation was wrongly self-consistent -- it ignored an entire possible type composition in arrow:

Before this PR

Moelf · 2023-05-10T18:44:24Z

@quinnj 👀 gentle bump?

simsurace · 2023-05-12T21:45:25Z

@ericphanson or @baumgold would you be willing/able to review this?

baumgold · 2023-05-13T13:58:23Z

@ericphanson or @baumgold would you be willing/able to review this?

Looks like some of the tests are failing. Would you mind investigating and fixing? Thanks.

Moelf · 2023-05-13T14:17:12Z

@baumgold

some tests are failing because this changes both code base, you should look at this monorepo test I added in #424:

https://github.com/apache/arrow-julia/actions/runs/4685294460/jobs/8302259625?pr=419

baumgold · 2023-05-14T02:21:16Z

I see. Probably we should split this up into 2 smaller PRs: one for ArrowTypes and one for Arrow. Assuming we make a new release of ArrowTypes then in the follow-on Arrow PR we can bump the compat of ArrowTypes to the newly released version.

Moelf · 2023-05-14T04:42:03Z

making a stand alone PR adding a convenient function that doesn't get used and has no direct test makes 0 sense to me.

all the changes needed are in Arrow.jl, the convenient function to ArrowType is because I think it should be the same function. I could have added a function with some other name into Arrow.jl, which would be ugly

the "normal" tests fail just because how tests are set up (which brings us back to question: isn't the whole point of monorepo so that when we have 2 packages we don't have to make rapid release to fix tests for the other package?)

ericphanson · 2023-05-14T09:58:41Z

isn't the whole point of monorepo so that when we have 2 packages we don't have to make rapid release to fix tests for the other package?

I’m not sure all the reasons for using this setup in this case but that definitely isn’t always a goal. E.g. https://discourse.julialang.org/t/how-beacon-packages-julia-code-in-a-monorepo/90822.

Moelf · 2023-05-14T12:23:38Z

So what people think, I should do a separate PR to add convince function??

simsurace · 2023-05-15T05:28:30Z

If this is what it takes to keep the ball moving, I would suggest doing so, but I am not a maintainer, just an observer interested to see this package continue getting improved.

Moelf · 2023-05-15T14:54:36Z

just to be clear, in that case we will be doing:

make a PR to add a convenient function ("will be used in next PR trust me")
release a new ArrowTypes.jl for that, and potentially making a bunch of users re-precompile their stuff for no reason
see tests in this PR turn all green?

split from apache#419

quinnj · 2023-05-18T21:31:49Z

@Moelf, do you mind if I make an alternative PR to fix the original issue here?

Moelf · 2023-05-18T21:35:02Z

go ahead

Fixes #411. Alternative to #419. This PR should be compatible with or without the ArrowTypes changes. I think it's fine to do compat things in Arrow like this as long as they don't get out of hand and we can eventually remove them as we bump required ArrowTypes versions and such. The PR consists of not treating `Vector{UInt8}` as the Arrow Binary type, which is meant for "binary string"s. Julia has a pretty good match for that in `Base.CodeUnits`, so instead, we use that to write Binary and `Vector{UInt8}` is treated as a regular List of Primitive UInt8 type.

quinnj · 2023-05-18T23:02:57Z

Ok, PR up: #439. Sorry to be so MIA lately; I've been tied up in some heavy other projects and it's been too hard to context-switch back here. A lot of that work (webstack-related) has wrapped up (mostly), I'm hoping to have more time to help review/fix stuff here.

This PR was definitely in the right direction @Moelf; thanks for the contribution. It was a great starting point. Couple of specific points:

I'd rather not pun the ArrowTypes.isstringtype function for non-ListKind types; it keeps that a little cleaner IMO
We can make Base.CodeUnits the main translation between the Arrow Binary type
We can add some compat shims in Arrow to avoid the awkward ArrowTypes/Arrow mis-compat

Fixes #411. Alternative to #419. This PR should be compatible with or without the ArrowTypes changes. I think it's fine to do compat things in Arrow like this as long as they don't get out of hand and we can eventually remove them as we bump required ArrowTypes versions and such. The PR consists of not treating `Vector{UInt8}` as the Arrow Binary type, which is meant for "binary string"s. Julia has a pretty good match for that in `Base.CodeUnits`, so instead, we use that to write Binary and `Vector{UInt8}` is treated as a regular List of Primitive UInt8 type.

fix Vector{UInt8} writing

224d94e

Moelf changed the title ~~fix Vector{UInt8} writing~~ fix Vector{UInt8} writing Apr 7, 2023

Moelf added 4 commits April 7, 2023 19:45

remove debug

d5a1f12

more robust

2d84241

more robust

7a22d95

restore tests

3e0fff3

ericphanson mentioned this pull request Apr 11, 2023

add dev CI for monorepo #424

Merged

Moelf force-pushed the uint8_jagged branch from 8ed9443 to 3e0fff3 Compare April 13, 2023 01:28

Moelf added 2 commits April 12, 2023 21:32

Merge branch 'main' of https://github.com/apache/arrow-julia into uin…

a1e123b

…t8_jagged

fix tests and give up fixedsizelist for now

18cee30

Moelf commented Apr 13, 2023

View reviewed changes

Moelf added a commit to Moelf/arrow-julia that referenced this pull request May 18, 2023

Update ArrowTypes.jl to add utility function

99df35e

split from apache#419

Moelf mentioned this pull request May 18, 2023

Update ArrowTypes.jl to add utility function #438

Closed

quinnj mentioned this pull request May 18, 2023

Don't treat Vector{UInt8} as Arrow Binary type #439

Merged

quinnj closed this May 18, 2023

Moelf deleted the uint8_jagged branch May 19, 2023 02:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix `Vector{UInt8}` writing #419

fix `Vector{UInt8}` writing #419

Moelf commented Apr 7, 2023 •

edited

Loading

Moelf commented Apr 8, 2023 •

edited

Loading

Moelf commented Apr 11, 2023

Moelf Apr 13, 2023

quinnj May 18, 2023

Moelf May 18, 2023 •

edited

Loading

Moelf commented Apr 13, 2023 •

edited

Loading

Moelf commented May 10, 2023

simsurace commented May 12, 2023

baumgold commented May 13, 2023

Moelf commented May 13, 2023 •

edited

Loading

baumgold commented May 14, 2023

Moelf commented May 14, 2023 •

edited

Loading

ericphanson commented May 14, 2023

Moelf commented May 14, 2023

simsurace commented May 15, 2023

Moelf commented May 15, 2023

quinnj commented May 18, 2023

Moelf commented May 18, 2023

quinnj commented May 18, 2023

fix Vector{UInt8} writing #419

fix Vector{UInt8} writing #419

Conversation

Moelf commented Apr 7, 2023 • edited Loading

Moelf commented Apr 8, 2023 • edited Loading

Moelf commented Apr 11, 2023

Moelf Apr 13, 2023

Choose a reason for hiding this comment

quinnj May 18, 2023

Choose a reason for hiding this comment

Moelf May 18, 2023 • edited Loading

Choose a reason for hiding this comment

Moelf commented Apr 13, 2023 • edited Loading

Before this PR

Moelf commented May 10, 2023

simsurace commented May 12, 2023

baumgold commented May 13, 2023

Moelf commented May 13, 2023 • edited Loading

baumgold commented May 14, 2023

Moelf commented May 14, 2023 • edited Loading

ericphanson commented May 14, 2023

Moelf commented May 14, 2023

simsurace commented May 15, 2023

Moelf commented May 15, 2023

quinnj commented May 18, 2023

Moelf commented May 18, 2023

quinnj commented May 18, 2023

fix `Vector{UInt8}` writing #419

fix `Vector{UInt8}` writing #419

Moelf commented Apr 7, 2023 •

edited

Loading

Moelf commented Apr 8, 2023 •

edited

Loading

Moelf May 18, 2023 •

edited

Loading

Moelf commented Apr 13, 2023 •

edited

Loading

Moelf commented May 13, 2023 •

edited

Loading

Moelf commented May 14, 2023 •

edited

Loading