You are viewing a plain text version of this content. The canonical link for it is here.
Posted to gitbox@hive.apache.org by "tanishq-chugh (via GitHub)" <gi...@apache.org> on 2026/04/18 13:13:21 UTC
[PR] HIVE-29551: Avoid quadratic runtime in ColumnStatsSemanticAnalyzer#ge… [hive]
tanishq-chugh opened a new pull request, #6443:
URL: https://github.com/apache/hive/pull/6443
…tColumnTypes
<!--
Thanks for sending a pull request! Here are some tips for you:
1. If this is your first time, please read our contributor guidelines: https://cwiki.apache.org/confluence/display/Hive/HowToContribute
2. Ensure that you have created an issue on the Hive project JIRA: https://issues.apache.org/jira/projects/HIVE/summary
3. Ensure you have added or run the appropriate tests for your PR:
4. If the PR is unfinished, add '[WIP]' in your PR title, e.g., '[WIP]HIVE-XXXXX: Your PR title ...'.
5. Be sure to keep the PR description updated to reflect all changes.
6. Please write your PR title to summarize what this PR proposes.
7. If possible, provide a concise example to reproduce the issue for a faster review.
-->
### What changes were proposed in this pull request?
<!--
Please clarify what changes you are proposing. The purpose of this section is to outline the changes and how this PR fixes the issue.
If possible, please consider writing useful notes for better and faster reviews in your PR. See the examples below.
1. If you refactor some codes with changing classes, showing the class hierarchy will help reviewers.
2. If you fix some SQL features, you can provide some references of other DBMSes.
3. If there is design documentation, please add the link.
4. If there is a discussion in the mailing list, please add the link.
-->
Improve time complexity in ColumnStatsSemanticAnalyzer#getColumnTypes
### Why are the changes needed?
<!--
Please clarify why the changes are needed. For instance,
1. If you propose a new API, clarify the use case for a new API.
2. If you fix a bug, you can clarify why it is a bug.
-->
Performance Improvement
### Does this PR introduce _any_ user-facing change?
<!--
Note that it means *any* user-facing change including all aspects such as the documentation fix.
If yes, please clarify the previous behavior and the change this PR proposes - provide the console output, description, screenshot and/or a reproducable example to show the behavior difference if possible.
If possible, please also clarify if this is a user-facing change compared to the released Hive versions or within the unreleased branches such as master.
If no, write 'No'.
-->
No
### How was this patch tested?
<!--
If tests were added, say they were added here. Please make sure to add some test cases that check the changes thoroughly including negative and positive cases if possible.
If it was tested in a way different from regular unit tests, please clarify how you tested step by step, ideally copy and paste-able, so that other reviewers can test and check, and descendants can verify in the future.
If tests were not added, please describe why they were not added and/or why it was difficult to add.
-->
Manual Testing + CI
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-29551: Avoid quadratic runtime in ColumnStatsSemanticAnalyzer#ge… [hive]
Posted by "abstractdog (via GitHub)" <gi...@apache.org>.
abstractdog commented on code in PR #6443:
URL: https://github.com/apache/hive/pull/6443#discussion_r3217975612
##########
ql/src/java/org/apache/hadoop/hive/ql/parse/ColumnStatsSemanticAnalyzer.java:
##########
@@ -749,10 +761,9 @@ static AnalyzeRewriteContext genAnalyzeRewriteContext(HiveConf conf, Table tbl)
AnalyzeRewriteContext analyzeRewrite = new AnalyzeRewriteContext();
analyzeRewrite.setTableName(tbl.getFullyQualifiedName());
analyzeRewrite.setTblLvl(!(conf.getBoolVar(ConfVars.HIVE_STATS_COLLECT_PART_LEVEL_STATS) && tbl.isPartitioned()));
- List<String> colNames = getColumnNamesSupportingStats(tbl);
- List<String> colTypes = getColumnTypes(tbl, colNames);
- analyzeRewrite.setColName(colNames);
- analyzeRewrite.setColType(colTypes);
+ List<FieldSchema> columnSchemas = getStatsEligibleFieldSchemas(tbl);
+ analyzeRewrite.setColName(Utilities.getColumnNamesFromFieldSchema(columnSchemas));
+ analyzeRewrite.setColType(Utilities.getColumnTypesFromFieldSchema(columnSchemas));
Review Comment:
What about refactoring `analyzeRewrite` too, to simply accept `columnSchemas`?
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-29551: Avoid quadratic runtime in ColumnStatsSemanticAnalyzer#ge… [hive]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #6443:
URL: https://github.com/apache/hive/pull/6443#issuecomment-4432071530
## [](https://sonarcloud.io/dashboard?id=apache_hive&pullRequest=6443) **Quality Gate passed**
Issues
 [2 New issues](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=6443&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=6443&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_hive&pullRequest=6443&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_hive&pullRequest=6443&metric=new_coverage&view=list)
 [0.3% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_hive&pullRequest=6443&metric=new_duplicated_lines_density&view=list)
