impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Dimitris Tsirogiannis (Code Review)" <>
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. (

Change subject: IMPALA-5152: Introduce metadata loading phase

Patch Set 1:


First round of comments. Overall approach looks reasonable to me.
File fe/src/main/java/org/apache/impala/analysis/
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.
File fe/src/main/java/org/apache/impala/analysis/
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.
PS1, Line 518: analysisResult_.isAlterTableStmt() ||
             :             analysisResult_.isAlterViewStmt());
Curious, where did this come from?
File fe/src/main/java/org/apache/impala/analysis/
PS1, Line 2361: or the db
I don't think the existence of db is checked in this function anymore.
File fe/src/main/java/org/apache/impala/analysis/
PS1, Line 68: public
File fe/src/main/java/org/apache/impala/analysis/
PS1, Line 64: public abstract void collectTableRefs(List<TableRef> tblRefs, boolean
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.
File fe/src/main/java/org/apache/impala/service/
PS1, Line 823:    * ignored and not returned or added to 'loadedTables'.
Comment where defaultDb is used.
PS1, Line 825: private Set<TableName> getMissingTables
nit: can you put the following two functions below requestTblLoadAndWait so that the code
reads top-down?
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.
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).
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?
PS1, Line 885: If non-NULL
You mean the timeline, no?
PS1, Line 889: defaultDb
nit: I would call it sessionDb
PS1, Line 903: public
PS1, Line 916: requestedTbls
It seems that this is only used for the log message in L965. Can't you use a counter instead?
PS1, Line 919: final long debugLoggingFrequency = 10;
             :     final long retryLoadFrequency = 20;
nit: move them at the beginning of this function?
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.
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.
PS1, Line 1091: AnalysisContext analysisCtx = new AnalysisContext(queryCtx, authzConfig_,
              :     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
To unsubscribe, visit

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 <>
Gerrit-Reviewer: Bharath Vissapragada <>
Gerrit-Reviewer: Dimitris Tsirogiannis <>
Gerrit-Reviewer: Vuk Ercegovac <>
Gerrit-Comment-Date: Thu, 11 Jan 2018 00:26:57 +0000
Gerrit-HasComments: Yes

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