Kikyou1997 commented on code in PR #17966:
URL: https://github.com/apache/doris/pull/17966#discussion_r1144194043
##########
fe/fe-core/src/main/java/org/apache/doris/analysis/AnalyzeStmt.java:
##########
@@ -166,46 +150,81 @@ private void checkAnalyzePriv(String dbName, String
tblName) throws AnalysisExce
}
private void checkPartitionNames() throws AnalysisException {
- if (optPartitionNames != null) {
- optPartitionNames.analyze(analyzer);
- if (tableName != null) {
- Database db =
analyzer.getEnv().getInternalCatalog().getDbOrAnalysisException(tableName.getDb());
- OlapTable olapTable = (OlapTable)
db.getTableOrAnalysisException(tableName.getTbl());
- if (!olapTable.isPartitioned()) {
- throw new AnalysisException("Not a partitioned table: " +
olapTable.getName());
- }
- List<String> names = optPartitionNames.getPartitionNames();
- Set<String> olapPartitionNames = olapTable.getPartitionNames();
- List<String> tempPartitionNames =
olapTable.getTempPartitions().stream()
- .map(Partition::getName).collect(Collectors.toList());
- Optional<String> optional = names.stream()
- .filter(name -> (tempPartitionNames.contains(name)
- || !olapPartitionNames.contains(name)))
- .findFirst();
- if (optional.isPresent()) {
- throw new AnalysisException("Temporary partition or
partition does not exist");
- }
- } else {
- throw new AnalysisException("Specify partition should specify
table name as well");
+ if (partitionNames != null) {
+ partitionNames.analyze(analyzer);
+ Database db = analyzer.getEnv().getInternalCatalog()
+ .getDbOrAnalysisException(tableName.getDb());
+ OlapTable olapTable = (OlapTable)
db.getTableOrAnalysisException(tableName.getTbl());
+ if (!olapTable.isPartitioned()) {
+ throw new AnalysisException("Not a partitioned table: " +
olapTable.getName());
+ }
+ List<String> names = partitionNames.getPartitionNames();
+ Set<String> olapPartitionNames = olapTable.getPartitionNames();
+ List<String> tempPartitionNames =
olapTable.getTempPartitions().stream()
+ .map(Partition::getName).collect(Collectors.toList());
+ Optional<String> optional = names.stream()
+ .filter(name -> (tempPartitionNames.contains(name)
+ || !olapPartitionNames.contains(name)))
+ .findFirst();
+ if (optional.isPresent()) {
+ throw new AnalysisException("Temporary partition or partition
does not exist");
Review Comment:
The exception message seems not match the computation logic of `optional`
variable
##########
fe/fe-core/src/main/java/org/apache/doris/statistics/HistogramTask.java:
##########
@@ -48,7 +49,16 @@ public class HistogramTask extends BaseAnalysisTask {
+ " HISTOGRAM(`${colName}`, 1, ${maxBucketNum}) AS buckets, "
+ " NOW() AS create_time "
+ "FROM "
- + " `${dbName}`.`${tblName}` TABLESAMPLE (${percentValue}
PERCENT)";
+ + " `${dbName}`.`${tblName}`";
+
+ /** To avoid too much data, use the following efficient sampling method */
+ private static final String ANALYZE_HISTOGRAM_SQL_TEMPLATE_PART =
ANALYZE_HISTOGRAM_SQL_TEMPLATE
+ + " PARTITION (${partName})"
+ + " TABLESAMPLE (${percentValue} PERCENT)";
+
+ /** To avoid too much data, use the following efficient sampling method */
+ private static final String ANALYZE_HISTOGRAM_SQL_TEMPLATE_FULL =
ANALYZE_HISTOGRAM_SQL_TEMPLATE
Review Comment:
Why full collection also based on table sample function
##########
fe/fe-core/src/main/java/org/apache/doris/statistics/AnalysisTaskInfo.java:
##########
@@ -62,6 +63,8 @@ public enum ScheduleType {
public final String colName;
+ public final List<String> partitionNames;
Review Comment:
Better use `Set` type to deduplicate
##########
fe/fe-core/src/main/java/org/apache/doris/statistics/MVAnalysisTask.java:
##########
@@ -111,7 +110,7 @@ public void execute() throws Exception {
params.put("idxId", String.valueOf(meta.getIndexId()));
String colName = column.getName();
params.put("colId", colName);
- long partId = part.getId();
+ long partId = olapTable.getPartition(partName).getId();
Review Comment:
olapTable.getPartition(partName) might return null
##########
fe/fe-core/src/main/java/org/apache/doris/analysis/AnalyzeStmt.java:
##########
@@ -63,48 +61,36 @@ public class AnalyzeStmt extends DdlStmt {
// time to wait for collect statistics
public static final String CBO_STATISTICS_TASK_TIMEOUT_SEC =
"cbo_statistics_task_timeout_sec";
- public boolean isHistogram = false;
-
private static final ImmutableSet<String> PROPERTIES_SET = new
ImmutableSet.Builder<String>()
.add(CBO_STATISTICS_TASK_TIMEOUT_SEC)
.build();
private static final Predicate<Long> DESIRED_TASK_TIMEOUT_SEC = (v) -> v >
0L;
- public final boolean wholeTbl;
+ public boolean isWholeTbl;
Review Comment:
Is this variable really necessary?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
