[IOTDB-340/339] Remove unnecessary getting data types from MManager #677
Merged
qiaojialin merged 12 commits intoapache:masterfrom Dec 28, 2019
liutaohua:remove_mmanager_for_query
Merged
[IOTDB-340/339] Remove unnecessary getting data types from MManager #677qiaojialin merged 12 commits intoapache:masterfrom liutaohua:remove_mmanager_for_query
qiaojialin merged 12 commits intoapache:masterfrom
liutaohua:remove_mmanager_for_query
Conversation
jt2594838
requested changes
Dec 27, 2019
server/src/main/java/org/apache/iotdb/db/qp/strategy/PhysicalGenerator.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/apache/iotdb/db/qp/strategy/PhysicalGenerator.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/apache/iotdb/db/query/dataset/DeviceIterateDataSet.java
Outdated
Show resolved
Hide resolved
tsfile/src/main/java/org/apache/iotdb/tsfile/read/expression/QueryExpression.java
Show resolved
Hide resolved
added 2 commits
December 27, 2019 11:12
…to remove_mmanager_for_query � Conflicts: � server/src/main/java/org/apache/iotdb/db/service/TSServiceImpl.java
qiaojialin
requested changes
Dec 27, 2019
Member
qiaojialin
left a comment
qiaojialin
left a comment
There was a problem hiding this comment.
Good work! just some optimizations
| import org.apache.hadoop.mapreduce.TaskAttemptContext; | ||
| import org.apache.iotdb.hadoop.fileSystem.HDFSInput; | ||
| import org.apache.iotdb.tsfile.common.constant.TsFileConstant; | ||
| import org.apache.iotdb.tsfile.file.metadata.enums.TSDataType; |
| deduplicatedAggregations); | ||
| queryDataSet = aggregate(deduplicatedPaths, deduplicatedAggregations, | ||
| queryPlan.getExpression(), | ||
| return groupBy(groupByPlan.getDeduplicatedPaths(), groupByPlan.getDeduplicatedDataTypes(), |
Member
There was a problem hiding this comment.
I prefer to replace the parameter to a GroupByPlan
| groupByPlan.getStartTime(), groupByPlan.getEndTime(), | ||
| context); | ||
| } else if (queryPlan instanceof AggregationPlan) { | ||
| queryDataSet = aggregate(queryPlan.getDeduplicatedPaths(), |
Member
There was a problem hiding this comment.
also, change the parameter of methods in IQueryProcesExecutor to a certain Plan
|
|
||
| private List<Path> deduplicatedPaths = null; | ||
| private List<TSDataType> deduplicatedDataTypes = null; | ||
| private List<String> deduplicatedAggregations = new ArrayList<>(); |
Member
There was a problem hiding this comment.
move this to AggregationPlan
| } else { | ||
| deduplicate(queryPlan); | ||
| } | ||
| if (queryPlan.getDeduplicatedPaths() == null) { |
Member
There was a problem hiding this comment.
The following 3 if could be avoided
| List<String> aggregations = queryPlan.getAggregations(); | ||
|
|
||
| Map<Path, TSDataType> dataTypeMapping = queryPlan.getDataTypeMapping(); | ||
| if (paths == null) { |
Member
There was a problem hiding this comment.
This could also be removed.
If the user selects some non-existing paths, the ConcatPathOptimizer will throw an exception.
| private void deduplicate(QueryPlan queryPlan) | ||
| throws QueryProcessException { | ||
| List<Path> paths = queryPlan.getPaths(); | ||
| if (paths == null) { |
| private List<Path> deduplicatedPaths = null; | ||
| private List<TSDataType> deduplicatedDataTypes = null; | ||
| private List<String> deduplicatedAggregations = new ArrayList<>(); | ||
| private Map<Path, TSDataType> dataTypeMapping = new HashMap<>(); |
Member
There was a problem hiding this comment.
This field may be useless, you can use deduplicatedPaths and deduplicatedDataTypes.
| public void initGroupBy(QueryContext context, List<String> aggres, IExpression expression) | ||
| throws StorageEngineException, QueryProcessException, IOException { | ||
| initAggreFuction(aggres); | ||
| public void initGroupBy(QueryContext context, List<String> aggres, List<TSDataType> dataTypes, |
Member
There was a problem hiding this comment.
change this to a private method that called in GroupByWithValueFilterDataSet constructor
| * init reader and aggregate function. | ||
| */ | ||
| public void initGroupBy(QueryContext context, List<String> aggres, IExpression expression) | ||
| public void initGroupBy(QueryContext context, List<String> aggres, List<TSDataType> dataTypes, |
Member
There was a problem hiding this comment.
Change this to a private method and called in constructor. More, the constructor could receive a GroupByPlan as a parameter, which is clearer
Member
|
Could the deduplicated codes be removed in IoTDBQueryResultSet? Just use the deduplicated paths passed by server |
qiaojialin
approved these changes
Dec 27, 2019
added 3 commits
December 28, 2019 00:27
jt2594838
approved these changes
Dec 28, 2019
qiaojialin
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
In a QueryPlan, the selected paths with data types, predicates (expression) are included.
private List paths;
private List dataTypes;
private IExpression expression;
However, when executing a QueryPlan in the AbstractQueryProcessExecutor, only paths and expression are passed to the EngineQueryRouter through QueryExpression.
Therefore, the data type information is lost and needs to get from manager again, which is a waste.
It's better to pass the full QueryPlan to the EngineQueryRouter for all query types: raw data query, fill query, group by query, aggregation query.
The QueryExpression is a parameter in the query interface of TsFile, we do not need to use it in the server, because we could get more information on the server.