impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Dimitris Tsirogiannis (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-5152: Introduce metadata loading phase
Date Thu, 11 Jan 2018 00:26:57 GMT
Dimitris Tsirogiannis has posted comments on this change. ( http://gerrit.cloudera.org:8080/8958
)

Change subject: IMPALA-5152: Introduce metadata loading phase
......................................................................


Patch Set 1:

(19 comments)

First round of comments. Overall approach looks reasonable to me.

http://gerrit.cloudera.org:8080/#/c/8958/1/fe/src/main/java/org/apache/impala/analysis/AlterTableStmt.java
File fe/src/main/java/org/apache/impala/analysis/AlterTableStmt.java:

http://gerrit.cloudera.org:8080/#/c/8958/1/fe/src/main/java/org/apache/impala/analysis/AlterTableStmt.java@73
PS1, Line 73:     if (!fromClauseOnly) tblRefs.add(new TableRef(tableName_.toPath(), null));
Maybe replace the if check with a Preconditions.checkState(!fromClauseOnly); since it doesn't
make sense to call this function with true in this context.


http://gerrit.cloudera.org:8080/#/c/8958/1/fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java
File fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java:

http://gerrit.cloudera.org:8080/#/c/8958/1/fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java@66
PS1, Line 66: // Set in analyzeAndAuthorize().
            :   private ImpaladCatalog catalog_;
If I understand it correctly, that shouldn't be needed, given that analysis can proceed with
the LoadedTables state.


http://gerrit.cloudera.org:8080/#/c/8958/1/fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java@518
PS1, Line 518: analysisResult_.isAlterTableStmt() ||
             :             analysisResult_.isAlterViewStmt());
Curious, where did this come from?


http://gerrit.cloudera.org:8080/#/c/8958/1/fe/src/main/java/org/apache/impala/analysis/Analyzer.java
File fe/src/main/java/org/apache/impala/analysis/Analyzer.java:

http://gerrit.cloudera.org:8080/#/c/8958/1/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@2361
PS1, Line 2361: or the db
I don't think the existence of db is checked in this function anymore.


http://gerrit.cloudera.org:8080/#/c/8958/1/fe/src/main/java/org/apache/impala/analysis/FromClause.java
File fe/src/main/java/org/apache/impala/analysis/FromClause.java:

http://gerrit.cloudera.org:8080/#/c/8958/1/fe/src/main/java/org/apache/impala/analysis/FromClause.java@68
PS1, Line 68: public
@Override


http://gerrit.cloudera.org:8080/#/c/8958/1/fe/src/main/java/org/apache/impala/analysis/StatementBase.java
File fe/src/main/java/org/apache/impala/analysis/StatementBase.java:

http://gerrit.cloudera.org:8080/#/c/8958/1/fe/src/main/java/org/apache/impala/analysis/StatementBase.java@64
PS1, Line 64: public abstract void collectTableRefs(List<TableRef> tblRefs, boolean
fromClauseOnly);
If most of the statement classes that extend StatementBase don't return anything you can just
provide a default implementation here and avoid adding a function in all these cases.


http://gerrit.cloudera.org:8080/#/c/8958/1/fe/src/main/java/org/apache/impala/service/Frontend.java
File fe/src/main/java/org/apache/impala/service/Frontend.java:

http://gerrit.cloudera.org:8080/#/c/8958/1/fe/src/main/java/org/apache/impala/service/Frontend.java@823
PS1, Line 823:    * ignored and not returned or added to 'loadedTables'.
Comment where defaultDb is used.


http://gerrit.cloudera.org:8080/#/c/8958/1/fe/src/main/java/org/apache/impala/service/Frontend.java@825
PS1, Line 825: private Set<TableName> getMissingTables
nit: can you put the following two functions below requestTblLoadAndWait so that the code
reads top-down?


http://gerrit.cloudera.org:8080/#/c/8958/1/fe/src/main/java/org/apache/impala/service/Frontend.java@866
PS1, Line 866: org.apache.impala.analysis.Path.getCandidateTables
nit: you can use import static if you want to avoid the classpath inside the function.


http://gerrit.cloudera.org:8080/#/c/8958/1/fe/src/main/java/org/apache/impala/service/Frontend.java@872
PS1, Line 872: LoadedTables
The name sounds a bit generic. Maybe LoadedTablesCache or something to indicate the temporal
nature of it (i.e. it is specific to a query not a global thing).


http://gerrit.cloudera.org:8080/#/c/8958/1/fe/src/main/java/org/apache/impala/service/Frontend.java@873
PS1, Line 873: public final ImpaladCatalog catalog;
I see that this is used for initializing the Analyzer. But given that the analysis proceeds
based on the loaded tables (my understanding from the commit msg), does the Analyzer still
need it?


http://gerrit.cloudera.org:8080/#/c/8958/1/fe/src/main/java/org/apache/impala/service/Frontend.java@885
PS1, Line 885: If non-NULL
You mean the timeline, no?


http://gerrit.cloudera.org:8080/#/c/8958/1/fe/src/main/java/org/apache/impala/service/Frontend.java@889
PS1, Line 889: defaultDb
nit: I would call it sessionDb


http://gerrit.cloudera.org:8080/#/c/8958/1/fe/src/main/java/org/apache/impala/service/Frontend.java@903
PS1, Line 903: public
public?


http://gerrit.cloudera.org:8080/#/c/8958/1/fe/src/main/java/org/apache/impala/service/Frontend.java@916
PS1, Line 916: requestedTbls
It seems that this is only used for the log message in L965. Can't you use a counter instead?


http://gerrit.cloudera.org:8080/#/c/8958/1/fe/src/main/java/org/apache/impala/service/Frontend.java@919
PS1, Line 919: final long debugLoggingFrequency = 10;
             :     final long retryLoadFrequency = 20;
nit: move them at the beginning of this function?


http://gerrit.cloudera.org:8080/#/c/8958/1/fe/src/main/java/org/apache/impala/service/Frontend.java@924
PS1, Line 924: if (issueLoadRequest) {
Since we know there are missing tbls (condition in while), why do we need this check? The
way I read this is "there are missing tbls but I will not ask them to be loaded" and I am
not sure I understand the reason.


http://gerrit.cloudera.org:8080/#/c/8958/1/fe/src/main/java/org/apache/impala/service/Frontend.java@943
PS1, Line 943: Tables remaining: %s
If views are used, that number may go up and down and I am wondering if this may cause some
confusion to anyone looking at the logs.


http://gerrit.cloudera.org:8080/#/c/8958/1/fe/src/main/java/org/apache/impala/service/Frontend.java@1091
PS1, Line 1091: AnalysisContext analysisCtx = new AnalysisContext(queryCtx, authzConfig_,
timeline);
              :     AnalysisResult analysisResult =
              :         analysisCtx.analyzeAndAuthorize(queryCtx.client_request.stmt, this);
As is today, we have the following call sequence: Frontend::createExecRequest()->AnalysisContext::analyzeAndAuthorize()->Frontend::requestTableLoadAndWait().
I think it may make more sense, to first collect the required tbls state (i.e. load any missing
ones) and pass that to the AnalysisContext to perform the analysis.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I68d32d5acd4a6f6bc6cedb05e6cc5cf604d24a55
Gerrit-Change-Number: 8958
Gerrit-PatchSet: 1
Gerrit-Owner: Alex Behm <alex.behm@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bharathv@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dtsirogiannis@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <vercegovac@cloudera.com>
Gerrit-Comment-Date: Thu, 11 Jan 2018 00:26:57 +0000
Gerrit-HasComments: Yes

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