-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
Conversation
Let's create a separate Issue for this vs the existing one being used for Go. |
.../flight-sql/src/test/java/org/apache/arrow/flight/sql/example/FlightSqlStatelessExample.java
Outdated
Show resolved
Hide resolved
.../flight-sql/src/test/java/org/apache/arrow/flight/sql/example/FlightSqlStatelessExample.java
Outdated
Show resolved
Hide resolved
.../flight-sql/src/test/java/org/apache/arrow/flight/sql/example/FlightSqlStatelessExample.java
Outdated
Show resolved
Hide resolved
...flight/flight-sql/src/test/java/org/apache/arrow/flight/sql/test/TestFlightSqlStateless.java
Outdated
Show resolved
Hide resolved
...flight/flight-sql/src/test/java/org/apache/arrow/flight/sql/test/TestFlightSqlStateless.java
Outdated
Show resolved
Hide resolved
.../flight-sql/src/test/java/org/apache/arrow/flight/sql/example/FlightSqlStatelessExample.java
Outdated
Show resolved
Hide resolved
.../flight-sql/src/test/java/org/apache/arrow/flight/sql/example/FlightSqlStatelessExample.java
Show resolved
Hide resolved
|
322a58c
to
0d533c9
Compare
It looks like all tests are failing? |
@lidavidm not sure if the failures are related though. Would it be possible to run the CIs again and verify? |
Still failing. |
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 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 |
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? |
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 |
I am investigating the test failure and thank you for the additional possible related CI failures. |
3f222bd
to
04fc871
Compare
I have rebased with, hopefully, some fixes to CI issues. |
@lidavidm most CIs are failing but the error message is Error: The operation was canceled. Could we re-run? |
Noted @lidavidm. I'll update to wrap results in Any. |
Thanks! |
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. |
As the consensus is that results' metadata is not wrapped in Any, this would welcome a review. |
@stevelorddremio do you mind rebasing again? |
…ents Part fixed caching of statementContext
PR comment updates
Clean up Derby database at end of class test
Continue to attempt to clean Derby database
Fix Derby clean up
Create separate database per test class
Rebased @lidavidm. Clean run this time. Nice. Thanks for the prompt. |
Thanks for sticking with this. |
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. |
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.