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-5789: Add always false flag in bloom filter
Date Fri, 13 Oct 2017 20:34:05 GMT
Tim Armstrong has posted comments on this change. ( )

Change subject: IMPALA-5789: Add always_false flag in bloom filter

Patch Set 4:

Commit Message:
PS4, Line 15: 
Did you do any perf runs? It would be good to verify that the extra flag checking doesn't
affect perf (I suspect it doesn't).

It would also be good to confirm that testdata/workloads/targeted-perf/queries/primitive_empty_build_join_1.test
gets faster (I think it should!).
File be/src/exec/
PS4, Line 403: std::
Shouldn't need std:: - it's imported in common/names.h
File be/src/exec/
PS4, Line 107:     if (!BaseSequenceScanner::FileFormatIsSequenceBased(
Might be more readable to factor subexpression into variable e.g. is_sequence_based.
File tests/custom_cluster/
PS4, Line 53:   seq_table_suffixes = ['_avro', '_rc', '_seq']
We'd normally create a test matrix based on these. Is the idea here to avoid restarting the
cluster for each file format? If so it would be good to leave a comment so that readers understand
PS4, Line 82:   def test_skip_file(self, cursor):
Does this need to be a custom cluster test? I.e. does it need a special minicluster to execute.
It's best to make things query tests if possible since starting a cluster is slow and the
tests aren't parallelisable.

To view, visit
To unsubscribe, visit

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If680240a3cd4583fc97c3192177d86d9567c4f8d
Gerrit-Change-Number: 8170
Gerrit-PatchSet: 4
Gerrit-Owner: Tianyi Wang <>
Gerrit-Reviewer: Sailesh Mukil <>
Gerrit-Reviewer: Thomas Tauber-Marshall <>
Gerrit-Reviewer: Tianyi Wang <>
Gerrit-Reviewer: Tim Armstrong <>
Gerrit-Comment-Date: Fri, 13 Oct 2017 20:34:05 +0000
Gerrit-HasComments: Yes

  • Unnamed multipart/alternative (inline, 8-Bit, 0 bytes)
View raw message