impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Tim Armstrong (Code Review)" <>
Subject [Impala-ASF-CR] IMPALA-3200: more buffer pool end-to-end tests
Date Wed, 02 Aug 2017 21:30:00 GMT
Tim Armstrong has posted comments on this change.

Change subject: IMPALA-3200: more buffer pool end-to-end tests

Patch Set 5:


If I understood correctly, the concern is that someone will skip it on core without understanding
the consequences, right?

I could move the tests to functional-query to avoid that, but it seems like that goes against
the idea of associating tests with workloads.
File testdata/workloads/functional-query/queries/QueryTest/spilling-sorts-exhaustive.test:

PS5, Line 86: from (select cast(l_comment as char(200)) char_col
> I realize this is a move, but is a varchar(200) test similar to this one ne
It was originally necessary to have two char(n) tests because there were two different representations
of char, with the switchover happening at 128 bytes. That is no longer the case after "IMPALA-5560:
always store CHAR(N) inline in tuple". We could probably remove this test and not lose meaningful

varchar(n) only has a single representation so the single test should be fine.
File testdata/workloads/targeted-perf/targeted-perf_dimensions.csv:

PS5, Line 1: file_format: text,seq,rc,avro,parquet,kudu
> Thanks for adding this extra coverage here and elsewhere. Have you verified
We didn't have any pre-existing targeted-perf tests, except for experiments/,
which seems to have already been broken. I ran with it --collect-only and it showed tests


I ran TestTpchPrimitivesMemLimitError and confirmed that it ran with Parquet (the other formats
are filtered out for that test).
File tests/custom_cluster/

PS5, Line 321:       impalad_args=impalad_admission_ctrl_flags(1, 1, 10 * 1024 * 1024,
             :           1024 * 1024 * 1024)
> Nit: There are so many numbers here it might be nice to consider kwargs-sty

PS5, Line 332:     assert"Failed to get minimum memory reservation", str(ex))
> Micro-optimization nit: You can use the in operator in python for substring
File tests/query_test/

PS5, Line 92:     return 'tpch'
> Clarification: If someone later attempts to skip these tests except when in
Added comments with warnings about IMPALA-3947 to the applicable test classes.

PS5, Line 108:     print str(result)
> Remove?

PS5, Line 254:     return 'targeted-perf'
> Clarification: If someone later attempts to skip these tests except when in
Added a warning in a comment.

To view, visit
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: I554aa5ddfef4f8e75295596e720a14eee1afa17f
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <>
Gerrit-Reviewer: Michael Brown <>
Gerrit-Reviewer: Tim Armstrong <>
Gerrit-HasComments: Yes

View raw message