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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
