impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "John Russell (Code Review)" <>
Subject [Impala-ASF-CR] Release note updates for Impala 2.8
Date Fri, 27 Jan 2017 01:00:57 GMT
John Russell has posted comments on this change.

Change subject: Release note updates for Impala 2.8

Patch Set 3:

Commit Message:

Line 28: Change-Id: I7c47f422e509cec6d3eb8aaa82294b584f393aed
> I won't have time to do a thorough review here but this commit message need
File docs/topics/impala_incompatible_changes.xml:

PS1, Line 91: now
> typo

PS1, Line 111:  There was a period during early experimental versions of Impala + Kudu where

             :             non-primary-key olumns had the <codeph>NOT NULL</codeph
> I don't think this is correct. AFAIK the default behavior is the same and w
I clarified the wording a bit.

PS1, Line 111:  There was a period during early experimental versions of Impala + Kudu where

             :             non-primary-key olumns had the <codeph>NOT NULL</codeph
> The GA behavior is columns are nullable by default, unless they are part of
File docs/topics/impala_incompatible_changes.xml:

PS3, Line 58: Impala Incompatible Changes Introduced in Impala 2.8.x
> This wording is surprising to me. Why is the first word needed at all?
There was something I saw relating to cross-reference links, but I think that was more applicable
in Cloudera-specific docs. I removed the leading "Impala" globally.

Line 65:             They were output in uppercase by mistake, but only for a single Impala
release (Impala 2.7).
> It makes changes easier to review when the lines are 90 characters or fewer
Good point. I'm going to wait and do that globally as a separate gerrit review. I suggest
for entirely new text like this, go into gerrit settings and set 'left side' to 'hide'.

PS3, Line 111:  
> space at end of line

Line 112:             non-primary-key olumns had the <codeph>NOT NULL</codeph>
attribute by default.
> "olumns"

Line 1523:   <concept id="incompatible_changes_07" audience="hidden">
> Why hide this?
The beta releases are so intertwined with CDH and CM release numbers, I want to make a blanket
rule to hide or remove such stale historical info. It is more of a Cloudera convention to
let release notes accrete forever. Contrast with Apache Kudu -
- where only the changes for the current release are listed in the docs for that release.
File docs/topics/impala_new_features.xml:

PS1, Line 66: The <codeph>COMPUTE STATS</codeph> statement can
            :                 take advantage of multithreading.
> Not sure if you want to be more specific here as the default for Parquet ta

PS1, Line 90: 
> move left?
You mean indentation-wise? For the XML tagging in the docs, I use Berkeley-style indenting,

  some text

is the equivalent of

  some C code

PS1, Line 91: >
            :             <li>
            :               <p rev="IMPALA-4397">
> SORTBY adds ordering for non-partition key columns to better support the ef

PS1, Line 103: that use dynamic 
> move to p?
Good catch. I use <p> even where optional inside list items, and prefer to apply attributes
on the inner <p> tags, because there's the possibility of reusing those paragraphs verbatim
in the detailed writeup of each feature.

PS1, Line 238: STAMP</codeph>, <codeph>DECIMAL<
> needed?
Clunky wording, I'll reword just to emphasize not to worry about inserting data in small batches
like we warn people about for other kinds of tables.

PS1, Line 253: of the Kudu data makes it more efficient than with HDFS
> dimitris should review
I'll mention that Sentry can be bypassed and then have Dimitris review the subsequent patch

PS1, Line 290: 
             :             <li>
Is this purely a CM feature? If so I won't mention it in the upstream docs.

To view, visit
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: I7c47f422e509cec6d3eb8aaa82294b584f393aed
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: John Russell <>
Gerrit-Reviewer: Alex Behm <>
Gerrit-Reviewer: Ambreen Kazi <>
Gerrit-Reviewer: Dan Hecht <>
Gerrit-Reviewer: Dimitris Tsirogiannis <>
Gerrit-Reviewer: Greg Rahn <>
Gerrit-Reviewer: Jim Apple <>
Gerrit-Reviewer: John Russell <>
Gerrit-Reviewer: Laurel Hale <>
Gerrit-Reviewer: Matthew Jacobs <>
Gerrit-HasComments: Yes

View raw message