Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/24544 )
Change subject: IMPALA-12422: Add Impala/Trino Iceberg V3 interop custom cluster test suite ...................................................................... Patch Set 1: (7 comments) http://gerrit.cloudera.org:8080/#/c/24544/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/24544/1//COMMIT_MSG@14 PS1, Line 14: TRINO_RESULTS Why is TRINO_RESULTS needed where HIVE_QUERY uses simply RESULTS http://gerrit.cloudera.org:8080/#/c/24544/1//COMMIT_MSG@26 PS1, Line 26: cluster setup or run-all-tests.sh. start() is : guarded by an inter-process file lock and the container is not stopped : from an individual pytest-xdist worker, so the suite is safe to run : concurrently (e.g. -n3). A ticket could be created for allowing Trino during data loading, both to support things not yet in Hive and to possibly speed loading up compared to Hive. http://gerrit.cloudera.org:8080/#/c/24544/1//COMMIT_MSG@38 PS1, Line 38: test_iceberg_trino_interop Does interop work with Hive tables too? I would prefer to focus this patch on adding the test tooling, and adding more complex Iceberg tests in another patch. This would make it easier to backport this patch if needed to older branches without Iceberg v3 support. Ideally there would be a test_trino_interop.py that looks for basic operations: - can Trino see databases/tables created by Impala? (e.g. from functional db) - can Trino create and write databases/tables and is Impala able to read these? http://gerrit.cloudera.org:8080/#/c/24544/1/tests/common/test_result_verifier.py File tests/common/test_result_verifier.py: http://gerrit.cloudera.org:8080/#/c/24544/1/tests/common/test_result_verifier.py@521 PS1, Line 521: Because : Trino's SQL type names differ from Impala's, every column is compared as an : opaque STRING (i.e. exact textual match); we do not cross-check column types. Can you create a ticket for these? I agree that this is not critical now, but it would be nice to have typed result checking. http://gerrit.cloudera.org:8080/#/c/24544/1/tests/common/trino_cluster.py File tests/common/trino_cluster.py: http://gerrit.cloudera.org:8080/#/c/24544/1/tests/common/trino_cluster.py@97 PS1, Line 97: def _build_image(self): : """Build the Trino Docker image via build-trino-docker-image.sh. This is done : lazily (only when the image is missing) so that non-interop workflows never : pay the cost; layer caching makes subsequent builds cheap. Raises : TrinoUnavailable on failure so callers can skip.""" : script = os.path.join(IMPALA_HOME, 'testdata/bin/build-trino-docker-image.sh') It is nice to avoid cost when we don't need this, but I think that it would be better to move this to an earlier phase of Impala builds, for example before dataload. If tests use trino that are in the core test suite then we generally can't avoid the cost of building it, and doing it randomly at the first test is harder to debug than doing it at a fixed place. It could be skipped with similar conditions as in tests (@SkipIf.not_hdfs). http://gerrit.cloudera.org:8080/#/c/24544/1/tests/common/trino_cluster.py@183 PS1, Line 183: """Run a single SQL statement through the containerized Trino CLI. Returns a : (stdout, stderr, returncode) tuple; does NOT raise on query error so callers : can decide how to report failures.""" : cmd = ['docker', 'exec', self.container, 'trino', : '--server=' + self.server, '--output-format=' + output_format] An alternative would be to add https://github.com/trinodb/trino-python-client (https://pypi.org/project/trino/) to the dependencies. This is a similar python dbapi client as impyla. http://gerrit.cloudera.org:8080/#/c/24544/1/tests/custom_cluster/test_iceberg_trino_interop.py File tests/custom_cluster/test_iceberg_trino_interop.py: http://gerrit.cloudera.org:8080/#/c/24544/1/tests/custom_cluster/test_iceberg_trino_interop.py@43 PS1, Line 43: @CustomClusterTestSuite.with_args(cluster_size=3) It seems more comfortable to me to add this feature to CustomClusterTestSuite, with_args could have a has_trino parameter taking care of the things below. For example I would also like to use Trino for geospatial interop tests, which may possible go to another test class. -- To view, visit http://gerrit.cloudera.org:8080/24544 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I7706244f909f32dff4d390537e71877b0635c661 Gerrit-Change-Number: 24544 Gerrit-PatchSet: 1 Gerrit-Owner: Zoltan Borok-Nagy <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Comment-Date: Tue, 07 Jul 2026 07:43:25 +0000 Gerrit-HasComments: Yes
