You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "palday (via GitHub)" <gi...@apache.org> on 2025/12/31 16:26:33 UTC
[PR] fix test failures on 1.12 [arrow-julia]
palday opened a new pull request, #582:
URL: https://github.com/apache/arrow-julia/pull/582
(no comment)
--
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
Re: [PR] fix test failures on 1.12, avoid race condition in multithreaded partitioned writes [arrow-julia]
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on PR #582:
URL: https://github.com/apache/arrow-julia/pull/582#issuecomment-3741338523
The vote thread: https://lists.apache.org/thread/4o09ts7gr86wjyt3smxw4vzrrvrqpjgs
Please verify and vote it.
--
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
Re: [PR] fix test failures on 1.12 [arrow-julia]
Posted by "palday (via GitHub)" <gi...@apache.org>.
palday commented on code in PR #582:
URL: https://github.com/apache/arrow-julia/pull/582#discussion_r2656839168
##########
src/arraytypes/dictencoding.jl:
##########
@@ -215,6 +218,8 @@ function arrowvector(
x = x.data
len = length(x)
validity = ValidityBitmap(x)
+ # XXX This is a race condition if two workers hit this block at the same time, then they'll create
Review Comment:
@quinnj I think there is a race condition baked into the current architecture that can't be addressed without a very large refactoring. The current architecture creates the locks on a worker thread if they don't already exist, which means that threads are competing for the creation of the initial lock. The locks should be created _before_ any tasks are spawned.
--
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
Re: [PR] fix test failures on 1.12 [arrow-julia]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #582:
URL: https://github.com/apache/arrow-julia/pull/582#issuecomment-3702483707
## [Codecov](https://app.codecov.io/gh/apache/arrow-julia/pull/582?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `0%` with `16 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 86.31%. Comparing base ([`3712291`](https://app.codecov.io/gh/apache/arrow-julia/commit/37122911c24f44318e6d4a0840408adb3364cf2a?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`58c1a9e`](https://app.codecov.io/gh/apache/arrow-julia/commit/58c1a9eadbda1698a1651410b4e166b392a10c53?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 42 commits behind head on main.
| [Files with missing lines](https://app.codecov.io/gh/apache/arrow-julia/pull/582?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [src/arraytypes/dictencoding.jl](https://app.codecov.io/gh/apache/arrow-julia/pull/582?src=pr&el=tree&filepath=src%2Farraytypes%2Fdictencoding.jl&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3JjL2FycmF5dHlwZXMvZGljdGVuY29kaW5nLmps) | 0.00% | [16 Missing :warning: ](https://app.codecov.io/gh/apache/arrow-julia/pull/582?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## main #582 +/- ##
==========================================
- Coverage 87.43% 86.31% -1.12%
==========================================
Files 26 27 +1
Lines 3288 3399 +111
==========================================
+ Hits 2875 2934 +59
- Misses 413 465 +52
```
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/arrow-julia/pull/582?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
</details>
--
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
Re: [PR] fix test failures on 1.12, avoid race condition in multithreaded partitioned writes [arrow-julia]
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on PR #582:
URL: https://github.com/apache/arrow-julia/pull/582#issuecomment-3747716955
The vote carried: https://lists.apache.org/thread/b9pmo5wl46fds2sw8ljjpxqj5vlk5z3m
I've released 2.8.1.
--
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
Re: [PR] fix test failures on 1.12, avoid race condition in multithreaded partitioned writes [arrow-julia]
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou merged PR #582:
URL: https://github.com/apache/arrow-julia/pull/582
--
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
Re: [PR] fix test failures on 1.12 [arrow-julia]
Posted by "palday (via GitHub)" <gi...@apache.org>.
palday commented on code in PR #582:
URL: https://github.com/apache/arrow-julia/pull/582#discussion_r2665726882
##########
src/arraytypes/dictencoding.jl:
##########
@@ -142,55 +142,58 @@ function arrowvector(
kw...,
)
id = x.encoding.id
+ # XXX This is a race condition if two workers hit this block at the same time, then they'll create
+ # distinct locks
if !haskey(de, id)
de[id] = Lockable(x.encoding)
- else
- encodinglockable = de[id]
- Base.@lock encodinglockable begin
- encoding = encodinglockable.value
- # in this case, we just need to check if any values in our local pool need to be delta dicationary serialized
- deltas = setdiff(x.encoding, encoding)
- if !isempty(deltas)
- ET = indextype(encoding)
- if length(deltas) + length(encoding) > typemax(ET)
- error(
- "fatal error serializing dict encoded column with ref index type of $ET; subsequent record batch unique values resulted in $(length(deltas) + length(encoding)) unique values, which exceeds possible index values in $ET",
- )
- end
- data = arrowvector(
- deltas,
- i,
- nl,
- fi,
- de,
- ded,
- nothing;
- dictencode=dictencodenested,
- dictencodenested=dictencodenested,
- dictencoding=true,
- kw...,
+ return x
Review Comment:
changed this to an early return to make the overall logic a little clearer / reduce nesting of the `else` branch.
--
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
Re: [PR] fix test failures on 1.12, avoid race condition in multithreaded partitioned writes [arrow-julia]
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on PR #582:
URL: https://github.com/apache/arrow-julia/pull/582#issuecomment-3741318138
No objection. I'll merge this and start a release process.
--
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
Re: [PR] fix test failures on 1.12 [arrow-julia]
Posted by "palday (via GitHub)" <gi...@apache.org>.
palday commented on code in PR #582:
URL: https://github.com/apache/arrow-julia/pull/582#discussion_r2665724669
##########
test/runtests.jl:
##########
@@ -28,12 +28,16 @@ using DataAPI
using FilePathsBase
using DataFrames
import Random: randstring
+using TestSetExtensions: ExtendedTestSet
Review Comment:
given how long the Arrow tests take, it's useful to have some indication of progress so that we can tell if tests have hung. ExtendedTestSet shows a `.` for each completed test.
(We also get colored diffs of arrays when tests fail, which is nice.)
--
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
Re: [PR] fix test failures on 1.12 [arrow-julia]
Posted by "palday (via GitHub)" <gi...@apache.org>.
palday commented on PR #582:
URL: https://github.com/apache/arrow-julia/pull/582#issuecomment-3716638464
@kou @ericphanson Could I get review on this please? It's not a pretty solution, but it works.
--
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
Re: [PR] fix test failures on 1.12, avoid race condition in multithreaded partitioned writes [arrow-julia]
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on PR #582:
URL: https://github.com/apache/arrow-julia/pull/582#issuecomment-3726633689
I'll merge this in a few days if nobody has more comments.
We can try 2.8.4 RC2 release with this, right?
See also: The 2.8.4 RC1 vote: https://lists.apache.org/thread/7g3wr39wlbs8dj08hpb87mf9z2mlqrft
--
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
Re: [PR] fix test failures on 1.12, avoid race condition in multithreaded partitioned writes [arrow-julia]
Posted by "palday (via GitHub)" <gi...@apache.org>.
palday commented on PR #582:
URL: https://github.com/apache/arrow-julia/pull/582#issuecomment-3726994517
> We can try 2.8.4 RC2 release with this, right?
@kou Yes, exactly. :smile:
--
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