impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Thomas Tauber-Marshall (Code Review)" <>
Subject [Impala-ASF-CR] IMPALA-3989: Display skew warning for poorly formatted Parquet files
Date Fri, 09 Dec 2016 22:10:25 GMT
Thomas Tauber-Marshall has posted comments on this change.

Change subject: IMPALA-3989: Display skew warning for poorly formatted Parquet files

Patch Set 4:

File be/src/exec/hdfs-scan-node-base.h:

Line 75:   bool poorly_formatted_file_warned;
Presumably you did this to prevent excessive logging, but I think its unnecessary. Removing
it won't result in excess logging because error messages are grouped by their error code (eg.
in the shell you'll just see one instance of the message followed by '(1 of X similar)', and
its better not to clutter up this struct.

If you're still worried about excessive logging, you could move the call to LogPoorlyFormattedParquetFileWarning
to the end of NextRowGroup outside of the 'while(true)'.

In fact, if you remove this, then LogPoorlyFormattedParquetFileWarning isn't really doing
much and you could eliminate it and just do the logging inline in NextRowGroup.
File common/thrift/

PS4, Line 315:    
Remove spaces, here and below.

Sorry, I know we're inconsistent about our Python style, but we try to mostly follow PEP8:
File tests/query_test/

PS4, Line 319: 'NumScannersWithMisalignedRowGroups'

PS4, Line 321: """
Move """ to next line.

Another place where we're inconsistent with our Python style:

PS4, Line 347: num_scanners_misaligned_row_groups

PS4, Line 349: excuting

PS4, Line 351: 'num_scanners_misaligned_row_groups'

PS4, Line 352: """
Move to next line.

PS4, Line 362: 'NumScannersWithMisalignedRowGroups:

To view, visit
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: Ibf48d978383d73efdade733a892e795ebd53c76a
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Attila Jeges <>
Gerrit-Reviewer: Attila Jeges <>
Gerrit-Reviewer: Michael Ho <>
Gerrit-Reviewer: Thomas Tauber-Marshall <>
Gerrit-HasComments: Yes

View raw message