[GitHub] kgyrtkirk commented on a change in pull request #550: HIVE-21198 Introduce a database object reference class

[GitBox]

kgyrtkirk commented on a change in pull request #550: HIVE-21198 Introduce a 
database object reference class
URL: https://github.com/apache/hive/pull/550#discussion_r260256192
 
 

 ##
 File path: 
ql/src/java/org/apache/hadoop/hive/ql/exec/repl/incremental/IncrementalLoadTasksBuilder.java
 ##
 @@ -266,16 +271,15 @@ private boolean shouldReplayEvent(FileStatus dir, 
DumpType dumpType, String dbNa
 return updateReplIdTxnTask;
   }
 
-  private Task tableUpdateReplStateTask(String dbName, 
String tableName,
-Map 
partSpec, String replState,
-Task preCursor) throws SemanticException {
+  private Task tableUpdateReplStateTask(TableName 
tableName, Map partSpec,
 
 Review comment:
   now that I see this...I think that it would probably make sense to also 
include `partSpec` into this object
   (`PartishRef`: `catalog,database,tablename,partitonspec` ) ?


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] kgyrtkirk commented on a change in pull request #550: HIVE-21198 Introduce a database object reference class

[GitBox]

kgyrtkirk commented on a change in pull request #550: HIVE-21198 Introduce a 
database object reference class
URL: https://github.com/apache/hive/pull/550#discussion_r260253361
 
 

 ##
 File path: 
hcatalog/core/src/main/java/org/apache/hive/hcatalog/cli/SemanticAnalysis/HCatSemanticAnalyzer.java
 ##
 @@ -348,8 +348,8 @@ protected void 
authorizeDDLWork(HiveSemanticAnalyzerHookContext cntxt, Hive hive
 
 AlterTableDesc alterTable = work.getAlterTblDesc();
 if (alterTable != null) {
-  Table table = hive.getTable(SessionState.get().getCurrentDatabase(),
-  Utilities.getDbTableName(alterTable.getOldName())[1], false);
+  final String tableName = 
Utilities.getTableName(alterTable.getOldName()).getTable();
 
 Review comment:
   I guess we can't add `of` methods to `TableName`; but I feel like putting 
this into "Utilities" is kinda unfortunate.
   How about: `HiveTableName.of(...)`
   If you need any utilities you can probably make HiveTableName extend 
TableName - not sure if that will be needed or not


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] kgyrtkirk commented on a change in pull request #550: HIVE-21198 Introduce a database object reference class

[GitBox]

kgyrtkirk commented on a change in pull request #550: HIVE-21198 Introduce a 
database object reference class
URL: https://github.com/apache/hive/pull/550#discussion_r260253836
 
 

 ##
 File path: 
hcatalog/core/src/main/java/org/apache/hive/hcatalog/cli/SemanticAnalysis/HCatSemanticAnalyzer.java
 ##
 @@ -348,8 +348,8 @@ protected void 
authorizeDDLWork(HiveSemanticAnalyzerHookContext cntxt, Hive hive
 
 AlterTableDesc alterTable = work.getAlterTblDesc();
 if (alterTable != null) {
-  Table table = hive.getTable(SessionState.get().getCurrentDatabase(),
-  Utilities.getDbTableName(alterTable.getOldName())[1], false);
+  final String tableName = 
Utilities.getTableName(alterTable.getOldName()).getTable();
+  Table table = hive.getTable(SessionState.get().getCurrentDatabase(), 
tableName, false);
 
 Review comment:
   `TableName` contains the database name; 
`SessionState.get().getCurrentDatabase()` should appear only at the time of 
creating the `TableName` - so as a sideeffect this "getTable()" will expect 
`TableName` argument


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] kgyrtkirk commented on a change in pull request #550: HIVE-21198 Introduce a database object reference class

[GitBox]

kgyrtkirk commented on a change in pull request #550: HIVE-21198 Introduce a 
database object reference class
URL: https://github.com/apache/hive/pull/550#discussion_r260253836
 
 

 ##
 File path: 
hcatalog/core/src/main/java/org/apache/hive/hcatalog/cli/SemanticAnalysis/HCatSemanticAnalyzer.java
 ##
 @@ -348,8 +348,8 @@ protected void 
authorizeDDLWork(HiveSemanticAnalyzerHookContext cntxt, Hive hive
 
 AlterTableDesc alterTable = work.getAlterTblDesc();
 if (alterTable != null) {
-  Table table = hive.getTable(SessionState.get().getCurrentDatabase(),
-  Utilities.getDbTableName(alterTable.getOldName())[1], false);
+  final String tableName = 
Utilities.getTableName(alterTable.getOldName()).getTable();
+  Table table = hive.getTable(SessionState.get().getCurrentDatabase(), 
tableName, false);
 
 Review comment:
   `TableName` contains the database name; 
`SessionState.get().getCurrentDatabase()` should appear only at the time of 
creating the `TableName` - so as a sideeffect this "getTable()" will expect 
`TableName` argument


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] kgyrtkirk commented on a change in pull request #550: HIVE-21198 Introduce a database object reference class

[GitBox]

kgyrtkirk commented on a change in pull request #550: HIVE-21198 Introduce a 
database object reference class
URL: https://github.com/apache/hive/pull/550#discussion_r260254455
 
 

 ##
 File path: 
ql/src/java/org/apache/hadoop/hive/ql/exec/ColumnStatsUpdateTask.java
 ##
 @@ -86,8 +86,8 @@ private ColumnStatistics constructColumnStatsFromInput()
   colStats.getStatsDesc().getTableName());
   return colStats;
 }
-String dbName = work.dbName();
-String tableName = work.getTableName();
+final String dbName = work.dbName();
 
 Review comment:
   the "work" should have method(and corresponding field) `getTableName` 
instead of these 2 getters 


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] kgyrtkirk commented on a change in pull request #550: HIVE-21198 Introduce a database object reference class

[GitBox]

kgyrtkirk commented on a change in pull request #550: HIVE-21198 Introduce a 
database object reference class
URL: https://github.com/apache/hive/pull/550#discussion_r260254184
 
 

 ##
 File path: 
ql/src/java/org/apache/hadoop/hive/ql/cache/results/QueryResultsCache.java
 ##
 @@ -651,7 +652,7 @@ public void notifyTableChanged(String dbName, String 
tableName, long updateTime)
 List entriesToInvalidate = null;
 rwLock.writeLock().lock();
 try {
-  String key = (dbName.toLowerCase() + "." + tableName.toLowerCase());
+  String key = TableName.getDbTable(dbName, tableName).toLowerCase();
 
 Review comment:
   I would think that at this point we should already have a tablename from 
somewhere; constructing it here doesn't really make sense


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] kgyrtkirk commented on a change in pull request #550: HIVE-21198 Introduce a database object reference class

[GitBox]

kgyrtkirk commented on a change in pull request #550: HIVE-21198 Introduce a 
database object reference class
URL: https://github.com/apache/hive/pull/550#discussion_r260254184
 
 

 ##
 File path: 
ql/src/java/org/apache/hadoop/hive/ql/cache/results/QueryResultsCache.java
 ##
 @@ -651,7 +652,7 @@ public void notifyTableChanged(String dbName, String 
tableName, long updateTime)
 List entriesToInvalidate = null;
 rwLock.writeLock().lock();
 try {
-  String key = (dbName.toLowerCase() + "." + tableName.toLowerCase());
+  String key = TableName.getDbTable(dbName, tableName).toLowerCase();
 
 Review comment:
   I would think that at this point we should already have a tablename from 
somewhere; constructing it here doesn't really make sense


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] kgyrtkirk commented on a change in pull request #550: HIVE-21198 Introduce a database object reference class

[GitBox]

kgyrtkirk commented on a change in pull request #550: HIVE-21198 Introduce a 
database object reference class
URL: https://github.com/apache/hive/pull/550#discussion_r260256192
 
 

 ##
 File path: 
ql/src/java/org/apache/hadoop/hive/ql/exec/repl/incremental/IncrementalLoadTasksBuilder.java
 ##
 @@ -266,16 +271,15 @@ private boolean shouldReplayEvent(FileStatus dir, 
DumpType dumpType, String dbNa
 return updateReplIdTxnTask;
   }
 
-  private Task tableUpdateReplStateTask(String dbName, 
String tableName,
-Map 
partSpec, String replState,
-Task preCursor) throws SemanticException {
+  private Task tableUpdateReplStateTask(TableName 
tableName, Map partSpec,
 
 Review comment:
   now that I see this...I think that it would probably make sense to also 
include `partSpec` into this object
   (`PartishRef`: `catalog,database,tablename,partitonspec` ) ?


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] kgyrtkirk commented on a change in pull request #550: HIVE-21198 Introduce a database object reference class

[GitBox]

kgyrtkirk commented on a change in pull request #550: HIVE-21198 Introduce a 
database object reference class
URL: https://github.com/apache/hive/pull/550#discussion_r260253361
 
 

 ##
 File path: 
hcatalog/core/src/main/java/org/apache/hive/hcatalog/cli/SemanticAnalysis/HCatSemanticAnalyzer.java
 ##
 @@ -348,8 +348,8 @@ protected void 
authorizeDDLWork(HiveSemanticAnalyzerHookContext cntxt, Hive hive
 
 AlterTableDesc alterTable = work.getAlterTblDesc();
 if (alterTable != null) {
-  Table table = hive.getTable(SessionState.get().getCurrentDatabase(),
-  Utilities.getDbTableName(alterTable.getOldName())[1], false);
+  final String tableName = 
Utilities.getTableName(alterTable.getOldName()).getTable();
 
 Review comment:
   I guess we can't add `of` methods to `TableName`; but I feel like putting 
this into "Utilities" is kinda unfortunate.
   How about: `HiveTableName.of(...)`
   If you need any utilities you can probably make HiveTableName extend 
TableName - not sure if that will be needed or not


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] kgyrtkirk commented on a change in pull request #550: HIVE-21198 Introduce a database object reference class

[GitBox]

kgyrtkirk commented on a change in pull request #550: HIVE-21198 Introduce a 
database object reference class
URL: https://github.com/apache/hive/pull/550#discussion_r260254455
 
 

 ##
 File path: 
ql/src/java/org/apache/hadoop/hive/ql/exec/ColumnStatsUpdateTask.java
 ##
 @@ -86,8 +86,8 @@ private ColumnStatistics constructColumnStatsFromInput()
   colStats.getStatsDesc().getTableName());
   return colStats;
 }
-String dbName = work.dbName();
-String tableName = work.getTableName();
+final String dbName = work.dbName();
 
 Review comment:
   the "work" should have method(and corresponding field) `getTableName` 
instead of these 2 getters 


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services