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

GH-41262: [Java][FlightSQL] Implement stateless prepared statements #41237

Merged
merged 16 commits into from
Jun 3, 2024

Conversation

stevelorddremio
Copy link
Contributor

@stevelorddremio stevelorddremio commented Apr 16, 2024

Rationale for this change

Expand the number of implemented languages for stateless prepared statements to include Java.

What changes are included in this PR?

Update FlightSqlClient and include a stateless server implementation example with tests.

Are these changes tested?

Yes, tests are added to cover a stateless server implementation.

Are there any user-facing changes?

There is a modified FlightSqlClient that is required to enable use of stateless prepared statements.

@jduo
Copy link
Member

jduo commented Apr 16, 2024

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Let's create a separate Issue for this vs the existing one being used for Go.

@stevelorddremio stevelorddremio changed the title GH-37720: [Java][FlightSQL] Implement stateless prepared statements GH-41262: [Java][FlightSQL] Implement stateless prepared statements Apr 17, 2024
Copy link

⚠️ GitHub issue #41262 has been automatically assigned in GitHub to PR creator.

@lidavidm
Copy link
Member

It looks like all tests are failing?

@vibhatha
Copy link
Collaborator

@lidavidm not sure if the failures are related though. Would it be possible to run the CIs again and verify?

@lidavidm
Copy link
Member

Still failing.

@vibhatha
Copy link
Collaborator

vibhatha commented Apr 19, 2024

Looking into a common CI failure[1] in PR[2] and in this PR the failure [3], seems to be the same.

The error occurs initially for current PR at [4], and for PR[2] the same failure[5] occurs.

Testing C ArrowArray from file 'map_non_canonical'
... with record batch #0
Traceback (most recent call last):
  File "/arrow/dev/archery/archery/integration/runner.py", line 521, in _run_c_array_test_cases
    do_run()
  File "/arrow/dev/archery/archery/integration/runner.py", line 501, in do_run
    importer.import_batch_and_compare_to_json(json_path,
  File "/arrow/dev/archery/archery/integration/tester_csharp.py", line 135, in import_batch_and_compare_to_json
    ArrowReaderVerifier.CompareBatches(batch, imported_batch,
Xunit.Sdk.TrueException: Bit at index 0/2 is not equal

In other failing CIs the failure is also with a java.net.SocketException: Broken pipe (Write failed)

Although there is a related failure[6], which seems to be an issue with a test case in this PR.

Error:  Errors: 
Error:    TestFlightSqlStateless.setUp:50->setUpClientServer:63 � IllegalState Failed to reset Derby database!

Seems like there are related an unrelated reasons for failing the CIs.

[1] https://github.com/apache/arrow/actions/runs/8746316554/job/24003804303?pr=41297#step:8:19557
[2] #41297
[3] https://github.com/apache/arrow/actions/runs/8743037827/job/23992652573?pr=41237#step:8:19559
[4] https://github.com/apache/arrow/actions/runs/8743037827/job/23992652573?pr=41237#step:8:15258
[5] https://github.com/apache/arrow/actions/runs/8746316554/job/24003804303?pr=41297#step:8:18118
[6] https://github.com/apache/arrow/actions/runs/8743037859/job/24003647141?pr=41237#step:5:3160

@vibhatha
Copy link
Collaborator

It would be better to wait for these changes and rebasing would probably solve the C# related issues, but the following needs to be fixed here.

Error:  Errors: 
Error:    TestFlightSqlStateless.setUp:50->setUpClientServer:63 � IllegalState Failed to reset Derby database!

I am not very fluent with this component. @lidavidm any thoughts?

@lidavidm
Copy link
Member

I'm not sure what you're looking at. The main Java pipelines all time out here.

@vibhatha
Copy link
Collaborator

I'm not sure what you're looking at. The main Java pipelines all time out here.

I was referring to this error in AMD64 Windows Server 2022 Java JDK 11

@stevelorddremio
Copy link
Contributor Author

I am investigating the test failure and thank you for the additional possible related CI failures.
When I run the tests locally as a complete suite, on Mac, this test hangs at https://github.com/apache/arrow/pull/41237/files#diff-e1eee7ea54762a289b8e5851de5f7d5d6be56d46b2abf02972c31a5673fa6f44R89.
If I run the tests individually they all pass. So there looks to be some contention between tests.
Grateful for any further suggestions.

@stevelorddremio
Copy link
Contributor Author

stevelorddremio commented Apr 19, 2024

I have rebased with, hopefully, some fixes to CI issues.
java.lang.IllegalStateException: Failed to reset Derby database! is still occurring in Windows tests.
In other environments the tests are being cancelled. I suspect this is the same as the hang I see running tests locally in a FlightStream.getRoot() call.

@vibhatha
Copy link
Collaborator

@lidavidm most CIs are failing but the error message is

Error: The operation was canceled.

Could we re-run?

@stevelorddremio
Copy link
Contributor Author

Noted @lidavidm. I'll update to wrap results in Any.

@lidavidm
Copy link
Member

Thanks!

@stevelorddremio
Copy link
Contributor Author

This implementation is currently on hold pending outcome of discussions in apache/arrow-rs#5731 and apache/arrow-go#34. The current Java implementation does not wrap PutResult.applicationMetadata with Any.

@stevelorddremio
Copy link
Contributor Author

As the consensus is that results' metadata is not wrapped in Any, this would welcome a review.
One outstanding issue is why Java JNI build is failing. I'm not sure how to resolve that.

@lidavidm
Copy link
Member

lidavidm commented Jun 3, 2024

@stevelorddremio do you mind rebasing again?

stevelorddremio and others added 16 commits June 3, 2024 08:48
Clean up Derby database at end of class test
Continue to attempt to clean Derby database
@stevelorddremio
Copy link
Contributor Author

Rebased @lidavidm. Clean run this time. Nice. Thanks for the prompt.

@lidavidm lidavidm merged commit 1598782 into apache:main Jun 3, 2024
15 checks passed
@lidavidm lidavidm removed the awaiting merge Awaiting merge label Jun 3, 2024
@lidavidm
Copy link
Member

lidavidm commented Jun 3, 2024

Thanks for sticking with this.

Copy link

After merging your PR, Conbench analyzed the 5 benchmarking runs that have been run so far on merge-commit 1598782.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details. It also includes information about 6 possible false positives for unstable benchmarks that are known to sometimes produce them.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants