Mihaly Szjatinya has posted comments on this change. ( http://gerrit.cloudera.org:8080/24590 )
Change subject: IMPALA-9821: Change DataSketches functions to return BINARY ...................................................................... Patch Set 12: (1 comment) http://gerrit.cloudera.org:8080/#/c/24590/6/testdata/workloads/functional-query/queries/QueryTest/datasketches-hll-hive-orc.test File testdata/workloads/functional-query/queries/QueryTest/datasketches-hll-hive-orc.test: http://gerrit.cloudera.org:8080/#/c/24590/6/testdata/workloads/functional-query/queries/QueryTest/datasketches-hll-hive-orc.test@16 PS6, Line 16: > If you want to test scenario that originally could cause the type mismatch Step 1 without [this](https://github.com/apache/Impala/blob/master/be/src/exec/orc/orc-metadata-utils.cc#L498) piece of code from IMPALA-9482 actually reproduces the original error stated in IMPALA-9821. You're right that Step 1 never historically produced that error. I.e. if we take IMPALA-9482 in its entirety, without it there wouldn't be BINARY support for Impala at all, not only the ORC part, and hence Step 1 comment's claim is factually incorrect. If my understanding is right so far then changing the phrasing as I did now would remove the incorrectness. But if you ask me personally, I think this is a level of imprecision acceptable for tests' comments. I.e. it's kind of obvious that in the context of IMPALA-9821 referring to 'before IMPALA-9482' is a semantic shortcut for saying 'without the minimal part of IMPALA-9482 that actually allows us to demonstrate our point' or even 'allows us to understand the genesis of error in IMPALA-9821, why it was reported and how it was fixed'. But hey, if there's a way to make it even more straightforward, it's always a good idea. And I think I understand your concern from IMPALA-9482's author pov :) As for ALTER TABLE, IMHO there's no need to craft the precise pre-IMPALA-9482 way to invoke the error with ALTER TABLE to demonstrate the essence of Step 1, given that we already have a simpler way to demonstrate it. IMHO doing that, and especially an extra cast in Step 2 would make the test much less clear overall. -- To view, visit http://gerrit.cloudera.org:8080/24590 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Id4a6b54089dd356e37257bc24adeb1eb98e82c25 Gerrit-Change-Number: 24590 Gerrit-PatchSet: 12 Gerrit-Owner: Mihaly Szjatinya <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Mihaly Szjatinya <[email protected]> Gerrit-Reviewer: Peter Rozsa <[email protected]> Gerrit-Reviewer: Zoltan Borok-Nagy <[email protected]> Gerrit-Comment-Date: Thu, 24 Sep 2026 15:30:50 +0000 Gerrit-HasComments: Yes