<!-- sqra-placement-anchor -->
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_hive&pullRequest=6443)
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-29551: Avoid quadratic runtime in ColumnStatsSemanticAnalyzer#ge… [hive]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #6443:
URL: https://github.com/apache/hive/pull/6443#issuecomment-4340167907
## [](https://sonarcloud.io/dashboard?id=apache_hive&pullRequest=6443) **Quality Gate passed**
Issues
 [0 New issues](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=6443&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=6443&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_hive&pullRequest=6443&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_hive&pullRequest=6443&metric=new_coverage&view=list)
 [0.3% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_hive&pullRequest=6443&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_hive&pullRequest=6443)
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-29551: Avoid quadratic runtime in ColumnStatsSemanticAnalyzer#ge… [hive]
Posted by "tanishq-chugh (via GitHub)" <gi...@apache.org>.
tanishq-chugh commented on code in PR #6443:
URL: https://github.com/apache/hive/pull/6443#discussion_r3219185057
##########
ql/src/java/org/apache/hadoop/hive/ql/parse/ColumnStatsSemanticAnalyzer.java:
##########
@@ -736,6 +729,25 @@ public ASTNode rewriteAST(ASTNode ast, ColumnStatsAutoGatherContext context)
return rewrittenTree;
}
+ protected List<FieldSchema> getColumnsFromAst(ASTNode ast) throws SemanticException {
+ List<FieldSchema> statsEligibleFS = null;
+ List<String> columnNames;
+ if (ast.getChildCount() == 2) {
+ statsEligibleFS = getStatsEligibleFieldSchemas(tbl);
+ columnNames = Utilities.getColumnNamesFromFieldSchema(statsEligibleFS);
+ } else{
+ columnNames = getExplicitColumnNamesFromAst(ast);
+ }
+
+ checkForPartitionColumns(columnNames, Utilities.getColumnNamesFromFieldSchema(tbl.getPartitionKeys()));
+ validateSpecifiedColumnNames(columnNames);
+
+ if (statsEligibleFS != null) {
+ return statsEligibleFS;
+ }
+ return getFieldSchemasByColName(tbl, columnNames);
Review Comment:
Made this change in commit: [4ca13d8](https://github.com/apache/hive/pull/6443/commits/4ca13d86634349416f0ae7d7479145d5356c0ccb)
##########
ql/src/java/org/apache/hadoop/hive/ql/parse/ColumnStatsSemanticAnalyzer.java:
##########
@@ -736,6 +729,25 @@ public ASTNode rewriteAST(ASTNode ast, ColumnStatsAutoGatherContext context)
return rewrittenTree;
}
+ protected List<FieldSchema> getColumnsFromAst(ASTNode ast) throws SemanticException {
+ List<FieldSchema> statsEligibleFS = null;
+ List<String> columnNames;
+ if (ast.getChildCount() == 2) {
+ statsEligibleFS = getStatsEligibleFieldSchemas(tbl);
+ columnNames = Utilities.getColumnNamesFromFieldSchema(statsEligibleFS);
+ } else{
Review Comment:
Fixed in commit: [4ca13d8](https://github.com/apache/hive/pull/6443/commits/4ca13d86634349416f0ae7d7479145d5356c0ccb)
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-29551: Avoid quadratic runtime in ColumnStatsSemanticAnalyzer#ge… [hive]
Posted by "abstractdog (via GitHub)" <gi...@apache.org>.
abstractdog commented on code in PR #6443:
URL: https://github.com/apache/hive/pull/6443#discussion_r3219776096
##########
ql/src/java/org/apache/hadoop/hive/ql/parse/BaseSemanticAnalyzer.java:
##########
@@ -1428,12 +1427,16 @@ public void setTableName(String tableName) {
this.tableName = tableName;
}
- public List<String> getColName() {
- return colName;
+ public List<FieldSchema> getColumnSchemas() {
+ return columnSchemas;
+ }
+
+ public void setColumnSchemas(List<FieldSchema> columnSchemas) {
+ this.columnSchemas = columnSchemas;
}
- public void setColName(List<String> colName) {
- this.colName = colName;
+ public List<String> getColName() {
+ return columnSchemas == null ? null : Utilities.getColumnNamesFromFieldSchema(columnSchemas);
Review Comment:
if by static code analysis we can be sure that `getColName` is not called e.g. 100 times during compilation, this is going to be fine, I'm inclined to accept your analysis here
my point was only to completely remove the separate `colName` and `colType` variables from method signatures where it's possible, and only have them as separate variables where it's really needed (e.g. where EXPLAIN plan is used), e.g.:
```
@Explain(displayName = "Columns")
public List<String> getColName() {
return colName;
}
```
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-29551: Avoid quadratic runtime in ColumnStatsSemanticAnalyzer#ge… [hive]
Posted by "Aggarwal-Raghav (via GitHub)" <gi...@apache.org>.
Aggarwal-Raghav commented on code in PR #6443:
URL: https://github.com/apache/hive/pull/6443#discussion_r3224921882
##########
ql/src/java/org/apache/hadoop/hive/ql/parse/ColumnStatsSemanticAnalyzer.java:
##########
@@ -726,33 +717,46 @@ public ASTNode rewriteAST(ASTNode ast, ColumnStatsAutoGatherContext context)
partTransformSpec = tbl.getStorageHandler().getPartitionTransformSpec(tbl);
}
}
- colType = getColumnTypes(tbl, colNames);
+ rewrittenColumnSchemas = new FieldSchemas(columnSchemas);
isTableLevel = !isPartitionStats;
- rewrittenQuery = genRewrittenQuery(colNames, colType, conf, partTransformSpec, -1,
+ rewrittenQuery = genRewrittenQuery(rewrittenColumnSchemas, conf, partTransformSpec, -1,
partSpec, isPartitionStats);
rewrittenTree = genRewrittenTree(rewrittenQuery);
return rewrittenTree;
}
+ protected List<FieldSchema> getColumnsFromAst(ASTNode ast) throws SemanticException {
+ List<FieldSchema> statsEligibleFS = null;
+ List<String> columnNames;
+ if (ast.getChildCount() == 2) {
+ FieldSchemas eligibleFS = getStatsEligibleFieldSchemas(tbl);
+ statsEligibleFS = eligibleFS.getSchemas();
+ columnNames = eligibleFS.getColName();
+ } else {
+ columnNames = getExplicitColumnNamesFromAst(ast);
+ }
+
+ checkForPartitionColumns(columnNames, Utilities.getColumnNamesFromFieldSchema(tbl.getPartitionKeys()));
Review Comment:
@tanishq-chugh , can you please check `checkForPartitionColumns` it also has nested for loop and `validateSpecifiedColumnNames` is using for `loop + contains()` which is also O(N*M). Both are used in `getColumnsFromAst()`
I haven't gone though the full PR yet, just wanted to hightlight.
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-29551: Avoid quadratic runtime in ColumnStatsSemanticAnalyzer#ge… [hive]
Posted by "tanishq-chugh (via GitHub)" <gi...@apache.org>.
tanishq-chugh commented on code in PR #6443:
URL: https://github.com/apache/hive/pull/6443#discussion_r3159119586
##########
ql/src/java/org/apache/hadoop/hive/ql/parse/ColumnStatsSemanticAnalyzer.java:
##########
@@ -102,38 +103,38 @@ private boolean shouldRewrite(ASTNode tree) {
return rwt;
}
+ private record StatsEligibleColumns(List<String> columnNames, List<String> columnTypes) {
+ }
+
/**
- * Get the names of the columns that support column statistics.
+ * Get the names and types of the columns that support column statistics.
*/
- private static List<String> getColumnNamesSupportingStats(Table tbl) {
+ private static StatsEligibleColumns getStatsEligibleColumns(Table tbl) {
List<String> colNames = new ArrayList<>();
+ List<String> colTypes = new ArrayList<>();
for (FieldSchema col : tbl.getCols()) {
String type = col.getType();
TypeInfo typeInfo = TypeInfoUtils.getTypeInfoFromTypeString(type);
boolean isSupported = ColumnStatsAutoGatherContext.isColumnSupported(typeInfo.getCategory(), () -> typeInfo);
if (isSupported) {
colNames.add(col.getName());
+ colTypes.add(col.getType());
}
}
- return colNames;
+ return new StatsEligibleColumns(colNames, colTypes);
}
private List<String> getColumnName(ASTNode tree) throws SemanticException {
-
- switch (tree.getChildCount()) {
- case 2:
- return getColumnNamesSupportingStats(tbl);
- case 3:
- int numCols = tree.getChild(2).getChildCount();
- List<String> colName = new ArrayList<>(numCols);
- for (int i = 0; i < numCols; i++) {
- colName.add(getUnescapedName((ASTNode) tree.getChild(2).getChild(i)));
- }
- return colName;
- default:
+ if (tree.getChildCount() != 3) {
throw new SemanticException("Internal error. Expected number of children of ASTNode to be"
+ " either 2 or 3. Found : " + tree.getChildCount());
Review Comment:
Updated the exception message in commit - [84d81f9](https://github.com/apache/hive/pull/6443/commits/84d81f933cf7ecd8112293895c3d3997cc7292f1)
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-29551: Avoid quadratic runtime in ColumnStatsSemanticAnalyzer#ge… [hive]
Posted by "tanishq-chugh (via GitHub)" <gi...@apache.org>.
tanishq-chugh commented on code in PR #6443:
URL: https://github.com/apache/hive/pull/6443#discussion_r3106447794
##########
ql/src/java/org/apache/hadoop/hive/ql/parse/ColumnStatsSemanticAnalyzer.java:
##########
@@ -205,23 +206,28 @@ private static String getColTypeOf(Table tbl, String partKey) {
protected static List<String> getColumnTypes(Table tbl, List<String> colNames) {
List<String> colTypes = new ArrayList<>();
List<FieldSchema> cols = tbl.getCols();
- List<String> copyColNames = new ArrayList<>(colNames);
-
- for (String colName : copyColNames) {
- for (FieldSchema col : cols) {
- if (colName.equalsIgnoreCase(col.getName())) {
- String type = col.getType();
- TypeInfo typeInfo = TypeInfoUtils.getTypeInfoFromTypeString(type);
- if (typeInfo.getCategory() != ObjectInspector.Category.PRIMITIVE) {
- logTypeWarning(colName, type);
- colNames.remove(colName);
- } else {
- colTypes.add(type);
- }
+ Map<String, String> colTypeMap = new HashMap<>();
+
+ for (FieldSchema col : cols) {
+ colTypeMap.put(col.getName().toLowerCase(), col.getType());
+ }
+
+ List<String> nonPrimColNames = new ArrayList<>();
+ for (String colName : colNames) {
+ String type = colTypeMap.get(colName.toLowerCase());
+ if (type != null) {
+ TypeInfo typeInfo = TypeInfoUtils.getTypeInfoFromTypeString(type);
+ if (typeInfo.getCategory() != ObjectInspector.Category.PRIMITIVE) {
+ logTypeWarning(colName, type);
+ } else {
+ nonPrimColNames.add(colName);
Review Comment:
@Aggarwal-Raghav My bad, i validated the columnTypes/names being returned for primitive types and used the wrong variable name. Updated in commit - [4a6804d](https://github.com/apache/hive/pull/6443/commits/4a6804d75a2d0bf084dd87b1315f657be7ce3d2a) .
Thanks for pointing this out !
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-29551: Avoid quadratic runtime in ColumnStatsSemanticAnalyzer#ge… [hive]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #6443:
URL: https://github.com/apache/hive/pull/6443#issuecomment-4275540666
## [](https://sonarcloud.io/dashboard?id=apache_hive&pullRequest=6443) **Quality Gate passed**
Issues
 [0 New issues](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=6443&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=6443&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_hive&pullRequest=6443&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_hive&pullRequest=6443&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_hive&pullRequest=6443&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_hive&pullRequest=6443)
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-29551: Avoid quadratic runtime in ColumnStatsSemanticAnalyzer#ge… [hive]
Posted by "abstractdog (via GitHub)" <gi...@apache.org>.
abstractdog commented on code in PR #6443:
URL: https://github.com/apache/hive/pull/6443#discussion_r3219180102
##########
ql/src/java/org/apache/hadoop/hive/ql/parse/ColumnStatsSemanticAnalyzer.java:
##########
@@ -103,37 +104,32 @@ private boolean shouldRewrite(ASTNode tree) {
}
/**
- * Get the names of the columns that support column statistics.
+ * Get the Field Schemas of the columns that support column statistics.
*/
- private static List<String> getColumnNamesSupportingStats(Table tbl) {
- List<String> colNames = new ArrayList<>();
+ private static List<FieldSchema> getStatsEligibleFieldSchemas(Table tbl) {
+ List<FieldSchema> result = new ArrayList<>();
for (FieldSchema col : tbl.getCols()) {
String type = col.getType();
TypeInfo typeInfo = TypeInfoUtils.getTypeInfoFromTypeString(type);
boolean isSupported = ColumnStatsAutoGatherContext.isColumnSupported(typeInfo.getCategory(), () -> typeInfo);
if (isSupported) {
- colNames.add(col.getName());
+ result.add(col);
}
}
- return colNames;
+ return result;
}
- private List<String> getColumnName(ASTNode tree) throws SemanticException {
-
- switch (tree.getChildCount()) {
- case 2:
- return getColumnNamesSupportingStats(tbl);
- case 3:
- int numCols = tree.getChild(2).getChildCount();
- List<String> colName = new ArrayList<>(numCols);
- for (int i = 0; i < numCols; i++) {
- colName.add(getUnescapedName((ASTNode) tree.getChild(2).getChild(i)));
- }
- return colName;
- default:
- throw new SemanticException("Internal error. Expected number of children of ASTNode to be"
- + " either 2 or 3. Found : " + tree.getChildCount());
+ private List<String> getExplicitColumnNamesFromAst(ASTNode tree) throws SemanticException {
+ if (tree.getChildCount() != 3) {
Review Comment:
nice, thank you very much!
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-29551: Avoid quadratic runtime in ColumnStatsSemanticAnalyzer#ge… [hive]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #6443:
URL: https://github.com/apache/hive/pull/6443#issuecomment-4433876181
## [](https://sonarcloud.io/dashboard?id=apache_hive&pullRequest=6443) **Quality Gate passed**
Issues
 [2 New issues](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=6443&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=6443&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_hive&pullRequest=6443&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_hive&pullRequest=6443&metric=new_coverage&view=list)
 [0.3% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_hive&pullRequest=6443&metric=new_duplicated_lines_density&view=list)
<!-- sqra-placement-anchor -->
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_hive&pullRequest=6443)
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-29551: Avoid quadratic runtime in ColumnStatsSemanticAnalyzer#ge… [hive]
Posted by "Aggarwal-Raghav (via GitHub)" <gi...@apache.org>.
Aggarwal-Raghav commented on PR #6443:
URL: https://github.com/apache/hive/pull/6443#issuecomment-4475544175
> > Hi @Aggarwal-Raghav Thanks for pointing it out & yes, this is definitely better for optimisation. I have refactored the same in commit [37d1c22](https://github.com/apache/hive/pull/6443/commits/37d1c2267a120a76d37e214c5e968ee2d2ef859e)
> >
> > CC: @abstractdog
>
> commit looks good to me and looks like it's addressing the optimization idea, which is good, so my approval still holds: please let us know @Aggarwal-Raghav if you're okay with the patch now
Hey, please proceed. I've reviewed the Java specific changes, and the comments appear to have been addressed.
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-29551: Avoid quadratic runtime in ColumnStatsSemanticAnalyzer#ge… [hive]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #6443:
URL: https://github.com/apache/hive/pull/6443#issuecomment-4273856361
## [](https://sonarcloud.io/dashboard?id=apache_hive&pullRequest=6443) **Quality Gate passed**
Issues
 [0 New issues](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=6443&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=6443&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_hive&pullRequest=6443&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_hive&pullRequest=6443&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_hive&pullRequest=6443&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_hive&pullRequest=6443)
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-29551: Avoid quadratic runtime in ColumnStatsSemanticAnalyzer#ge… [hive]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #6443:
URL: https://github.com/apache/hive/pull/6443#issuecomment-4423936777
## [](https://sonarcloud.io/dashboard?id=apache_hive&pullRequest=6443) **Quality Gate passed**
Issues
 [3 New issues](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=6443&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=6443&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_hive&pullRequest=6443&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_hive&pullRequest=6443&metric=new_coverage&view=list)
 [0.3% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_hive&pullRequest=6443&metric=new_duplicated_lines_density&view=list)
<!-- sqra-placement-anchor -->
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_hive&pullRequest=6443)
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-29551: Avoid quadratic runtime in ColumnStatsSemanticAnalyzer#ge… [hive]
Posted by "abstractdog (via GitHub)" <gi...@apache.org>.
abstractdog commented on code in PR #6443:
URL: https://github.com/apache/hive/pull/6443#discussion_r3217929168
##########
ql/src/java/org/apache/hadoop/hive/ql/exec/Utilities.java:
##########
@@ -2286,6 +2286,14 @@ public static List<String> getColumnNamesFromFieldSchema(List<FieldSchema> partC
return names;
}
+ public static List<String> getColumnTypesFromFieldSchema(List<FieldSchema> fieldSchemas) {
+ List<String> types = new ArrayList<>();
+ for (FieldSchema fs : fieldSchemas) {
+ types.add(fs.getType());
+ }
+ return types;
Review Comment:
what about: `return fieldSchemas.stream().map(FieldSchema::getType).toList();`
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-29551: Avoid quadratic runtime in ColumnStatsSemanticAnalyzer#ge… [hive]
Posted by "tanishq-chugh (via GitHub)" <gi...@apache.org>.
tanishq-chugh commented on code in PR #6443:
URL: https://github.com/apache/hive/pull/6443#discussion_r3159129172
##########
ql/src/java/org/apache/hadoop/hive/ql/parse/ColumnStatsSemanticAnalyzer.java:
##########
@@ -102,38 +103,38 @@ private boolean shouldRewrite(ASTNode tree) {
return rwt;
}
+ private record StatsEligibleColumns(List<String> columnNames, List<String> columnTypes) {
+ }
+
/**
- * Get the names of the columns that support column statistics.
+ * Get the names and types of the columns that support column statistics.
*/
- private static List<String> getColumnNamesSupportingStats(Table tbl) {
+ private static StatsEligibleColumns getStatsEligibleColumns(Table tbl) {
List<String> colNames = new ArrayList<>();
+ List<String> colTypes = new ArrayList<>();
for (FieldSchema col : tbl.getCols()) {
String type = col.getType();
TypeInfo typeInfo = TypeInfoUtils.getTypeInfoFromTypeString(type);
boolean isSupported = ColumnStatsAutoGatherContext.isColumnSupported(typeInfo.getCategory(), () -> typeInfo);
if (isSupported) {
colNames.add(col.getName());
+ colTypes.add(col.getType());
}
}
- return colNames;
+ return new StatsEligibleColumns(colNames, colTypes);
}
private List<String> getColumnName(ASTNode tree) throws SemanticException {
Review Comment:
Renamed the function to `getExplicitColumnNamesFromAst` in commit - [84d81f9](https://github.com/apache/hive/pull/6443/commits/84d81f933cf7ecd8112293895c3d3997cc7292f1)
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-29551: Avoid quadratic runtime in ColumnStatsSemanticAnalyzer#ge… [hive]
Posted by "tanishq-chugh (via GitHub)" <gi...@apache.org>.
tanishq-chugh commented on code in PR #6443:
URL: https://github.com/apache/hive/pull/6443#discussion_r3223937507
##########
ql/src/java/org/apache/hadoop/hive/ql/parse/ColumnStatsSemanticAnalyzer.java:
##########
@@ -257,28 +255,27 @@ private String genRewrittenQuery(List<String> colNames, List<String> colTypes, H
protected static String genRewrittenQuery(Table tbl,
HiveConf conf, List<TransformSpec> partTransformSpec, Map<String, String> partSpec,
boolean isPartitionStats) {
- List<String> colNames = getColumnNamesSupportingStats(tbl);
- List<String> colTypes = ColumnStatsSemanticAnalyzer.getColumnTypes(tbl, colNames);
return ColumnStatsSemanticAnalyzer.genRewrittenQuery(
- tbl, colNames, colTypes, conf, partTransformSpec, -1, partSpec, isPartitionStats, true);
+ tbl, getStatsEligibleFieldSchemas(tbl).getSchemas(), conf, partTransformSpec, -1, partSpec, isPartitionStats, true);
}
- private static String genRewrittenQuery(Table tbl, List<String> colNames, List<String> colTypes,
+ private static String genRewrittenQuery(Table tbl, List<FieldSchema> columnSchemas,
Review Comment:
Nice catch, have made this change in commit: [c030988](https://github.com/apache/hive/pull/6443/commits/c030988b39fdef59d0bc2bbe92a4a0215c33e9a2)
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-29551: Avoid quadratic runtime in ColumnStatsSemanticAnalyzer#ge… [hive]
Posted by "tanishq-chugh (via GitHub)" <gi...@apache.org>.
tanishq-chugh commented on code in PR #6443:
URL: https://github.com/apache/hive/pull/6443#discussion_r3219707088
##########
ql/src/java/org/apache/hadoop/hive/ql/parse/BaseSemanticAnalyzer.java:
##########
@@ -1428,12 +1427,16 @@ public void setTableName(String tableName) {
this.tableName = tableName;
}
- public List<String> getColName() {
- return colName;
+ public List<FieldSchema> getColumnSchemas() {
+ return columnSchemas;
+ }
+
+ public void setColumnSchemas(List<FieldSchema> columnSchemas) {
+ this.columnSchemas = columnSchemas;
}
- public void setColName(List<String> colName) {
- this.colName = colName;
+ public List<String> getColName() {
+ return columnSchemas == null ? null : Utilities.getColumnNamesFromFieldSchema(columnSchemas);
Review Comment:
@abstractdog I see only one call of `AnalyzeRewriteContext#getColName()` from [TaskCompiler](https://github.com/apache/hive/blob/master/ql/src/java/org/apache/hadoop/hive/ql/parse/TaskCompiler.java#L675-L676) before initialising `ColumnStatsDesc`, and no other calls which would lead to multiple calls to `Utilities.getColumnNamesFromFieldSchema()`.
I am afraid i did not fully understand about how could this lead to issues and rather removing `getColName/getColType` and introducing a new constructor in `ColumnStatsDesc` using `columnSchemas` instead of `colName/colType` which internally calls same Utilities methods wouldn't be behaving the same? could you please elaborate on this? Thanks!
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-29551: Avoid quadratic runtime in ColumnStatsSemanticAnalyzer#ge… [hive]
Posted by "tanishq-chugh (via GitHub)" <gi...@apache.org>.
tanishq-chugh commented on code in PR #6443:
URL: https://github.com/apache/hive/pull/6443#discussion_r3220872569
##########
ql/src/java/org/apache/hadoop/hive/ql/parse/BaseSemanticAnalyzer.java:
##########
@@ -1428,12 +1427,16 @@ public void setTableName(String tableName) {
this.tableName = tableName;
}
- public List<String> getColName() {
- return colName;
+ public List<FieldSchema> getColumnSchemas() {
+ return columnSchemas;
+ }
+
+ public void setColumnSchemas(List<FieldSchema> columnSchemas) {
+ this.columnSchemas = columnSchemas;
}
- public void setColName(List<String> colName) {
- this.colName = colName;
+ public List<String> getColName() {
+ return columnSchemas == null ? null : Utilities.getColumnNamesFromFieldSchema(columnSchemas);
Review Comment:
Right, i have refactored the approach into a wrapper `FieldSchemas` as discussed in this commit: [9813bb5](https://github.com/apache/hive/pull/6443/commits/9813bb59d39181cb29f67032429a27f179a8e483)
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-29551: Avoid quadratic runtime in ColumnStatsSemanticAnalyzer#ge… [hive]
Posted by "tanishq-chugh (via GitHub)" <gi...@apache.org>.
tanishq-chugh commented on code in PR #6443:
URL: https://github.com/apache/hive/pull/6443#discussion_r3159116400
##########
ql/src/java/org/apache/hadoop/hive/ql/parse/ColumnStatsSemanticAnalyzer.java:
##########
@@ -217,27 +218,32 @@ private static String getColTypeOf(Table tbl, String partKey) {
throw new RuntimeException("Unknown partition key : " + partKey);
}
- protected static List<String> getColumnTypes(Table tbl, List<String> colNames) {
+ protected static List<String> getColumnTypesByName(Table tbl, List<String> colNames) {
List<String> colTypes = new ArrayList<>();
List<FieldSchema> cols = tbl.getCols();
- List<String> copyColNames = new ArrayList<>(colNames);
-
- for (String colName : copyColNames) {
- for (FieldSchema col : cols) {
- if (colName.equalsIgnoreCase(col.getName())) {
- String type = col.getType();
- TypeInfo typeInfo = TypeInfoUtils.getTypeInfoFromTypeString(type);
- boolean isSupported = ColumnStatsAutoGatherContext.isColumnSupported(typeInfo.getCategory(), () -> typeInfo);
- if (!isSupported) {
- logTypeWarning(colName, type);
- colNames.remove(colName);
- } else {
- colTypes.add(type);
- }
+ Map<String, String> colTypeMap = new HashMap<>();
+
+ for (FieldSchema col : cols) {
+ colTypeMap.put(col.getName().toLowerCase(), col.getType());
+ }
+
+ List<String> primColNames = new ArrayList<>();
+ for (String colName : colNames) {
+ String type = colTypeMap.get(colName.toLowerCase());
+ if (type != null) {
+ TypeInfo typeInfo = TypeInfoUtils.getTypeInfoFromTypeString(type);
+ boolean isSupported = ColumnStatsAutoGatherContext.isColumnSupported(typeInfo.getCategory(), () -> typeInfo);
+ if (!isSupported) {
+ logTypeWarning(colName, type);
+ } else {
+ primColNames.add(colName);
+ colTypes.add(type);
}
}
}
+ colNames.clear();
+ colNames.addAll(primColNames);
Review Comment:
Yes, the code has been updated such that modifying this argument is avoided, in commit - [84d81f9](https://github.com/apache/hive/pull/6443/commits/84d81f933cf7ecd8112293895c3d3997cc7292f1)
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-29551: Avoid quadratic runtime in ColumnStatsSemanticAnalyzer#ge… [hive]
Posted by "abstractdog (via GitHub)" <gi...@apache.org>.
abstractdog commented on code in PR #6443:
URL: https://github.com/apache/hive/pull/6443#discussion_r3217954112
##########
ql/src/java/org/apache/hadoop/hive/ql/parse/ColumnStatsSemanticAnalyzer.java:
##########
@@ -736,6 +729,25 @@ public ASTNode rewriteAST(ASTNode ast, ColumnStatsAutoGatherContext context)
return rewrittenTree;
}
+ protected List<FieldSchema> getColumnsFromAst(ASTNode ast) throws SemanticException {
+ List<FieldSchema> statsEligibleFS = null;
+ List<String> columnNames;
+ if (ast.getChildCount() == 2) {
+ statsEligibleFS = getStatsEligibleFieldSchemas(tbl);
+ columnNames = Utilities.getColumnNamesFromFieldSchema(statsEligibleFS);
+ } else{
Review Comment:
formatting: `} else {`
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-29551: Avoid quadratic runtime in ColumnStatsSemanticAnalyzer#ge… [hive]
Posted by "tanishq-chugh (via GitHub)" <gi...@apache.org>.
tanishq-chugh commented on code in PR #6443:
URL: https://github.com/apache/hive/pull/6443#discussion_r3219174000
##########
ql/src/java/org/apache/hadoop/hive/ql/parse/ColumnStatsSemanticAnalyzer.java:
##########
@@ -103,37 +104,32 @@ private boolean shouldRewrite(ASTNode tree) {
}
/**
- * Get the names of the columns that support column statistics.
+ * Get the Field Schemas of the columns that support column statistics.
*/
- private static List<String> getColumnNamesSupportingStats(Table tbl) {
- List<String> colNames = new ArrayList<>();
+ private static List<FieldSchema> getStatsEligibleFieldSchemas(Table tbl) {
+ List<FieldSchema> result = new ArrayList<>();
for (FieldSchema col : tbl.getCols()) {
String type = col.getType();
TypeInfo typeInfo = TypeInfoUtils.getTypeInfoFromTypeString(type);
boolean isSupported = ColumnStatsAutoGatherContext.isColumnSupported(typeInfo.getCategory(), () -> typeInfo);
if (isSupported) {
- colNames.add(col.getName());
+ result.add(col);
}
}
- return colNames;
+ return result;
}
- private List<String> getColumnName(ASTNode tree) throws SemanticException {
-
- switch (tree.getChildCount()) {
- case 2:
- return getColumnNamesSupportingStats(tbl);
- case 3:
- int numCols = tree.getChild(2).getChildCount();
- List<String> colName = new ArrayList<>(numCols);
- for (int i = 0; i < numCols; i++) {
- colName.add(getUnescapedName((ASTNode) tree.getChild(2).getChild(i)));
- }
- return colName;
- default:
- throw new SemanticException("Internal error. Expected number of children of ASTNode to be"
- + " either 2 or 3. Found : " + tree.getChildCount());
+ private List<String> getExplicitColumnNamesFromAst(ASTNode tree) throws SemanticException {
+ if (tree.getChildCount() != 3) {
Review Comment:
Added a comment to explain the behaviour in commit : [2c7c593](https://github.com/apache/hive/pull/6443/commits/2c7c593fa9f83c135312ef24f0e2b1018a9da0da)
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-29551: Avoid quadratic runtime in ColumnStatsSemanticAnalyzer#ge… [hive]
Posted by "tanishq-chugh (via GitHub)" <gi...@apache.org>.
tanishq-chugh commented on code in PR #6443:
URL: https://github.com/apache/hive/pull/6443#discussion_r3219178111
##########
ql/src/java/org/apache/hadoop/hive/ql/parse/ColumnStatsSemanticAnalyzer.java:
##########
@@ -257,10 +252,11 @@ private String genRewrittenQuery(List<String> colNames, List<String> colTypes, H
protected static String genRewrittenQuery(Table tbl,
HiveConf conf, List<TransformSpec> partTransformSpec, Map<String, String> partSpec,
boolean isPartitionStats) {
- List<String> colNames = getColumnNamesSupportingStats(tbl);
- List<String> colTypes = ColumnStatsSemanticAnalyzer.getColumnTypes(tbl, colNames);
+ List<FieldSchema> columnSchemas = getStatsEligibleFieldSchemas(tbl);
return ColumnStatsSemanticAnalyzer.genRewrittenQuery(
- tbl, colNames, colTypes, conf, partTransformSpec, -1, partSpec, isPartitionStats, true);
+ tbl, Utilities.getColumnNamesFromFieldSchema(columnSchemas),
+ Utilities.getColumnTypesFromFieldSchema(columnSchemas), conf, partTransformSpec, -1, partSpec,
Review Comment:
Nice catch, that makes more sense. Updated in commit : [a269464](https://github.com/apache/hive/pull/6443/commits/a269464e8e49570a18125abddfc12c3102f933ca)
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-29551: Avoid quadratic runtime in ColumnStatsSemanticAnalyzer#ge… [hive]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #6443:
URL: https://github.com/apache/hive/pull/6443#issuecomment-4456238413
## [](https://sonarcloud.io/dashboard?id=apache_hive&pullRequest=6443) **Quality Gate passed**
Issues
 [2 New issues](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=6443&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=6443&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_hive&pullRequest=6443&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_hive&pullRequest=6443&metric=new_coverage&view=list)
 [0.3% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_hive&pullRequest=6443&metric=new_duplicated_lines_density&view=list)
<!-- sqra-placement-anchor -->
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_hive&pullRequest=6443)
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-29551: Avoid quadratic runtime in ColumnStatsSemanticAnalyzer#ge… [hive]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #6443:
URL: https://github.com/apache/hive/pull/6443#issuecomment-4329910768
## [](https://sonarcloud.io/dashboard?id=apache_hive&pullRequest=6443) **Quality Gate passed**
Issues
 [0 New issues](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=6443&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=6443&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_hive&pullRequest=6443&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_hive&pullRequest=6443&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_hive&pullRequest=6443&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_hive&pullRequest=6443)
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-29551: Avoid quadratic runtime in ColumnStatsSemanticAnalyzer#ge… [hive]
Posted by "tanishq-chugh (via GitHub)" <gi...@apache.org>.
tanishq-chugh commented on code in PR #6443:
URL: https://github.com/apache/hive/pull/6443#discussion_r3159117722
##########
ql/src/java/org/apache/hadoop/hive/ql/parse/ColumnStatsSemanticAnalyzer.java:
##########
@@ -217,27 +218,32 @@ private static String getColTypeOf(Table tbl, String partKey) {
throw new RuntimeException("Unknown partition key : " + partKey);
}
- protected static List<String> getColumnTypes(Table tbl, List<String> colNames) {
+ protected static List<String> getColumnTypesByName(Table tbl, List<String> colNames) {
Review Comment:
Made this change in commit - [84d81f9](https://github.com/apache/hive/pull/6443/commits/84d81f933cf7ecd8112293895c3d3997cc7292f1)
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-29551: Avoid quadratic runtime in ColumnStatsSemanticAnalyzer#ge… [hive]
Posted by "abstractdog (via GitHub)" <gi...@apache.org>.
abstractdog commented on PR #6443:
URL: https://github.com/apache/hive/pull/6443#issuecomment-4447952281
double-checking here: I’d like to know whether anyone has any further concerns or comments, my +1 still stands, I’ll wait another 24 hours unless we receive confirmation sooner
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-29551: Avoid quadratic runtime in ColumnStatsSemanticAnalyzer#ge… [hive]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #6443:
URL: https://github.com/apache/hive/pull/6443#issuecomment-4322985566
## [](https://sonarcloud.io/dashboard?id=apache_hive&pullRequest=6443) **Quality Gate passed**
Issues
 [2 New issues](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=6443&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=6443&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_hive&pullRequest=6443&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_hive&pullRequest=6443&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_hive&pullRequest=6443&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_hive&pullRequest=6443)
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-29551: Avoid quadratic runtime in ColumnStatsSemanticAnalyzer#ge… [hive]
Posted by "abstractdog (via GitHub)" <gi...@apache.org>.
abstractdog commented on code in PR #6443:
URL: https://github.com/apache/hive/pull/6443#discussion_r3217901670
##########
ql/src/java/org/apache/hadoop/hive/ql/parse/ColumnStatsSemanticAnalyzer.java:
##########
@@ -103,37 +104,32 @@ private boolean shouldRewrite(ASTNode tree) {
}
/**
- * Get the names of the columns that support column statistics.
+ * Get the Field Schemas of the columns that support column statistics.
*/
- private static List<String> getColumnNamesSupportingStats(Table tbl) {
- List<String> colNames = new ArrayList<>();
+ private static List<FieldSchema> getStatsEligibleFieldSchemas(Table tbl) {
+ List<FieldSchema> result = new ArrayList<>();
for (FieldSchema col : tbl.getCols()) {
String type = col.getType();
TypeInfo typeInfo = TypeInfoUtils.getTypeInfoFromTypeString(type);
boolean isSupported = ColumnStatsAutoGatherContext.isColumnSupported(typeInfo.getCategory(), () -> typeInfo);
if (isSupported) {
- colNames.add(col.getName());
+ result.add(col);
}
}
- return colNames;
+ return result;
}
- private List<String> getColumnName(ASTNode tree) throws SemanticException {
-
- switch (tree.getChildCount()) {
- case 2:
- return getColumnNamesSupportingStats(tbl);
- case 3:
- int numCols = tree.getChild(2).getChildCount();
- List<String> colName = new ArrayList<>(numCols);
- for (int i = 0; i < numCols; i++) {
- colName.add(getUnescapedName((ASTNode) tree.getChild(2).getChild(i)));
- }
- return colName;
- default:
- throw new SemanticException("Internal error. Expected number of children of ASTNode to be"
- + " either 2 or 3. Found : " + tree.getChildCount());
+ private List<String> getExplicitColumnNamesFromAst(ASTNode tree) throws SemanticException {
+ if (tree.getChildCount() != 3) {
Review Comment:
@tanishq-chugh, @thomasrebele : `if (tree.getChildCount() != 3)` looks like black magic to me at first sight :) , can you help the future code readers with a small code comment here regarding why only 3 is correct? I'm going to accept it, just need an explanation
The below exception message "Expected number of children of ASTNode should be 3" doesn't help at all
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-29551: Avoid quadratic runtime in ColumnStatsSemanticAnalyzer#ge… [hive]
Posted by "tanishq-chugh (via GitHub)" <gi...@apache.org>.
tanishq-chugh commented on code in PR #6443:
URL: https://github.com/apache/hive/pull/6443#discussion_r3219180067
##########
ql/src/java/org/apache/hadoop/hive/ql/parse/ColumnStatsSemanticAnalyzer.java:
##########
@@ -749,10 +761,9 @@ static AnalyzeRewriteContext genAnalyzeRewriteContext(HiveConf conf, Table tbl)
AnalyzeRewriteContext analyzeRewrite = new AnalyzeRewriteContext();
analyzeRewrite.setTableName(tbl.getFullyQualifiedName());
analyzeRewrite.setTblLvl(!(conf.getBoolVar(ConfVars.HIVE_STATS_COLLECT_PART_LEVEL_STATS) && tbl.isPartitioned()));
- List<String> colNames = getColumnNamesSupportingStats(tbl);
- List<String> colTypes = getColumnTypes(tbl, colNames);
- analyzeRewrite.setColName(colNames);
- analyzeRewrite.setColType(colTypes);
+ List<FieldSchema> columnSchemas = getStatsEligibleFieldSchemas(tbl);
+ analyzeRewrite.setColName(Utilities.getColumnNamesFromFieldSchema(columnSchemas));
+ analyzeRewrite.setColType(Utilities.getColumnTypesFromFieldSchema(columnSchemas));
Review Comment:
Refactored this in commit : [afc8188](https://github.com/apache/hive/pull/6443/commits/afc818817675c4362556ee80b1e7e08ac2892b22)
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-29551: Avoid quadratic runtime in ColumnStatsSemanticAnalyzer#ge… [hive]
Posted by "abstractdog (via GitHub)" <gi...@apache.org>.
abstractdog commented on code in PR #6443:
URL: https://github.com/apache/hive/pull/6443#discussion_r3220212053
##########
ql/src/java/org/apache/hadoop/hive/ql/parse/BaseSemanticAnalyzer.java:
##########
@@ -1428,12 +1427,16 @@ public void setTableName(String tableName) {
this.tableName = tableName;
}
- public List<String> getColName() {
- return colName;
+ public List<FieldSchema> getColumnSchemas() {
+ return columnSchemas;
+ }
+
+ public void setColumnSchemas(List<FieldSchema> columnSchemas) {
+ this.columnSchemas = columnSchemas;
}
- public void setColName(List<String> colName) {
- this.colName = colName;
+ public List<String> getColName() {
+ return columnSchemas == null ? null : Utilities.getColumnNamesFromFieldSchema(columnSchemas);
Review Comment:
I feel we should have a single constructor, so simply change the existing one
regarding complexity: right, I don't know this codepath in general, so I'm not sure either if we need to be extra cautious about storing `colName` separately, but there is a good way to handle this complexity: refactor it to a new class :D I mean, really, what about `FieldSchemas`, a simple container to handle this mess, and it can act as `FieldSchemas.getColName()` and `FieldSchemas.getColType()`: might seem overengineering, but already has some value, and you can introduce it early, so instead of:
```
private static List<FieldSchema> getStatsEligibleFieldSchemas(Table tbl) {
List<FieldSchema> result = new ArrayList<>();
...
return result;
}
```
you can do:
```
private static List<FieldSchema> getStatsEligibleFieldSchemas(Table tbl) {
List<FieldSchema> result = new ArrayList<>();
...
return new FieldSchemas(result);
}
```
this can already eliminate some functions from the already huge `Utilities` class
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-29551: Avoid quadratic runtime in ColumnStatsSemanticAnalyzer#ge… [hive]
Posted by "tanishq-chugh (via GitHub)" <gi...@apache.org>.
tanishq-chugh commented on code in PR #6443:
URL: https://github.com/apache/hive/pull/6443#discussion_r3220872569
##########
ql/src/java/org/apache/hadoop/hive/ql/parse/BaseSemanticAnalyzer.java:
##########
@@ -1428,12 +1427,16 @@ public void setTableName(String tableName) {
this.tableName = tableName;
}
- public List<String> getColName() {
- return colName;
+ public List<FieldSchema> getColumnSchemas() {
+ return columnSchemas;
+ }
+
+ public void setColumnSchemas(List<FieldSchema> columnSchemas) {
+ this.columnSchemas = columnSchemas;
}
- public void setColName(List<String> colName) {
- this.colName = colName;
+ public List<String> getColName() {
+ return columnSchemas == null ? null : Utilities.getColumnNamesFromFieldSchema(columnSchemas);
Review Comment:
Right, i have refactored the approach into a wrapper `FieldSchemas` as discussed in this commit: [2b1f556](https://github.com/apache/hive/pull/6443/commits/2b1f556c0e3373d385a7eff5b7ab27fde5b686bc)
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-29551: Avoid quadratic runtime in ColumnStatsSemanticAnalyzer#ge… [hive]
Posted by "thomasrebele (via GitHub)" <gi...@apache.org>.
thomasrebele commented on code in PR #6443:
URL: https://github.com/apache/hive/pull/6443#discussion_r3117290252
##########
ql/src/java/org/apache/hadoop/hive/ql/parse/ColumnStatsSemanticAnalyzer.java:
##########
@@ -205,23 +206,28 @@ private static String getColTypeOf(Table tbl, String partKey) {
protected static List<String> getColumnTypes(Table tbl, List<String> colNames) {
List<String> colTypes = new ArrayList<>();
List<FieldSchema> cols = tbl.getCols();
- List<String> copyColNames = new ArrayList<>(colNames);
-
- for (String colName : copyColNames) {
- for (FieldSchema col : cols) {
- if (colName.equalsIgnoreCase(col.getName())) {
- String type = col.getType();
- TypeInfo typeInfo = TypeInfoUtils.getTypeInfoFromTypeString(type);
- if (typeInfo.getCategory() != ObjectInspector.Category.PRIMITIVE) {
- logTypeWarning(colName, type);
- colNames.remove(colName);
- } else {
- colTypes.add(type);
- }
+ Map<String, String> colTypeMap = new HashMap<>();
Review Comment:
Thanks for the PR! When I created [HIVE-29551](https://issues.apache.org/jira/browse/HIVE-29551), I had in mind do it without a HashMap if possible. There are two types of usages, depending on where the column names came from:
* ColumnStatsSemanticAnalyzer#getColumnName
* Utilities.getColumnNamesFromFieldSchema
The latter iterates over a list of FieldSchema, so the type info can be obtained from these items as well.
The HashMap is only needed when the ASTNode has 3 children.
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-29551: Avoid quadratic runtime in ColumnStatsSemanticAnalyzer#ge… [hive]
Posted by "abstractdog (via GitHub)" <gi...@apache.org>.
abstractdog commented on PR #6443:
URL: https://github.com/apache/hive/pull/6443#issuecomment-4419599411
@tanishq-chugh: left a few comments: I believe we can eliminate the duplicated List of names + types from the code to simplify method signatures
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-29551: Avoid quadratic runtime in ColumnStatsSemanticAnalyzer#ge… [hive]
Posted by "tanishq-chugh (via GitHub)" <gi...@apache.org>.
tanishq-chugh commented on code in PR #6443:
URL: https://github.com/apache/hive/pull/6443#discussion_r3219186651
##########
ql/src/java/org/apache/hadoop/hive/ql/exec/Utilities.java:
##########
@@ -2286,6 +2286,14 @@ public static List<String> getColumnNamesFromFieldSchema(List<FieldSchema> partC
return names;
}
+ public static List<String> getColumnTypesFromFieldSchema(List<FieldSchema> fieldSchemas) {
+ List<String> types = new ArrayList<>();
+ for (FieldSchema fs : fieldSchemas) {
+ types.add(fs.getType());
+ }
+ return types;
Review Comment:
Made this change in commit: [4ca13d8](https://github.com/apache/hive/pull/6443/commits/4ca13d86634349416f0ae7d7479145d5356c0ccb)
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-29551: Avoid quadratic runtime in ColumnStatsSemanticAnalyzer#ge… [hive]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #6443:
URL: https://github.com/apache/hive/pull/6443#issuecomment-4421558784
## [](https://sonarcloud.io/dashboard?id=apache_hive&pullRequest=6443) **Quality Gate passed**
Issues
 [2 New issues](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=6443&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=6443&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_hive&pullRequest=6443&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_hive&pullRequest=6443&metric=new_coverage&view=list)
 [0.3% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_hive&pullRequest=6443&metric=new_duplicated_lines_density&view=list)
<!-- sqra-placement-anchor -->
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_hive&pullRequest=6443)
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-29551: Avoid quadratic runtime in ColumnStatsSemanticAnalyzer#ge… [hive]
Posted by "tanishq-chugh (via GitHub)" <gi...@apache.org>.
tanishq-chugh commented on code in PR #6443:
URL: https://github.com/apache/hive/pull/6443#discussion_r3219707088
##########
ql/src/java/org/apache/hadoop/hive/ql/parse/BaseSemanticAnalyzer.java:
##########
@@ -1428,12 +1427,16 @@ public void setTableName(String tableName) {
this.tableName = tableName;
}
- public List<String> getColName() {
- return colName;
+ public List<FieldSchema> getColumnSchemas() {
+ return columnSchemas;
+ }
+
+ public void setColumnSchemas(List<FieldSchema> columnSchemas) {
+ this.columnSchemas = columnSchemas;
}
- public void setColName(List<String> colName) {
- this.colName = colName;
+ public List<String> getColName() {
+ return columnSchemas == null ? null : Utilities.getColumnNamesFromFieldSchema(columnSchemas);
Review Comment:
@abstractdog I see only one call of `AnalyzeRewriteContext#getColName()` from [TaskCompiler](https://github.com/apache/hive/blob/master/ql/src/java/org/apache/hadoop/hive/ql/parse/TaskCompiler.java#L675-L676) before initialising `ColumnStatsDesc`, and no other calls which would lead to multiple calls to `Utilities.getColumnNamesFromFieldSchema()`.
I am afraid i did not understand fully about how could this lead to issues and rather removing getColName/getColType and introducing a new constructor in ColumnStatsDesc wouldn't be behaving same? could you please elaborate on this? Thanks!
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-29551: Avoid quadratic runtime in ColumnStatsSemanticAnalyzer#ge… [hive]
Posted by "abstractdog (via GitHub)" <gi...@apache.org>.
abstractdog commented on code in PR #6443:
URL: https://github.com/apache/hive/pull/6443#discussion_r3217959081
##########
ql/src/java/org/apache/hadoop/hive/ql/parse/ColumnStatsSemanticAnalyzer.java:
##########
@@ -736,6 +729,25 @@ public ASTNode rewriteAST(ASTNode ast, ColumnStatsAutoGatherContext context)
return rewrittenTree;
}
+ protected List<FieldSchema> getColumnsFromAst(ASTNode ast) throws SemanticException {
+ List<FieldSchema> statsEligibleFS = null;
+ List<String> columnNames;
+ if (ast.getChildCount() == 2) {
+ statsEligibleFS = getStatsEligibleFieldSchemas(tbl);
+ columnNames = Utilities.getColumnNamesFromFieldSchema(statsEligibleFS);
+ } else{
+ columnNames = getExplicitColumnNamesFromAst(ast);
+ }
+
+ checkForPartitionColumns(columnNames, Utilities.getColumnNamesFromFieldSchema(tbl.getPartitionKeys()));
+ validateSpecifiedColumnNames(columnNames);
+
+ if (statsEligibleFS != null) {
+ return statsEligibleFS;
+ }
+ return getFieldSchemasByColName(tbl, columnNames);
Review Comment:
what about merging this together?
`return statsEligibleFS != null ? statsEligibleFS : getFieldSchemasByColName(tbl, columnNames);`
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-29551: Avoid quadratic runtime in ColumnStatsSemanticAnalyzer#ge… [hive]
Posted by "thomasrebele (via GitHub)" <gi...@apache.org>.
thomasrebele commented on code in PR #6443:
URL: https://github.com/apache/hive/pull/6443#discussion_r3153232111
##########
ql/src/java/org/apache/hadoop/hive/ql/parse/ColumnStatsSemanticAnalyzer.java:
##########
@@ -102,38 +103,38 @@ private boolean shouldRewrite(ASTNode tree) {
return rwt;
}
+ private record StatsEligibleColumns(List<String> columnNames, List<String> columnTypes) {
Review Comment:
Instead of creating a new type, could you please use `List<FieldSchema>`, which contains both the name and the type of the column?
##########
ql/src/java/org/apache/hadoop/hive/ql/parse/ColumnStatsSemanticAnalyzer.java:
##########
@@ -102,38 +103,38 @@ private boolean shouldRewrite(ASTNode tree) {
return rwt;
}
+ private record StatsEligibleColumns(List<String> columnNames, List<String> columnTypes) {
+ }
+
/**
- * Get the names of the columns that support column statistics.
+ * Get the names and types of the columns that support column statistics.
*/
- private static List<String> getColumnNamesSupportingStats(Table tbl) {
+ private static StatsEligibleColumns getStatsEligibleColumns(Table tbl) {
List<String> colNames = new ArrayList<>();
+ List<String> colTypes = new ArrayList<>();
for (FieldSchema col : tbl.getCols()) {
String type = col.getType();
TypeInfo typeInfo = TypeInfoUtils.getTypeInfoFromTypeString(type);
boolean isSupported = ColumnStatsAutoGatherContext.isColumnSupported(typeInfo.getCategory(), () -> typeInfo);
if (isSupported) {
colNames.add(col.getName());
+ colTypes.add(col.getType());
}
}
- return colNames;
+ return new StatsEligibleColumns(colNames, colTypes);
}
private List<String> getColumnName(ASTNode tree) throws SemanticException {
Review Comment:
I would suggest to rename the function, maybe "getExplictColumnNames", though there may be a better name.
##########
ql/src/java/org/apache/hadoop/hive/ql/parse/ColumnStatsSemanticAnalyzer.java:
##########
@@ -217,27 +218,32 @@ private static String getColTypeOf(Table tbl, String partKey) {
throw new RuntimeException("Unknown partition key : " + partKey);
}
- protected static List<String> getColumnTypes(Table tbl, List<String> colNames) {
+ protected static List<String> getColumnTypesByName(Table tbl, List<String> colNames) {
List<String> colTypes = new ArrayList<>();
List<FieldSchema> cols = tbl.getCols();
- List<String> copyColNames = new ArrayList<>(colNames);
-
- for (String colName : copyColNames) {
- for (FieldSchema col : cols) {
- if (colName.equalsIgnoreCase(col.getName())) {
- String type = col.getType();
- TypeInfo typeInfo = TypeInfoUtils.getTypeInfoFromTypeString(type);
- boolean isSupported = ColumnStatsAutoGatherContext.isColumnSupported(typeInfo.getCategory(), () -> typeInfo);
- if (!isSupported) {
- logTypeWarning(colName, type);
- colNames.remove(colName);
- } else {
- colTypes.add(type);
- }
+ Map<String, String> colTypeMap = new HashMap<>();
+
+ for (FieldSchema col : cols) {
+ colTypeMap.put(col.getName().toLowerCase(), col.getType());
+ }
+
+ List<String> primColNames = new ArrayList<>();
+ for (String colName : colNames) {
+ String type = colTypeMap.get(colName.toLowerCase());
+ if (type != null) {
+ TypeInfo typeInfo = TypeInfoUtils.getTypeInfoFromTypeString(type);
+ boolean isSupported = ColumnStatsAutoGatherContext.isColumnSupported(typeInfo.getCategory(), () -> typeInfo);
+ if (!isSupported) {
+ logTypeWarning(colName, type);
+ } else {
+ primColNames.add(colName);
+ colTypes.add(type);
}
}
}
+ colNames.clear();
+ colNames.addAll(primColNames);
Review Comment:
Modifying the argument can be avoided when implementing my other comments.
##########
ql/src/java/org/apache/hadoop/hive/ql/parse/ColumnStatsSemanticAnalyzer.java:
##########
@@ -217,27 +218,32 @@ private static String getColTypeOf(Table tbl, String partKey) {
throw new RuntimeException("Unknown partition key : " + partKey);
}
- protected static List<String> getColumnTypes(Table tbl, List<String> colNames) {
+ protected static List<String> getColumnTypesByName(Table tbl, List<String> colNames) {
Review Comment:
I recommend to refactor getColumnTypesByName to return `List<FieldSchema>`.
##########
ql/src/java/org/apache/hadoop/hive/ql/parse/ColumnStatsSemanticAnalyzer.java:
##########
@@ -102,38 +103,38 @@ private boolean shouldRewrite(ASTNode tree) {
return rwt;
}
+ private record StatsEligibleColumns(List<String> columnNames, List<String> columnTypes) {
+ }
+
/**
- * Get the names of the columns that support column statistics.
+ * Get the names and types of the columns that support column statistics.
*/
- private static List<String> getColumnNamesSupportingStats(Table tbl) {
+ private static StatsEligibleColumns getStatsEligibleColumns(Table tbl) {
List<String> colNames = new ArrayList<>();
+ List<String> colTypes = new ArrayList<>();
for (FieldSchema col : tbl.getCols()) {
String type = col.getType();
TypeInfo typeInfo = TypeInfoUtils.getTypeInfoFromTypeString(type);
boolean isSupported = ColumnStatsAutoGatherContext.isColumnSupported(typeInfo.getCategory(), () -> typeInfo);
if (isSupported) {
colNames.add(col.getName());
+ colTypes.add(col.getType());
}
}
- return colNames;
+ return new StatsEligibleColumns(colNames, colTypes);
}
private List<String> getColumnName(ASTNode tree) throws SemanticException {
-
- switch (tree.getChildCount()) {
- case 2:
- return getColumnNamesSupportingStats(tbl);
- case 3:
- int numCols = tree.getChild(2).getChildCount();
- List<String> colName = new ArrayList<>(numCols);
- for (int i = 0; i < numCols; i++) {
- colName.add(getUnescapedName((ASTNode) tree.getChild(2).getChild(i)));
- }
- return colName;
- default:
+ if (tree.getChildCount() != 3) {
throw new SemanticException("Internal error. Expected number of children of ASTNode to be"
+ " either 2 or 3. Found : " + tree.getChildCount());
Review Comment:
If we modify the method that way, the expected number of children is 3, so the exception message would need to be changed.
##########
ql/src/java/org/apache/hadoop/hive/ql/parse/ColumnStatsSemanticAnalyzer.java:
##########
@@ -634,7 +640,13 @@ public void analyze(ASTNode ast, Context origCtx) throws SemanticException {
*/
if (shouldRewrite(ast)) {
tbl = AnalyzeCommandUtils.getTable(ast, this);
- colNames = getColumnName(ast);
+ StatsEligibleColumns statsCols = null;
+ if (ast.getChildCount() == 2) {
+ statsCols = getStatsEligibleColumns(tbl);
+ colNames = statsCols.columnNames();
+ } else {
+ colNames = getColumnName(ast);
+ }
Review Comment:
The handling of the AST should stay at once place to avoid code duplication here and in #rewriteAST. Maybe a new method `List<FieldSchema> getColumns(ASTNode)`. To keep the behavior the same, I would do roughly the following:
1. Collect the column names as string using the original method
2. Verify the names with checkForPartitionColumns and validateSpecifiedColumnNames (and removing the calls to these functions in ColumnStatsSemanticAnalyzer#rewriteAST and ColumnStatsSemanticAnalyzer#analyze)
3. Collect the columns as `List<FieldSchema>`
4. The caller extracts the names (with `org.apache.hadoop.hive.ql.exec.Utilities#getColumnNamesFromFieldSchema`) and the types (I don't know of an existing function, at least I couldn't find one in Utilities).
This approach avoids the need to modify the column names later, and should make the code easier to understand. It would be nice (if that optimization does not make the code too complex) to optimize the case `ast.getChildCount() == 2`, so that step 1 and 3 only collect the columns once.
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-29551: Avoid quadratic runtime in ColumnStatsSemanticAnalyzer#ge… [hive]
Posted by "abstractdog (via GitHub)" <gi...@apache.org>.
abstractdog commented on PR #6443:
URL: https://github.com/apache/hive/pull/6443#issuecomment-4475288277
> Hi @Aggarwal-Raghav Thanks for pointing it out & yes, this is definitely better for optimisation. I have refactored the same in commit [37d1c22](https://github.com/apache/hive/pull/6443/commits/37d1c2267a120a76d37e214c5e968ee2d2ef859e)
>
> CC: @abstractdog
commit looks good to me and looks like it's addressing the optimization idea, which is good, so my approval still holds: please let us know @Aggarwal-Raghav if you're okay with the patch now
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-29551: Avoid quadratic runtime in ColumnStatsSemanticAnalyzer#ge… [hive]
Posted by "abstractdog (via GitHub)" <gi...@apache.org>.
abstractdog commented on code in PR #6443:
URL: https://github.com/apache/hive/pull/6443#discussion_r3217901670
##########
ql/src/java/org/apache/hadoop/hive/ql/parse/ColumnStatsSemanticAnalyzer.java:
##########
@@ -103,37 +104,32 @@ private boolean shouldRewrite(ASTNode tree) {
}
/**
- * Get the names of the columns that support column statistics.
+ * Get the Field Schemas of the columns that support column statistics.
*/
- private static List<String> getColumnNamesSupportingStats(Table tbl) {
- List<String> colNames = new ArrayList<>();
+ private static List<FieldSchema> getStatsEligibleFieldSchemas(Table tbl) {
+ List<FieldSchema> result = new ArrayList<>();
for (FieldSchema col : tbl.getCols()) {
String type = col.getType();
TypeInfo typeInfo = TypeInfoUtils.getTypeInfoFromTypeString(type);
boolean isSupported = ColumnStatsAutoGatherContext.isColumnSupported(typeInfo.getCategory(), () -> typeInfo);
if (isSupported) {
- colNames.add(col.getName());
+ result.add(col);
}
}
- return colNames;
+ return result;
}
- private List<String> getColumnName(ASTNode tree) throws SemanticException {
-
- switch (tree.getChildCount()) {
- case 2:
- return getColumnNamesSupportingStats(tbl);
- case 3:
- int numCols = tree.getChild(2).getChildCount();
- List<String> colName = new ArrayList<>(numCols);
- for (int i = 0; i < numCols; i++) {
- colName.add(getUnescapedName((ASTNode) tree.getChild(2).getChild(i)));
- }
- return colName;
- default:
- throw new SemanticException("Internal error. Expected number of children of ASTNode to be"
- + " either 2 or 3. Found : " + tree.getChildCount());
+ private List<String> getExplicitColumnNamesFromAst(ASTNode tree) throws SemanticException {
+ if (tree.getChildCount() != 3) {
Review Comment:
@tanishq-chugh, @thomasrebele : `if (tree.getChildCount() != 3)` looks like black magic to me at first sight :) , can you help the future code readers with a small code comment here, which only 3 is correct? I'm going to accept it, just need an explanation
The below exception message "Expected number of children of ASTNode should be 3" doesn't help at all
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-29551: Avoid quadratic runtime in ColumnStatsSemanticAnalyzer#ge… [hive]
Posted by "abstractdog (via GitHub)" <gi...@apache.org>.
abstractdog commented on code in PR #6443:
URL: https://github.com/apache/hive/pull/6443#discussion_r3219776096
##########
ql/src/java/org/apache/hadoop/hive/ql/parse/BaseSemanticAnalyzer.java:
##########
@@ -1428,12 +1427,16 @@ public void setTableName(String tableName) {
this.tableName = tableName;
}
- public List<String> getColName() {
- return colName;
+ public List<FieldSchema> getColumnSchemas() {
+ return columnSchemas;
+ }
+
+ public void setColumnSchemas(List<FieldSchema> columnSchemas) {
+ this.columnSchemas = columnSchemas;
}
- public void setColName(List<String> colName) {
- this.colName = colName;
+ public List<String> getColName() {
+ return columnSchemas == null ? null : Utilities.getColumnNamesFromFieldSchema(columnSchemas);
Review Comment:
if by static code analysis we can be sure that `getColName` is not called e.g. 100 times during compilation, this is going to be fine, I'm inclined to accept your analysis here
my point was only to completely remove the separate `colName` and `colType` variables from method signatures where it's possible, and only have them as separate variables where it's really needed (e.g. where EXPLAIN plan is used), e.g.:
```
@Explain(displayName = "Columns")
public List<String> getColName() {
return colName;
}
```
so I can see that TaskCompiler uses them separately only for passing them to ColumnStatsDesc, which just complicates code, so what about passing schemas to ColumnStatsDesc, and we can resolve names/types there like:
```
@Explain(displayName = "Column Types")
public List<String> getColType() {
return ...the logic parsing types for schemas...;
}
```
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-29551: Avoid quadratic runtime in ColumnStatsSemanticAnalyzer#ge… [hive]
Posted by "tanishq-chugh (via GitHub)" <gi...@apache.org>.
tanishq-chugh commented on code in PR #6443:
URL: https://github.com/apache/hive/pull/6443#discussion_r3220057650
##########
ql/src/java/org/apache/hadoop/hive/ql/parse/BaseSemanticAnalyzer.java:
##########
@@ -1428,12 +1427,16 @@ public void setTableName(String tableName) {
this.tableName = tableName;
}
- public List<String> getColName() {
- return colName;
+ public List<FieldSchema> getColumnSchemas() {
+ return columnSchemas;
+ }
+
+ public void setColumnSchemas(List<FieldSchema> columnSchemas) {
+ this.columnSchemas = columnSchemas;
}
- public void setColName(List<String> colName) {
- this.colName = colName;
+ public List<String> getColName() {
+ return columnSchemas == null ? null : Utilities.getColumnNamesFromFieldSchema(columnSchemas);
Review Comment:
Yeah, that definitely makes sense that we should remove colName and colType usage wherever possible to ensure consistency.
So, what about introducing a new constructor in `ColumnStatsDesc` like:
```
public ColumnStatsDesc(String tableName, List<FieldSchema> columnSchemas, boolean isTblLevel,
int numBitVector, FetchWork fWork1) {
this(tableName,
columnSchemas == null ? null : Utilities.getColumnNamesFromFieldSchema(columnSchemas),
columnSchemas == null ? null : Utilities.getColumnTypesFromFieldSchema(columnSchemas),
isTblLevel, numBitVector, fWork1);
}
```
To compute and store the colNames and colTypes in ColumnStatsDesc itself.
Thinking of changing the `ColumnStatsDesc` constructor to have only `columnSchemas` instead of `colNames / colTypes`, the `getColName / getColType` will have to be changed to either:
```
@Explain(displayName = "Columns")
public List<String> getColName() {
return columnSchemas == null ? null : Utilities.getColumnNamesFromFieldSchema(columnSchemas);
}
```
or
```
@Explain(displayName = "Columns")
public List<String> getColName() {
if (colName != null) {
return colName;
}
colName = columnSchemas == null ? null : Utilities.getColumnTypesFromFieldSchema(columnSchemas);
return colName;
}
```
In the first case, we will have multiple calls to getColumnNamesFromFieldSchema() as i see from static code, multiple usages of getColName() here.
In the second case, we will have columnSchemas as class variable in addition to colName and colType, increasing the complexity in my opinion.
Let me know what do u think of introducing the new constructor as above and keeping the getColName / getColType same as is? Thanks!
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-29551: Avoid quadratic runtime in ColumnStatsSemanticAnalyzer#ge… [hive]
Posted by "abstractdog (via GitHub)" <gi...@apache.org>.
abstractdog commented on code in PR #6443:
URL: https://github.com/apache/hive/pull/6443#discussion_r3219218538
##########
ql/src/java/org/apache/hadoop/hive/ql/parse/BaseSemanticAnalyzer.java:
##########
@@ -1428,12 +1427,16 @@ public void setTableName(String tableName) {
this.tableName = tableName;
}
- public List<String> getColName() {
- return colName;
+ public List<FieldSchema> getColumnSchemas() {
+ return columnSchemas;
+ }
+
+ public void setColumnSchemas(List<FieldSchema> columnSchemas) {
+ this.columnSchemas = columnSchemas;
}
- public void setColName(List<String> colName) {
- this.colName = colName;
+ public List<String> getColName() {
+ return columnSchemas == null ? null : Utilities.getColumnNamesFromFieldSchema(columnSchemas);
Review Comment:
is there a chance you can go one level further?
we cannot be sure how hot is `getColName`, so running `getColumnNamesFromFieldSchema` all the time can be even worse...I mean without investigating this, we can even simply check the callers and adapt them accordingly, like in `ColumnStatsDesc`
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-29551: Avoid quadratic runtime in ColumnStatsSemanticAnalyzer#ge… [hive]
Posted by "abstractdog (via GitHub)" <gi...@apache.org>.
abstractdog commented on code in PR #6443:
URL: https://github.com/apache/hive/pull/6443#discussion_r3221665725
##########
ql/src/java/org/apache/hadoop/hive/ql/parse/ColumnStatsSemanticAnalyzer.java:
##########
@@ -257,28 +255,27 @@ private String genRewrittenQuery(List<String> colNames, List<String> colTypes, H
protected static String genRewrittenQuery(Table tbl,
HiveConf conf, List<TransformSpec> partTransformSpec, Map<String, String> partSpec,
boolean isPartitionStats) {
- List<String> colNames = getColumnNamesSupportingStats(tbl);
- List<String> colTypes = ColumnStatsSemanticAnalyzer.getColumnTypes(tbl, colNames);
return ColumnStatsSemanticAnalyzer.genRewrittenQuery(
- tbl, colNames, colTypes, conf, partTransformSpec, -1, partSpec, isPartitionStats, true);
+ tbl, getStatsEligibleFieldSchemas(tbl).getSchemas(), conf, partTransformSpec, -1, partSpec, isPartitionStats, true);
}
- private static String genRewrittenQuery(Table tbl, List<String> colNames, List<String> colTypes,
+ private static String genRewrittenQuery(Table tbl, List<FieldSchema> columnSchemas,
Review Comment:
nit: I like `FieldSchemas` as it is, so what about using it here too (as you already touched the signature)? it can also have a `size()` and `get(i)` method to fulfill the requirements in this method
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-29551: Avoid quadratic runtime in ColumnStatsSemanticAnalyzer#ge… [hive]
Posted by "tanishq-chugh (via GitHub)" <gi...@apache.org>.
tanishq-chugh commented on code in PR #6443:
URL: https://github.com/apache/hive/pull/6443#discussion_r3159125356
##########
ql/src/java/org/apache/hadoop/hive/ql/parse/ColumnStatsSemanticAnalyzer.java:
##########
@@ -634,7 +640,13 @@ public void analyze(ASTNode ast, Context origCtx) throws SemanticException {
*/
if (shouldRewrite(ast)) {
tbl = AnalyzeCommandUtils.getTable(ast, this);
- colNames = getColumnName(ast);
+ StatsEligibleColumns statsCols = null;
+ if (ast.getChildCount() == 2) {
+ statsCols = getStatsEligibleColumns(tbl);
+ colNames = statsCols.columnNames();
+ } else {
+ colNames = getColumnName(ast);
+ }
Review Comment:
Thanks for pointing this out @thomasrebele !
And, yes this definitely makes more sense and helps to keep code clean. I have made all these changes in commit - [84d81f9](https://github.com/apache/hive/pull/6443/commits/84d81f933cf7ecd8112293895c3d3997cc7292f1)
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-29551: Avoid quadratic runtime in ColumnStatsSemanticAnalyzer#ge… [hive]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #6443:
URL: https://github.com/apache/hive/pull/6443#issuecomment-4338708879
## [](https://sonarcloud.io/dashboard?id=apache_hive&pullRequest=6443) **Quality Gate passed**
Issues
 [4 New issues](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=6443&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=6443&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_hive&pullRequest=6443&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_hive&pullRequest=6443&metric=new_coverage&view=list)
 [0.3% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_hive&pullRequest=6443&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_hive&pullRequest=6443)
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-29551: Avoid quadratic runtime in ColumnStatsSemanticAnalyzer#ge… [hive]
Posted by "Aggarwal-Raghav (via GitHub)" <gi...@apache.org>.
Aggarwal-Raghav commented on PR #6443:
URL: https://github.com/apache/hive/pull/6443#issuecomment-4448600043
@tanishq-chugh / @abstractdog , I have a question.
1. In `validateSpecifiedColumnNames` we are checking if columns exists — 1 HashMap
2. In `checkForPartitionColumns` we are checking for partitions columns — 1 HashSet
3. In `getFieldSchemasByColName` we are getting the type of the above validated columns — 1 HashMap
Can't we do all 1 and 2 inside 3 while maintaining 1 DataStrucuture? I think it should be possible.
**The optimization + refactoring in this patch is good**.
Just thinking in terms of math, ColumnStatsSemanticAnalyzer will run in `Query Compilation` phase so If my competitive coding concepts are correct then:
```
For 1000 columns, O(N^2) => 1Million i.e 10^6, which modern computer it does this in 1 sec.
```
For columns more than 3k or so the real benefit of this optimization will kick in i guess.
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-29551: Avoid quadratic runtime in ColumnStatsSemanticAnalyzer#ge… [hive]
Posted by "Aggarwal-Raghav (via GitHub)" <gi...@apache.org>.
Aggarwal-Raghav commented on code in PR #6443:
URL: https://github.com/apache/hive/pull/6443#discussion_r3105571000
##########
ql/src/java/org/apache/hadoop/hive/ql/parse/ColumnStatsSemanticAnalyzer.java:
##########
@@ -205,23 +206,28 @@ private static String getColTypeOf(Table tbl, String partKey) {
protected static List<String> getColumnTypes(Table tbl, List<String> colNames) {
List<String> colTypes = new ArrayList<>();
List<FieldSchema> cols = tbl.getCols();
- List<String> copyColNames = new ArrayList<>(colNames);
-
- for (String colName : copyColNames) {
- for (FieldSchema col : cols) {
- if (colName.equalsIgnoreCase(col.getName())) {
- String type = col.getType();
- TypeInfo typeInfo = TypeInfoUtils.getTypeInfoFromTypeString(type);
- if (typeInfo.getCategory() != ObjectInspector.Category.PRIMITIVE) {
- logTypeWarning(colName, type);
- colNames.remove(colName);
- } else {
- colTypes.add(type);
- }
+ Map<String, String> colTypeMap = new HashMap<>();
+
+ for (FieldSchema col : cols) {
+ colTypeMap.put(col.getName().toLowerCase(), col.getType());
+ }
+
+ List<String> nonPrimColNames = new ArrayList<>();
+ for (String colName : colNames) {
+ String type = colTypeMap.get(colName.toLowerCase());
+ if (type != null) {
+ TypeInfo typeInfo = TypeInfoUtils.getTypeInfoFromTypeString(type);
+ if (typeInfo.getCategory() != ObjectInspector.Category.PRIMITIVE) {
+ logTypeWarning(colName, type);
+ } else {
+ nonPrimColNames.add(colName);
Review Comment:
the variable name should be `PrimColNames` instead of `nonPrimColNames`. As the primitve type will enter the else flow.
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-29551: Avoid quadratic runtime in ColumnStatsSemanticAnalyzer#ge… [hive]
Posted by "tanishq-chugh (via GitHub)" <gi...@apache.org>.
tanishq-chugh commented on code in PR #6443:
URL: https://github.com/apache/hive/pull/6443#discussion_r3227023935
##########
ql/src/java/org/apache/hadoop/hive/ql/parse/ColumnStatsSemanticAnalyzer.java:
##########
@@ -726,33 +717,46 @@ public ASTNode rewriteAST(ASTNode ast, ColumnStatsAutoGatherContext context)
partTransformSpec = tbl.getStorageHandler().getPartitionTransformSpec(tbl);
}
}
- colType = getColumnTypes(tbl, colNames);
+ rewrittenColumnSchemas = new FieldSchemas(columnSchemas);
isTableLevel = !isPartitionStats;
- rewrittenQuery = genRewrittenQuery(colNames, colType, conf, partTransformSpec, -1,
+ rewrittenQuery = genRewrittenQuery(rewrittenColumnSchemas, conf, partTransformSpec, -1,
partSpec, isPartitionStats);
rewrittenTree = genRewrittenTree(rewrittenQuery);
return rewrittenTree;
}
+ protected List<FieldSchema> getColumnsFromAst(ASTNode ast) throws SemanticException {
+ List<FieldSchema> statsEligibleFS = null;
+ List<String> columnNames;
+ if (ast.getChildCount() == 2) {
+ FieldSchemas eligibleFS = getStatsEligibleFieldSchemas(tbl);
+ statsEligibleFS = eligibleFS.getSchemas();
+ columnNames = eligibleFS.getColName();
+ } else {
+ columnNames = getExplicitColumnNamesFromAst(ast);
+ }
+
+ checkForPartitionColumns(columnNames, Utilities.getColumnNamesFromFieldSchema(tbl.getPartitionKeys()));
Review Comment:
Made this change in commit: [68446bd](https://github.com/apache/hive/pull/6443/commits/68446bd50fcca103d8533386e936814f72f30725)
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-29551: Avoid quadratic runtime in ColumnStatsSemanticAnalyzer#ge… [hive]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #6443:
URL: https://github.com/apache/hive/pull/6443#issuecomment-4428092698
## [](https://sonarcloud.io/dashboard?id=apache_hive&pullRequest=6443) **Quality Gate passed**
Issues
 [2 New issues](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=6443&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=6443&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_hive&pullRequest=6443&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_hive&pullRequest=6443&metric=new_coverage&view=list)
 [0.3% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_hive&pullRequest=6443&metric=new_duplicated_lines_density&view=list)
<!-- sqra-placement-anchor -->
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_hive&pullRequest=6443)
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-29551: Refactor ColumnStatsSemanticAnalyzer for performance optimization [hive]
Posted by "abstractdog (via GitHub)" <gi...@apache.org>.
abstractdog merged PR #6443:
URL: https://github.com/apache/hive/pull/6443
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-29551: Avoid quadratic runtime in ColumnStatsSemanticAnalyzer#ge… [hive]
Posted by "tanishq-chugh (via GitHub)" <gi...@apache.org>.
tanishq-chugh commented on code in PR #6443:
URL: https://github.com/apache/hive/pull/6443#discussion_r3159110115
##########
ql/src/java/org/apache/hadoop/hive/ql/parse/ColumnStatsSemanticAnalyzer.java:
##########
@@ -102,38 +103,38 @@ private boolean shouldRewrite(ASTNode tree) {
return rwt;
}
+ private record StatsEligibleColumns(List<String> columnNames, List<String> columnTypes) {
Review Comment:
Made this change in commit - [84d81f9](https://github.com/apache/hive/pull/6443/commits/84d81f933cf7ecd8112293895c3d3997cc7292f1)
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-29551: Avoid quadratic runtime in ColumnStatsSemanticAnalyzer#ge… [hive]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #6443:
URL: https://github.com/apache/hive/pull/6443#issuecomment-4341774290
## [](https://sonarcloud.io/dashboard?id=apache_hive&pullRequest=6443) **Quality Gate passed**
Issues
 [0 New issues](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=6443&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=6443&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_hive&pullRequest=6443&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_hive&pullRequest=6443&metric=new_coverage&view=list)
 [0.3% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_hive&pullRequest=6443&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_hive&pullRequest=6443)
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-29551: Avoid quadratic runtime in ColumnStatsSemanticAnalyzer#ge… [hive]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #6443:
URL: https://github.com/apache/hive/pull/6443#issuecomment-4454876940
## [](https://sonarcloud.io/dashboard?id=apache_hive&pullRequest=6443) **Quality Gate passed**
Issues
 [8 New issues](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=6443&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=6443&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_hive&pullRequest=6443&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_hive&pullRequest=6443&metric=new_coverage&view=list)
 [0.3% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_hive&pullRequest=6443&metric=new_duplicated_lines_density&view=list)
<!-- sqra-placement-anchor -->
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_hive&pullRequest=6443)
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-29551: Avoid quadratic runtime in ColumnStatsSemanticAnalyzer#ge… [hive]
Posted by "tanishq-chugh (via GitHub)" <gi...@apache.org>.
tanishq-chugh commented on code in PR #6443:
URL: https://github.com/apache/hive/pull/6443#discussion_r3219186651
##########
ql/src/java/org/apache/hadoop/hive/ql/exec/Utilities.java:
##########
@@ -2286,6 +2286,14 @@ public static List<String> getColumnNamesFromFieldSchema(List<FieldSchema> partC
return names;
}
+ public static List<String> getColumnTypesFromFieldSchema(List<FieldSchema> fieldSchemas) {
+ List<String> types = new ArrayList<>();
+ for (FieldSchema fs : fieldSchemas) {
+ types.add(fs.getType());
+ }
+ return types;
Review Comment:
MAde this change in commit: [4ca13d8](https://github.com/apache/hive/pull/6443/commits/4ca13d86634349416f0ae7d7479145d5356c0ccb)
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-29551: Avoid quadratic runtime in ColumnStatsSemanticAnalyzer#ge… [hive]
Posted by "abstractdog (via GitHub)" <gi...@apache.org>.
abstractdog commented on code in PR #6443:
URL: https://github.com/apache/hive/pull/6443#discussion_r3219776096
##########
ql/src/java/org/apache/hadoop/hive/ql/parse/BaseSemanticAnalyzer.java:
##########
@@ -1428,12 +1427,16 @@ public void setTableName(String tableName) {
this.tableName = tableName;
}
- public List<String> getColName() {
- return colName;
+ public List<FieldSchema> getColumnSchemas() {
+ return columnSchemas;
+ }
+
+ public void setColumnSchemas(List<FieldSchema> columnSchemas) {
+ this.columnSchemas = columnSchemas;
}
- public void setColName(List<String> colName) {
- this.colName = colName;
+ public List<String> getColName() {
+ return columnSchemas == null ? null : Utilities.getColumnNamesFromFieldSchema(columnSchemas);
Review Comment:
if by static code analysis we can be sure that `getColName` is not called e.g. 100 times during compilation, this is going to be fine, I'm inclined to accept your analysis here
my point was only to completely remove the separate `colName` and `colType` variables from method signatures where it's possible, and only have them as separate variables where it's really needed (e.g. where EXPLAIN plan is used), e.g.:
```
@Explain(displayName = "Columns")
public List<String> getColName() {
return colName;
}
```
so I can see that TaskCompiler uses them separately only for passing them to ColumnStatsDesc, which just complicates code
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-29551: Avoid quadratic runtime in ColumnStatsSemanticAnalyzer#ge… [hive]
Posted by "abstractdog (via GitHub)" <gi...@apache.org>.
abstractdog commented on code in PR #6443:
URL: https://github.com/apache/hive/pull/6443#discussion_r3217943769
##########
ql/src/java/org/apache/hadoop/hive/ql/parse/ColumnStatsSemanticAnalyzer.java:
##########
@@ -257,10 +252,11 @@ private String genRewrittenQuery(List<String> colNames, List<String> colTypes, H
protected static String genRewrittenQuery(Table tbl,
HiveConf conf, List<TransformSpec> partTransformSpec, Map<String, String> partSpec,
boolean isPartitionStats) {
- List<String> colNames = getColumnNamesSupportingStats(tbl);
- List<String> colTypes = ColumnStatsSemanticAnalyzer.getColumnTypes(tbl, colNames);
+ List<FieldSchema> columnSchemas = getStatsEligibleFieldSchemas(tbl);
return ColumnStatsSemanticAnalyzer.genRewrittenQuery(
- tbl, colNames, colTypes, conf, partTransformSpec, -1, partSpec, isPartitionStats, true);
+ tbl, Utilities.getColumnNamesFromFieldSchema(columnSchemas),
+ Utilities.getColumnTypesFromFieldSchema(columnSchemas), conf, partTransformSpec, -1, partSpec,
Review Comment:
in order to call `genRewrittenQuery`, we parse `columnSchemas` back to names and types, why don't simply pass `columnSchemas` as is?
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-29551: Avoid quadratic runtime in ColumnStatsSemanticAnalyzer#ge… [hive]
Posted by "tanishq-chugh (via GitHub)" <gi...@apache.org>.
tanishq-chugh commented on code in PR #6443:
URL: https://github.com/apache/hive/pull/6443#discussion_r3219707088
##########
ql/src/java/org/apache/hadoop/hive/ql/parse/BaseSemanticAnalyzer.java:
##########
@@ -1428,12 +1427,16 @@ public void setTableName(String tableName) {
this.tableName = tableName;
}
- public List<String> getColName() {
- return colName;
+ public List<FieldSchema> getColumnSchemas() {
+ return columnSchemas;
+ }
+
+ public void setColumnSchemas(List<FieldSchema> columnSchemas) {
+ this.columnSchemas = columnSchemas;
}
- public void setColName(List<String> colName) {
- this.colName = colName;
+ public List<String> getColName() {
+ return columnSchemas == null ? null : Utilities.getColumnNamesFromFieldSchema(columnSchemas);
Review Comment:
@abstractdog I see only one call of `AnalyzeRewriteContext#getColName()` from [TaskCompiler](https://github.com/apache/hive/blob/master/ql/src/java/org/apache/hadoop/hive/ql/parse/TaskCompiler.java#L675-L676) before initialising `ColumnStatsDesc`, and no other calls which would lead to multiple calls to `Utilities.getColumnNamesFromFieldSchema()`.
I am afraid i did not fully understand about how could this lead to issues and rather removing getColName/getColType and introducing a new constructor in ColumnStatsDesc using columnSchemas instead of colname/coltype which internally calls same Utility funcs wouldn't be behaving the same? could you please elaborate on this? Thanks!
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-29551: Avoid quadratic runtime in ColumnStatsSemanticAnalyzer#ge… [hive]
Posted by "tanishq-chugh (via GitHub)" <gi...@apache.org>.
tanishq-chugh commented on PR #6443:
URL: https://github.com/apache/hive/pull/6443#issuecomment-4458846833
Hi @Aggarwal-Raghav
Thanks for pointing it out & yes, this is definitely a better for optimisation. I have refactored the same in commit [37d1c22](https://github.com/apache/hive/pull/6443/commits/37d1c2267a120a76d37e214c5e968ee2d2ef859e)
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org