Skip to content
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

JavaClient: Encode Packed Booleans Instead of Reinterpretted Bytes #3812

Merged
merged 5 commits into from
May 9, 2023

Conversation

nbauernfeind
Copy link
Member

@nbauernfeind nbauernfeind commented May 8, 2023

Fixes #3799.
Fixes #1373.

@nbauernfeind nbauernfeind added bug Something isn't working barrage java-client release blocker A bug/behavior that puts is below the "good enough" threshold to release. labels May 8, 2023
@nbauernfeind nbauernfeind added this to the Apr 2023 milestone May 8, 2023
@nbauernfeind nbauernfeind self-assigned this May 8, 2023
@devinrsmith
Copy link
Member

devinrsmith commented May 8, 2023

May be worthwhil to try and fix #1373, here is the commented out code updated from io.deephaven.client.DeephavenFlightSessionTest:

    // TODO (deephaven-core#1373): Hook up doPut integration unit testing
    @Test
    public void doPutStream() throws Exception {
        try (
                final TableHandle ten = flightSession.session().execute(TableSpec.empty(10).view("I=i"));
                // DoGet
                final FlightStream tenStream = flightSession.stream(ten);
                // DoPut
                final TableHandle tenAgain = flightSession.putExport(tenStream)) {
            assertThat(tenAgain.response().getSchemaHeader()).isEqualTo(ten.response().getSchemaHeader());
        }
    }

     @Test
     public void doPutNewTable() throws TableHandleException, InterruptedException {
         try (final TableHandle newTableHandle = flightSession.putExport(newTable(), bufferAllocator)) {
            // ignore
         }
     }

It appears the flight client is blocking on getResult; it may be a factor that this is an in-process setup?

java.lang.Thread.State: WAITING
     at jdk.internal.misc.Unsafe.park(Unsafe.java:-1)
     at java.util.concurrent.locks.LockSupport.park(LockSupport.java:194)
     at java.util.concurrent.CompletableFuture$Signaller.block(CompletableFuture.java:1796)
     at java.util.concurrent.ForkJoinPool.managedBlock(ForkJoinPool.java:3128)
     at java.util.concurrent.CompletableFuture.waitingGet(CompletableFuture.java:1823)
     at java.util.concurrent.CompletableFuture.get(CompletableFuture.java:1998)
     at org.apache.arrow.flight.AsyncPutListener.getResult(AsyncPutListener.java:46)
     at org.apache.arrow.flight.FlightClient$$Lambda$788.1823014131.run(Unknown Source:-1)
     at org.apache.arrow.flight.FlightClient$PutObserver.getResult(FlightClient.java:491)
     at io.deephaven.client.impl.FlightClientHelper.put(FlightClientHelper.java:51)
     at io.deephaven.client.impl.FlightClientHelper.put(FlightClientHelper.java:29)
     at io.deephaven.client.impl.FlightSession.put(FlightSession.java:198)
     at io.deephaven.client.impl.FlightSession.putExportManual(FlightSession.java:183)
     at io.deephaven.client.impl.FlightSession.putExport(FlightSession.java:135)
     at io.deephaven.client.DeephavenFlightSessionTest.doPutStream(DeephavenFlightSessionTest.java:93)

@devinrsmith devinrsmith self-requested a review May 9, 2023 14:15
Copy link
Member

@devinrsmith devinrsmith left a comment

Choose a reason for hiding this comment

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

All looks good to me. Looks like spotless in complaining. We probably want to push this through nightlies?

@devinrsmith devinrsmith self-requested a review May 9, 2023 15:23
Comment on lines +82 to +85
if (UpdateGraphProcessor.DEFAULT.isUnitTestModeAllowed()) {
UpdateGraphProcessor.DEFAULT.enableUnitTestMode();
UpdateGraphProcessor.DEFAULT.resetForUnitTests(false);
}
Copy link
Member

Choose a reason for hiding this comment

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

Not sure exactly the pattern we are supposed to be using - will defer to @rcaudy

@nbauernfeind nbauernfeind merged commit 2bb4643 into deephaven:main May 9, 2023
@github-actions github-actions bot locked and limited conversation to collaborators May 9, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
barrage bug Something isn't working java-client NoDocumentationNeeded release blocker A bug/behavior that puts is below the "good enough" threshold to release. ReleaseNotesNeeded Release notes are needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Java client fails to publish columns of booleans Hook up doPut integration unit testing
2 participants