Skip to content

GH-655: Failure in UnionReader.read after DecimalVector promotion to UnionVector #656

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Mar 13, 2025

Conversation

lriggs
Copy link
Contributor

@lriggs lriggs commented Mar 7, 2025

When a DecimalVector is promoted to a UnionVector via a PromotableWriter, the UnionVector will have the decimal vector in it's internal struct vector, but the decimalVector field will not be set. If UnionReader.read is then used to read from the UnionVector, it will fail when it tries to read one of the promoted decimal values, due to decimalVector being null, and the exact decimal type not being provided. This failure is unnecessary though as we have a pre-existing decimal vector, the caller just does not know the exact type - and it shouldn't be required to.

The change here is to check for a pre-existing decimal vector in the internal struct when getDecimalVector() is called. If one exists, set the decimalVector field and return. Otherwise, if none exists, throw the exception.

What's Changed

Updated UnionVector to handle this case.

Closes GH-655

…o UnionVector (apache#61)

When a DecimalVector is promoted to a UnionVector via a
PromotableWriter, the UnionVector will have the decimal vector in it's
internal struct vector, but the decimalVector field will not be set.
If UnionReader.read is then used to read from the UnionVector, it will
fail when it tries to read one of the promoted decimal values, due
to decimalVector being null, and the exact decimal type not being
provided.  This failure is unnecessary though as we have a pre-existing
decimal vector, the caller just does not know the exact type - and
it shouldn't be required to.

The change here is to check for a pre-existing decimal vector in the
internal struct when getDecimalVector() is called.  If one exists,
set the decimalVector field and return.  Otherwise, if none exists,
throw the exception.

This comment has been minimized.

@lidavidm lidavidm changed the title GH-655: Failure in UnionReader.read after DecimalVector promotion t… GH-655: Failure in UnionReader.read after DecimalVector promotion to UnionVector Mar 7, 2025
@lidavidm lidavidm added the bug-fix PRs that fix a big. label Mar 7, 2025
@lidavidm lidavidm added this to the 18.3.0 milestone Mar 7, 2025
@jbonofre jbonofre self-requested a review March 7, 2025 10:34
@@ -306,6 +307,9 @@ public StructVector getStruct() {

public ${name}Vector get${name}Vector(String name) {
if (${uncappedName}Vector == null) {
${uncappedName}Vector = internalStruct.getChild(fieldName(MinorType.${name?upper_case}), ${name}Vector.class);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we apply to only child, or also on parameterized types ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure, I'm submitting a change someone else made in our repo. Would it be any different if used the parameterized type instead?

@lidavidm
Copy link
Member

lidavidm commented Mar 8, 2025

It seems there's a syntax error still

@lriggs
Copy link
Contributor Author

lriggs commented Mar 12, 2025

I fixed the syntax error but not sure whats causing the latest Dev PR task error:
Linked issue: 655
failed to update #655: GraphQL: Resource not accessible by integration (updateIssue)

@lidavidm
Copy link
Member

That's separate (I think I need to grant that step permissions to update issues, too)

@lidavidm
Copy link
Member

Are you able to mvn spotless:apply and commit the result? (Or would it be ok for me to push a commit here to fix up things?)

@lidavidm lidavidm merged commit 32e4ae4 into apache:main Mar 13, 2025
25 of 26 checks passed
@lidavidm
Copy link
Member

Thanks!

dongjoon-hyun pushed a commit to apache/spark that referenced this pull request May 15, 2025
### What changes were proposed in this pull request?
This pr aims to upgrade `arrow-java` from 18.2.0 to 18.3.0.

### Why are the changes needed?
The new version bring some bug fixes, like:

- apache/arrow-java#627
- apache/arrow-java#654
- apache/arrow-java#656
- apache/arrow-java#693
- apache/arrow-java#705
- apache/arrow-java#707
- apache/arrow-java#722

In addition, the new version introduces a cascading upgrade for flatbuffers-java([ from 24.3.25 to 25.1.24 ](apache/arrow-java#600))

the full release note as follows:
- https://github.com/apache/arrow-java/releases/tag/v18.3.0

### Does this PR introduce _any_ user-facing change?
No

### How was this patch tested?
- Pass GitHub Acitons

### Was this patch authored or co-authored using generative AI tooling?
No

Closes #50892 from LuciferYang/arrow-java-18.3.0.

Authored-by: yangjie01 <yangjie01@baidu.com>
Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>
yhuang-db pushed a commit to yhuang-db/spark that referenced this pull request Jun 9, 2025
### What changes were proposed in this pull request?
This pr aims to upgrade `arrow-java` from 18.2.0 to 18.3.0.

### Why are the changes needed?
The new version bring some bug fixes, like:

- apache/arrow-java#627
- apache/arrow-java#654
- apache/arrow-java#656
- apache/arrow-java#693
- apache/arrow-java#705
- apache/arrow-java#707
- apache/arrow-java#722

In addition, the new version introduces a cascading upgrade for flatbuffers-java([ from 24.3.25 to 25.1.24 ](apache/arrow-java#600))

the full release note as follows:
- https://github.com/apache/arrow-java/releases/tag/v18.3.0

### Does this PR introduce _any_ user-facing change?
No

### How was this patch tested?
- Pass GitHub Acitons

### Was this patch authored or co-authored using generative AI tooling?
No

Closes apache#50892 from LuciferYang/arrow-java-18.3.0.

Authored-by: yangjie01 <yangjie01@baidu.com>
Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug-fix PRs that fix a big.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Failure in UnionReader.read after DecimalVector promotion to UnionVector
4 participants