Zoltan Borok-Nagy 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 2:

(7 comments)

Thanks for the 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: RESULTS .test
> Why is TRINO_RESULTS needed where HIVE_QUERY uses simply RESULTS
Done


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 s
This was not the intention(/leftover TODO) of this CR, but filed IMPALA-15143. 
Please feel free to file any ticket for further improvements.


http://gerrit.cloudera.org:8080/#/c/24544/1//COMMIT_MSG@38
PS1, Line 38: TH_BYTE_ARRAY Parquet enco
> Does interop work with Hive tables too?
I refactored the code and created test_trino_interop.py, but kept 
test_iceberg_trino_interop.py as well. If it ever gets backported to an old 
version (which I doubt) the backporter can just omit 
test_iceberg_trino_interop.py.


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, b
Filed IMPALA-15149


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
I'd rather do it later when these test setups prove to be stable. Maybe it 
could be covered in the context of IMPALA-15143.


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-clie
If it's not necessary I'd rather not add extra dependencies.


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: @SkipIf.not_hdfs
> It seems more comfortable to me to add this feature to CustomClusterTestSui
Good idea, done.



--
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: 2
Gerrit-Owner: Zoltan Borok-Nagy <[email protected]>
Gerrit-Reviewer: Csaba Ringhofer <[email protected]>
Gerrit-Reviewer: Impala Public Jenkins <[email protected]>
Gerrit-Reviewer: Zoltan Borok-Nagy <[email protected]>
Gerrit-Comment-Date: Tue, 07 Jul 2026 19:33:59 +0000
Gerrit-HasComments: Yes

Reply via email to