ARROW-15112: [Integration][C++][Java] Implement Flight SQL integration tests #11989

rafael-telles · 2021-12-17T21:46:28Z

This adds Flight SQL scenarios for integration tests for C++ and Java implementations.
The integrations tests assert that:

RPC objects built on clients are parsed correctly on servers
Arrow vectors built on servers have the expected Arrow schemas when received on clients

Note: Had to separate integration tests in a new module within Java and C++ to avoid circular dependencies (Flight <-> Flight SQL)

While working on this I found some inconsistencies between both implementation, this PR also fixes these problems.

github-actions · 2021-12-17T21:46:46Z

https://issues.apache.org/jira/browse/ARROW-15112

rafael-telles · 2021-12-17T21:49:39Z

Hi @lidavidm ! We wrote the integration tests for Flight SQL as requested :)

lidavidm

Thanks for the quick turnaround, this looks good overall. I left some comments.

lidavidm · 2021-12-17T22:10:39Z

cpp/src/arrow/flight/integration_tests/CMakeLists.txt

+# specific language governing permissions and limitations
+# under the License.
+
+add_custom_target(arrow_flight_integration_tests)


Is the target necessary?

It was already there on arrow/flight/CMakeLists.txt before moving to the new directory

cpp/src/arrow/CMakeLists.txt

cpp/src/arrow/flight/integration_tests/test_integration.cc

cpp/src/arrow/flight/integration_tests/test_integration.h

...tests/src/main/java/org/apache/arrow/flight/integration/tests/FlightSqlScenarioProducer.java

lidavidm · 2021-12-20T14:01:44Z

We need to exclude Go and Rust from the new test scenario.

Also, the PR needs to be formatted (see https://arrow.apache.org/docs/developers/cpp/development.html#code-style-linting-and-ci, note that we use ClangTools 12 now not version 8)

rafael-telles · 2021-12-20T17:38:36Z

Done @lidavidm

rafael-telles · 2021-12-20T21:27:05Z

Hey @lidavidm , I don't know if these CI failures has something to do with these changes... Please let me know if there is anything else to do here

lidavidm · 2021-12-20T21:48:35Z

There's some lint failures:


[1/1] cd /tmp/arrow-lint-6fb2hzaq/cpp-build && /usr/local/bin/python /arrow/cpp/build-support/lint_cpp_cli.py /arrow/cpp/src
INFO:archery:Running Python formatter (autopep8)
INFO:archery:Running Python linter (flake8)
/arrow/dev/archery/archery/integration/tester_java.py:52:80: E501 line too long (95 > 79 characters)
/arrow/dev/archery/archery/integration/runner.py:387:80: E501 line too long (82 > 79 characters)
INFO:archery:Running cmake-format linters
ERROR __main__.py:618: Check failed: /arrow/cpp/src/arrow/CMakeLists.txt

I think the other one is unrelated, but could you rebase on master to see if it goes away?

rafael-telles · 2021-12-20T22:39:12Z

Ops @lidavidm ... Sorry, I don't know if I messed up when git rebase master or if also need to run it in the base branch flight-sql...

lidavidm · 2021-12-20T23:05:45Z

Ah, right, we have a branch…How about I rebase the branch, and then we can rebase the PRs?

lidavidm · 2021-12-20T23:07:50Z

Hmm. I just rebased the branch…so do you mind trying to rebase again here? Sorry for the trouble.

This refactor is a prerequisite to adding Flight SQL integration tests, as the current integration tests do not allow Flight SQL usage (because it would cause circular dependencies)

…tegration tests

rafael-telles · 2021-12-20T23:13:00Z

No problem! Just rebased :)

lidavidm · 2021-12-20T23:26:30Z

Well, looks like the Docker registry isn't doing so well right now. I'll kick all the CI pipelines in a bit to try again.

lidavidm · 2021-12-21T13:19:55Z

Hmm, I wonder if the test failure in Dev is just because it's getting confused since we're on a branch. I think we can resolve that when we make the final PR from flight-sql into master, then.

lidavidm

Thanks, just one more nit that caught my eye.

lidavidm · 2021-12-21T13:38:44Z

...gration-tests/src/main/java/org/apache/arrow/flight/integration/tests/FlightSqlScenario.java

+        sqlClient.execute("SELECT STATEMENT", options), sqlClient);
+
+    IntegrationAssertions.assertEquals(sqlClient.executeUpdate("UPDATE STATEMENT", options),
+        10000L);


Just a nit, but can we move these numbers to constants so it's clear what they represent? (UPDATE_STATEMENT_EXPECTED_ROWS or something)

(The same goes for the producer, C++, etc.)

Thanks! Done

… and Java)

rafael-telles · 2021-12-21T17:46:00Z

Hey @lidavidm ! Are we good to merge this on flight-sql branch and then on master?

Thank you

lidavidm · 2021-12-21T17:53:35Z

We should be. I just kicked off CI.

lidavidm · 2021-12-21T18:42:36Z

I think the failures are spurious. (I filed ARROW-15181 for the crash in the Flight tests.)

lidavidm

Thanks. Sorry, I just noticed a couple more small things - we can merge into the branch next.

cpp/src/arrow/flight/integration_tests/test_integration.cc

rafael-telles · 2021-12-21T21:07:04Z

Glad to see everything passing! Except to that "Dev/ Source Release and Merge Script", but it indeed seem to fail because we are in our own branch

