impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "John Russell (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-2522: Add doc for sortby() and clustered hints
Date Tue, 25 Apr 2017 22:14:41 GMT
John Russell has posted comments on this change.

Change subject: IMPALA-2522: Add doc for sortby() and clustered hints
......................................................................


Patch Set 4:

(10 comments)

http://gerrit.cloudera.org:8080/#/c/5655/4/docs/shared/impala_common.xml
File docs/shared/impala_common.xml:

PS4, Line 2639: minimizes
> Technically this is incorrect now, since CLUSTERED reduces the number of co
Done


PS4, Line 2672: perform organize
> This reads as if one word was superfluous
Took out "perform".


PS4, Line 2674: risking
> to reduce the risk of an...
Done


PS4, Line 2686: large insert operations
              :             that use dynamic partitioning
> This surprises me. Maybe I'm missing something?
Not applicable since this whole SORTBY piece is being removed.


Line 3707:     <section id="kudu_common">
> ?
Just a minor housekeeping change for easier doc source maintenance.


http://gerrit.cloudera.org:8080/#/c/5655/4/docs/topics/impala_hints.xml
File docs/topics/impala_hints.xml:

Line 87:       either the <codeph>/* */</codeph> or <codeph>--</codeph>
notation.
> We might want to state that the use of [] is discourage and will be depreca
Done


PS4, Line 91: immediately before the hint name
> It must only be present immediately before the hint list, i.e. the first hi
Done


Line 129: </codeblock>
> we should have one example with multiple hints.
I included an example like that in the discussion right before this list. Now with SORTBY
gone, seems like the only combination for INSERT could be CLUSTERED and SHUFFLE or NOSHUFFLE.
So I prefer not to repeat the same example twice so close together. Let's see how the result
looks when all is said and done.


http://gerrit.cloudera.org:8080/#/c/5655/4/docs/topics/impala_insert.xml
File docs/topics/impala_insert.xml:

Line 67: hint_with_dashes ::= -- +SHUFFLE | -- +NOSHUFFLE <ph rev="IMPALA-4163">| --
+SORTBY(<varname>col</varname>, <varname>col</varname> ...)]</ph>
> Add CLUSTERED to the list?
Done. I added it under hints_with_dashes and hints_with_cstyle_comments. Not under hints_with_brackets.


Line 72:   (Note: with this hint format, the square brackets are part of the syntax.)
> Maybe mention that this will deprecated in the next compatibility-breaking 
Done


-- 
To view, visit http://gerrit.cloudera.org:8080/5655
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Id3c1da9a87ace361b096fa73d8504b2f54e75bed
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: John Russell <jrussell@cloudera.com>
Gerrit-Reviewer: John Russell <jrussell@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message