Daniel Becker has posted comments on this change. ( http://gerrit.cloudera.org:8080/23342 )
Change subject: IMPALA-13125: Fix pairwise test vector generation ...................................................................... Patch Set 7: (6 comments) Thanks for working on this, Csaba. http://gerrit.cloudera.org:8080/#/c/23342/7/testdata/bin/generate-test-vectors.py File testdata/bin/generate-test-vectors.py: http://gerrit.cloudera.org:8080/#/c/23342/7/testdata/bin/generate-test-vectors.py@54 PS7, Line 54: # TODO IMPALA-13125 turned out to be not completely reliably - this script may also miss Is this TODO still needed? http://gerrit.cloudera.org:8080/#/c/23342/7/tests/common/test_vector.py File tests/common/test_vector.py: http://gerrit.cloudera.org:8080/#/c/23342/7/tests/common/test_vector.py@320 PS7, Line 320: vector Nit: 'vectors'? http://gerrit.cloudera.org:8080/#/c/23342/7/tests/common/test_vector.py@324 PS7, Line 324: in the result Nit: this is not incorrect but could cause a misunderstanding because there is also a 'results' output variable. http://gerrit.cloudera.org:8080/#/c/23342/7/tests/common/test_vector.py@343 PS7, Line 343: for i, x in enumerate(v): : for j, y in enumerate(v): : if i >= j: continue : t = (i, x, j, y) : pairs_covered.add(t) Could we store the tuples near L335 instead, to save another iteration? http://gerrit.cloudera.org:8080/#/c/23342/7/tests/query_test/test_queries.py File tests/query_test/test_queries.py: http://gerrit.cloudera.org:8080/#/c/23342/7/tests/query_test/test_queries.py@128 PS7, Line 128: # set timestamp options to get consistent results for both format. This is the same as L171-175. Do you think it's worth extracting into a function? http://gerrit.cloudera.org:8080/#/c/23342/7/tests/query_test/test_queries.py@156 PS7, Line 156: return Nit: duplicate "return"? See also L182. -- To view, visit http://gerrit.cloudera.org:8080/23342 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I419c24659a08d8d6592fadbbd5b764ff73cbba3e Gerrit-Change-Number: 23342 Gerrit-PatchSet: 7 Gerrit-Owner: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Daniel Becker <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Riza Suminto <[email protected]> Gerrit-Comment-Date: Wed, 27 Aug 2025 12:54:22 +0000 Gerrit-HasComments: Yes
