-
Notifications
You must be signed in to change notification settings - Fork 902
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
Java: Support struct scalar [skip ci] #8327
Java: Support struct scalar [skip ci] #8327
Conversation
Signed-off-by: sperlingxx <[email protected]>
Signed-off-by: sperlingxx <[email protected]>
Signed-off-by: sperlingxx <[email protected]>
Signed-off-by: sperlingxx <[email protected]>
Signed-off-by: sperlingxx <[email protected]>
Signed-off-by: sperlingxx <[email protected]>
Hi @revans2 @jlowe @firestarman, thanks for review! I updated this PR, adding the support of |
LGTM |
} | ||
return new Scalar(DType.STRUCT, makeStructScalar(childHandles, false)); | ||
} finally { | ||
IllegalStateException closeException = null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This won't do what we want. If an exception is thrown from the try
block then this finally
block will still clobber that exception if any close()
call fails. It needs something like this:
RuntimeException error = null;
try {
...
} catch (RuntimeException e) {
error = e;
} catch (Exception e) {
error = new RuntimeException(e);
} finally {
for (ColumnVector child : children) {
if (child != null) {
try {
child.close();
} catch (Exception e) {
if (error != null) {
error.addSuppressed(e);
} else {
error = new RuntimeException(e);
}
}
}
}
if (error != null) {
throw error;
}
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed. Thanks for the demo code!
// make sure the close process is exception-free | ||
try { | ||
child.close(); | ||
} catch (Exception ignore) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This has the same problem as up above we need to not just eat the exceptions but add them to the suppression list.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
Signed-off-by: sperlingxx <[email protected]>
@gpucibot merge |
Current PR is to support struct scalar in Java package, which is required by spark-rapids (issue link). In detail, current PR introduces three new features: