You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "lriggs (via GitHub)" <gi...@apache.org> on 2025/03/07 00:39:09 UTC
[PR] GH-655: Failure in UnionReader.read after DecimalVector promotion t… [arrow-java]
lriggs opened a new pull request, #656:
URL: https://github.com/apache/arrow-java/pull/656
…o UnionVector (#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.
## What's Changed
Updated UnionVector to handle this case.
Closes GH-655
--
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] GH-655: Failure in UnionReader.read after DecimalVector promotion to UnionVector [arrow-java]
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on PR #656:
URL: https://github.com/apache/arrow-java/pull/656#issuecomment-2719325890
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?)
--
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] GH-655: Failure in UnionReader.read after DecimalVector promotion to UnionVector [arrow-java]
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on PR #656:
URL: https://github.com/apache/arrow-java/pull/656#issuecomment-2719322502
That's separate (I think I need to grant that step permissions to update issues, too)
--
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] GH-655: Failure in UnionReader.read after DecimalVector promotion to UnionVector [arrow-java]
Posted by "lriggs (via GitHub)" <gi...@apache.org>.
lriggs commented on code in PR #656:
URL: https://github.com/apache/arrow-java/pull/656#discussion_r1985758964
##########
vector/src/main/codegen/templates/UnionVector.java:
##########
@@ -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);
+ if (${uncappedName}Vector == null) {
+ throw new IllegalArgumentException("No ${uncappedName} present. Provide ArrowType argument to create a new vector");
Review Comment:
I pushed a new commit where this code is moved slightly, had forgotten that this one was updated in our repo.
--
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] GH-655: Failure in UnionReader.read after DecimalVector promotion to UnionVector [arrow-java]
Posted by "jbonofre (via GitHub)" <gi...@apache.org>.
jbonofre commented on code in PR #656:
URL: https://github.com/apache/arrow-java/pull/656#discussion_r1984837734
##########
vector/src/main/codegen/templates/UnionVector.java:
##########
@@ -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);
Review Comment:
Should we apply to only child, or also on parameterized types ?
--
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] GH-655: Failure in UnionReader.read after DecimalVector promotion to UnionVector [arrow-java]
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on code in PR #656:
URL: https://github.com/apache/arrow-java/pull/656#discussion_r1984241340
##########
vector/src/main/codegen/templates/UnionVector.java:
##########
@@ -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);
+ if (${uncappedName}Vector == null) {
+ throw new IllegalArgumentException("No ${uncappedName} present. Provide ArrowType argument to create a new vector");
Review Comment:
Is it just me or is there no closing brace here?
Shouldn't we only add this new check for parameterized/nested types?
--
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] GH-655: Failure in UnionReader.read after DecimalVector promotion t… [arrow-java]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #656:
URL: https://github.com/apache/arrow-java/pull/656#issuecomment-2705254094
<!-- label_helper_comment -->
Thank you for opening a pull request!
Please label the PR with one or more of:
- bug-fix
- chore
- dependencies
- documentation
- enhancement
Also, add the 'breaking-change' label if appropriate.
See [CONTRIBUTING.md](https://github.com/apache/arrow-java/blob/main/CONTRIBUTING.md) for 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] GH-655: Failure in UnionReader.read after DecimalVector promotion to UnionVector [arrow-java]
Posted by "jbonofre (via GitHub)" <gi...@apache.org>.
jbonofre commented on code in PR #656:
URL: https://github.com/apache/arrow-java/pull/656#discussion_r1984836267
##########
vector/src/main/codegen/templates/UnionVector.java:
##########
@@ -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);
+ if (${uncappedName}Vector == null) {
+ throw new IllegalArgumentException("No ${uncappedName} present. Provide ArrowType argument to create a new vector");
Review Comment:
From a syntax standpoint (brace), it looks good to me.
--
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] GH-655: Failure in UnionReader.read after DecimalVector promotion to UnionVector [arrow-java]
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm merged PR #656:
URL: https://github.com/apache/arrow-java/pull/656
--
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] GH-655: Failure in UnionReader.read after DecimalVector promotion to UnionVector [arrow-java]
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on PR #656:
URL: https://github.com/apache/arrow-java/pull/656#issuecomment-2719441097
Thanks!
--
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] GH-655: Failure in UnionReader.read after DecimalVector promotion to UnionVector [arrow-java]
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on PR #656:
URL: https://github.com/apache/arrow-java/pull/656#issuecomment-2707897863
It seems there's a syntax error still
--
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] GH-655: Failure in UnionReader.read after DecimalVector promotion to UnionVector [arrow-java]
Posted by "lriggs (via GitHub)" <gi...@apache.org>.
lriggs commented on PR #656:
URL: https://github.com/apache/arrow-java/pull/656#issuecomment-2719221518
I fixed the syntax error but not sure whats causing the latest Dev PR task error:
Linked issue: 655
failed to update https://github.com/apache/arrow-java/issues/655: GraphQL: Resource not accessible by integration (updateIssue)
--
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] GH-655: Failure in UnionReader.read after DecimalVector promotion to UnionVector [arrow-java]
Posted by "lriggs (via GitHub)" <gi...@apache.org>.
lriggs commented on code in PR #656:
URL: https://github.com/apache/arrow-java/pull/656#discussion_r1985764504
##########
vector/src/main/codegen/templates/UnionVector.java:
##########
@@ -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);
Review Comment:
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?
--
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