…n tests This adds Flight SQL scenarios for integration tests for C++ and Java implementations. The integrations tests assert that: - RPC objects built on clients are parsed correctly on servers - Arrow vectors built on servers have the expected Arrow schemas when received on clients Note: Had to separate integration tests in a new module within Java and C++ to avoid circular dependencies (Flight <-> Flight SQL) While working on this I found some inconsistencies between both implementation, this PR also fixes these problems. Closes #11989 from rafael-telles/flight-sql-integration-tests-2 Authored-by: Rafael Telles <rafael@telles.dev> Signed-off-by: David Li <li.davidm96@gmail.com>

lidavidm · 2021-12-21T21:25:00Z

Mega-PR is up at #12013

github-actions bot added Component: FlightRPC Component: C++ Component: Java labels Dec 17, 2021

lidavidm reviewed Dec 17, 2021

View reviewed changes

rafael-telles force-pushed the flight-sql-integration-tests-2 branch 2 times, most recently from 94f98fe to 5866b91 Compare December 20, 2021 22:33

github-actions bot added Component: GLib Component: Python Component: R Component: Parquet labels Dec 20, 2021

rafael-telles force-pushed the flight-sql-integration-tests-2 branch from 5866b91 to 78352e9 Compare December 20, 2021 22:37

lidavidm force-pushed the flight-sql branch from 9c2d8ed to 22bd624 Compare December 20, 2021 23:07

rafael-telles added 6 commits December 20, 2021 20:12

Separate Flight integration tests into new modules

9b55daa

This refactor is a prerequisite to adding Flight SQL integration tests, as the current integration tests do not allow Flight SQL usage (because it would cause circular dependencies)

Create integration tests for Flight SQL Java and C++

0121012

Implement validation for prepared statement's parameter binding on in…

163fa77

…tegration tests

Skip Rust and Go from Flight SQL integration test scenario

4d384ab

Fix formatting issues on C++ integration tests

f6bbf6c

Fix TestFlightSqlServer#TestCommandGetSqlInfoNoInfo failure on CI

97fe3ce

rafael-telles force-pushed the flight-sql-integration-tests-2 branch from 78352e9 to 97fe3ce Compare December 20, 2021 23:12

lidavidm approved these changes Dec 21, 2021

View reviewed changes

nit: Extract expected results for update statements as constants (C++…

f273156

… and Java)

lidavidm approved these changes Dec 21, 2021

View reviewed changes

cpp/src/arrow/flight/integration_tests/test_integration.cc Outdated Show resolved Hide resolved

cpp/src/arrow/flight/integration_tests/test_integration.cc Outdated Show resolved Hide resolved

nit: Use quotes on arrow includes on test_integration.cc

4b21e94

lidavidm mentioned this pull request Dec 21, 2021

ARROW-14421: [C++] Implement Flight SQL #12013

Closed

lidavidm closed this in 968e6ea Dec 23, 2021

asfimport mentioned this pull request Dec 23, 2021

[C++][FlightRPC][Integration][Java] Implement Flight RPC integration tests #30623

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ARROW-15112: [Integration][C++][Java] Implement Flight SQL integration tests #11989

ARROW-15112: [Integration][C++][Java] Implement Flight SQL integration tests #11989

rafael-telles commented Dec 17, 2021 •

edited

Loading

github-actions bot commented Dec 17, 2021

rafael-telles commented Dec 17, 2021

lidavidm left a comment

lidavidm Dec 17, 2021

rafael-telles Dec 20, 2021

lidavidm commented Dec 20, 2021

rafael-telles commented Dec 20, 2021

rafael-telles commented Dec 20, 2021

lidavidm commented Dec 20, 2021

rafael-telles commented Dec 20, 2021

lidavidm commented Dec 20, 2021

lidavidm commented Dec 20, 2021

rafael-telles commented Dec 20, 2021

lidavidm commented Dec 20, 2021

lidavidm commented Dec 21, 2021

lidavidm left a comment

lidavidm Dec 21, 2021

lidavidm Dec 21, 2021

rafael-telles Dec 21, 2021

rafael-telles commented Dec 21, 2021

lidavidm commented Dec 21, 2021

lidavidm commented Dec 21, 2021

lidavidm left a comment

rafael-telles commented Dec 21, 2021

lidavidm commented Dec 21, 2021

ARROW-15112: [Integration][C++][Java] Implement Flight SQL integration tests #11989

ARROW-15112: [Integration][C++][Java] Implement Flight SQL integration tests #11989

Conversation

rafael-telles commented Dec 17, 2021 • edited Loading

github-actions bot commented Dec 17, 2021

rafael-telles commented Dec 17, 2021

lidavidm left a comment

Choose a reason for hiding this comment

lidavidm Dec 17, 2021

Choose a reason for hiding this comment

rafael-telles Dec 20, 2021

Choose a reason for hiding this comment

lidavidm commented Dec 20, 2021

rafael-telles commented Dec 20, 2021

rafael-telles commented Dec 20, 2021

lidavidm commented Dec 20, 2021

rafael-telles commented Dec 20, 2021

lidavidm commented Dec 20, 2021

lidavidm commented Dec 20, 2021

rafael-telles commented Dec 20, 2021

lidavidm commented Dec 20, 2021

lidavidm commented Dec 21, 2021

lidavidm left a comment

Choose a reason for hiding this comment

lidavidm Dec 21, 2021

Choose a reason for hiding this comment

lidavidm Dec 21, 2021

Choose a reason for hiding this comment

rafael-telles Dec 21, 2021

Choose a reason for hiding this comment

rafael-telles commented Dec 21, 2021

lidavidm commented Dec 21, 2021

lidavidm commented Dec 21, 2021

lidavidm left a comment

Choose a reason for hiding this comment

rafael-telles commented Dec 21, 2021

lidavidm commented Dec 21, 2021

rafael-telles commented Dec 17, 2021 •

edited

Loading