GH-46513: [Archery] Add external library support in Archery

[Alex-PLACET]

Rationale for this change

Address #46513:
Add the possibility to use a library which is not part of the "official" implementations, in the integration tests

What changes are included in this PR?

Add new options in the CLI to be able to define the path to a library to test, and it's compatibility with the tests (IPC producer/consumer, c data array/schema impoter/exporter).
Add tester_external_library.py
Modify runner.py to use ExternalLibraryTester

Are these changes tested?

Yes localy and on the Sparrow CI:
https://github.com/man-group/sparrow/pull/426/checks#step:9:1
https://github.com/man-group/sparrow/pull/426/checks#step:9:7051

Are there any user-facing changes?

Yes, new options in the CLI and environment variable supported.
there is no breaking change.

• GitHub Issue: [Archery] Add the possibility to test a third party library #46513

[github-actions]

Thanks for opening a pull request!

If this is not a minor PR. Could you open an issue for this pull request on GitHub? https://github.com/apache/arrow/issues/new/choose

Opening GitHub issues ahead of time contributes to the Openness of the Apache Arrow project.

Then could you also rename the pull request title in the following format?

GH-${GITHUB_ISSUE_ID}: [${COMPONENT}] ${SUMMARY}

or

MINOR: [${COMPONENT}] ${SUMMARY}

See also:

• Other pull requests
• Contribution Guidelines - Contributing Overview

[github-actions]

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

[kou]

Could you add a document how to use this?

[kou]

Suggested change

	@click.option('--external-library-IPC-producer', type=bool, default=False,
	@click.option('--external-library-ipc-producer', type=bool, default=False,

[Alex-PLACET]

Ok fixed

[kou]

Suggested change

	@click.option('--external-library-IPC-consumer', type=bool, default=False,
	@click.option('--external-library-ipc-consumer', type=bool, default=False,

[Alex-PLACET]

Ok fixed

[kou]

Do we need this...?

Suggested change

	enabled_implementations = True
	if args[param]:
	enabled_implementations = True

[Alex-PLACET]

I finally simplified the code to raise an error if no implementation is selected

[kou]

Suggested change

	if enabled_formats == False:
	if not enabled_formats:

[Alex-PLACET]

I finally simplified the code to raise an error if no format is selected

[kou]

Suggested change

	if enabled_implementations == False:
	if not enabled_implementations:

[Alex-PLACET]

fixed

[kou]

If we want to use bool for this, we may need to rename this something like have_enabled_implementation.

[Alex-PLACET]

fixed

[kou]

Could you add a document how to use this?

[pitrou]

We already have docs for the Integration tests in https://github.com/apache/arrow/blob/main/docs/source/format/Integration.rst, I suggest augmenting this document rather than the README.

[pitrou]

Can you ensure you wrap long lines to max 90 ~chars? This makes the code more readable, especially in diff views.

[pitrou]

The Final annotation seems pedantic to me. Besides, any returns a boolean, so no annotation should be required.

[pitrou]

Same here.

[pitrou]

A more general comment. The approach in this PR is a bit inflexible, as any implementation-specific setting is exposed as a dedicated CLI option. This may proliferate CLI options as we need more settings to be exposed.

For example, some per-implementation skips are currently hardcoded in the data generation (example). Perhaps we would like an external implementation to tell which tests it needs to skip. That will complicate the CLI quite a bit.

Another possibility is to allow passing a custom YAML file that records all the implementation-specific settings. The CLI would only have one additional argument and the YAML would carry the rest of the information in a structured way.

But of course, Sparrow might remain the only user of this functionality, so it's also up to you. What do you think @Alex-PLACET ?

[pitrou]

@Alex-PLACET Would you like to comment on #46530 (comment) ? I think we'll need something more flexible anyway, because at some points some skips will be required.

[Alex-PLACET]

Apologies for the delayed response.
You’re right, the skipping feature is indeed missing. This wasn’t an issue for Sparrow, as its primary goal was full compatibility.
The main purpose of these changes is to enable non-Apache developers to test their libraries in their own CI environments. We’re already using this branch in our CI to run Sparrow against the Apache Arrow C++ implementation.
Using a YAML file could be a good solution, but I don’t have the bandwidth to implement it at the moment.
If you want, we can close this pull request and create a new one for the YAML later.

[pitrou]

If you want, we can close this pull request and create a new one for the YAML later.

What do you think @kou @raulcd ?

[kou]

If we use the YAML approach, I want to replace the all existing testers with YAML configurations.
In the case, we may want to specify multiple YAML files. For example:

apache/arrow provides one YAML file for all known implementations:

archery integration ... --config /.../apache/arrow/dev/archery/data/builtins.yaml ...

apache/arrow provides one YAML file per known implementation:

archery integration ... --config=/.../apache/arrow/dev/archery/data/{cpp,go,js}.yaml ...

Each repository provides a YAML file for their implementation:

archery integration ... \
  --config=/.../apache/arrow/dev/archery/data/cpp.yaml
  --config=/.../apache/arrow-go/integration/archery.yaml
  --config=/.../apache/arrow-js/integration/archery.yaml
  ...

[pitrou]

If we use the YAML approach, I want to replace the all existing testers with YAML configurations.

This sounds an order of magnitude more difficult than simply supporting YAML for third-party (external) implementations. For example, the Java tester uses JPype to load and access the JVM in-process from Python.