You are viewing a plain text version of this content. The canonical link for it is here.
Posted to gitbox@hive.apache.org by GitBox <gi...@apache.org> on 2020/01/13 14:38:33 UTC
[GitHub] [hive] dlavati opened a new pull request #876: HIVE-22585: Clean up
catalog/db/table name usage
dlavati opened a new pull request #876: HIVE-22585: Clean up catalog/db/table name usage
URL: https://github.com/apache/hive/pull/876
Ugly things are yet to come. Will also do some method signature changes later.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] kgyrtkirk commented on a change in pull request #876:
HIVE-22585: Clean up catalog/db/table name usage
Posted by GitBox <gi...@apache.org>.
kgyrtkirk commented on a change in pull request #876: HIVE-22585: Clean up catalog/db/table name usage
URL: https://github.com/apache/hive/pull/876#discussion_r381992821
##########
File path: ql/src/java/org/apache/hadoop/hive/ql/cache/results/QueryResultsCache.java
##########
@@ -631,13 +632,13 @@ public long getSize() {
}
}
- public void notifyTableChanged(String dbName, String tableName, long updateTime) {
- LOG.debug("Table changed: {}.{}, at {}", dbName, tableName, updateTime);
+ public void notifyTableChanged(TableName tableName, long updateTime) {
+ LOG.debug("Table changed: {}, at {}", tableName.getNotEmptyDbTable(), updateTime);
// Invalidate all cache entries using this table.
List<CacheEntry> entriesToInvalidate = null;
rwLock.writeLock().lock();
try {
- String key = (dbName.toLowerCase() + "." + tableName.toLowerCase());
+ String key = (tableName.getNotEmptyDbTable().toLowerCase());
Review comment:
we might want to consider to remove all these "toLowerCase" calls;and instead make it a contract for tablenames; so it's enforced at the time the tablename is created
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] kgyrtkirk commented on a change in pull request #876:
HIVE-22585: Clean up catalog/db/table name usage
Posted by GitBox <gi...@apache.org>.
kgyrtkirk commented on a change in pull request #876: HIVE-22585: Clean up catalog/db/table name usage
URL: https://github.com/apache/hive/pull/876#discussion_r381994244
##########
File path: ql/src/java/org/apache/hadoop/hive/ql/ddl/table/misc/AlterTableRenameDesc.java
##########
@@ -33,16 +33,16 @@
public class AlterTableRenameDesc extends AbstractAlterTableDesc {
private static final long serialVersionUID = 1L;
- private final String newName;
+ private final TableName newName;
Review comment:
nit: newName -> newTableName
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] kgyrtkirk commented on a change in pull request #876:
HIVE-22585: Clean up catalog/db/table name usage
Posted by GitBox <gi...@apache.org>.
kgyrtkirk commented on a change in pull request #876: HIVE-22585: Clean up catalog/db/table name usage
URL: https://github.com/apache/hive/pull/876#discussion_r381996213
##########
File path: ql/src/test/results/clientnegative/create_external_transactional.q.out
##########
@@ -1 +1 @@
-FAILED: SemanticException transactional_external cannot be declared transactional because it's an external table
+FAILED: SemanticException default.transactional_external cannot be declared transactional because it's an external table
Review comment:
:+1:
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] dlavati commented on a change in pull request #876:
HIVE-22585: Clean up catalog/db/table name usage
Posted by GitBox <gi...@apache.org>.
dlavati commented on a change in pull request #876: HIVE-22585: Clean up catalog/db/table name usage
URL: https://github.com/apache/hive/pull/876#discussion_r382020206
##########
File path: ql/src/java/org/apache/hadoop/hive/ql/parse/repl/dump/TableExport.java
##########
@@ -152,7 +152,7 @@ private void writeData(PartitionIterable partitions) throws SemanticException {
if (tableSpec.tableHandle.isPartitioned()) {
if (partitions == null) {
throw new IllegalStateException("partitions cannot be null for partitionTable :"
- + tableSpec.getTableName().getTable());
+ + tableSpec.getTableName().getNotEmptyDbTable());
Review comment:
I guess using the logic of `getNotEmptyDbTable` for `toString` would make the most sense then.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] kgyrtkirk commented on a change in pull request #876:
HIVE-22585: Clean up catalog/db/table name usage
Posted by GitBox <gi...@apache.org>.
kgyrtkirk commented on a change in pull request #876: HIVE-22585: Clean up catalog/db/table name usage
URL: https://github.com/apache/hive/pull/876#discussion_r381997220
##########
File path: ql/src/test/results/clientpositive/alter_rename_table.q.out
##########
@@ -131,7 +131,7 @@ STAGE PLANS:
Stage: Stage-0
Rename Table
table name: source.src
Review comment:
the old tablename doesn't qualified hat the "category"
we should be consistent in showing or not showing it category
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] kgyrtkirk commented on a change in pull request #876:
HIVE-22585: Clean up catalog/db/table name usage
Posted by GitBox <gi...@apache.org>.
kgyrtkirk commented on a change in pull request #876: HIVE-22585: Clean up catalog/db/table name usage
URL: https://github.com/apache/hive/pull/876#discussion_r381998918
##########
File path: ql/src/java/org/apache/hadoop/hive/ql/parse/HiveTableName.java
##########
@@ -38,37 +38,22 @@ public HiveTableName(String catName, String dbName, String tableName) {
* @throws SemanticException
*/
public static TableName of(Table table) throws SemanticException {
- return ofNullable(table.getTableName(), table.getDbName());
+ return ofNullable(table.getTableName(), table.getDbName()); // todo: this shouldn't call nullable
}
/**
- * Set a @{@link Table} object's table and db names based on the provided string.
- * @param dbTable the dbtable string
+ * Set a @{@link Table} object's table and db names based on the provided tableName object.
+ * @param tableName the tableName object
* @param table the table to update
* @return the table
* @throws SemanticException
*/
- public static Table setFrom(String dbTable, Table table) throws SemanticException{
- TableName name = ofNullable(dbTable);
- table.setTableName(name.getTable());
- table.setDbName(name.getDb());
+ public static Table setFrom(TableName tableName, Table table) throws SemanticException{
Review comment:
this would be better served as an instance method - I guess it can't be added to Table...
how abut something like `tableName.writeInto(table)`
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] kgyrtkirk commented on a change in pull request #876:
HIVE-22585: Clean up catalog/db/table name usage
Posted by GitBox <gi...@apache.org>.
kgyrtkirk commented on a change in pull request #876: HIVE-22585: Clean up catalog/db/table name usage
URL: https://github.com/apache/hive/pull/876#discussion_r381993869
##########
File path: ql/src/java/org/apache/hadoop/hive/ql/cache/results/QueryResultsCache.java
##########
@@ -989,7 +990,7 @@ public void accept(NotificationEvent event) {
QueryResultsCache cache = QueryResultsCache.getInstance();
if (cache != null) {
long eventTime = event.getEventTime() * 1000L;
- cache.notifyTableChanged(dbName, tableName, eventTime);
+ cache.notifyTableChanged(TableName.fromString(tableName, null, dbName), eventTime);
Review comment:
I wonder if it would make sense to leave out the category for now....it's not really used - instead of passing null everywhere we could have a separate method for (db,name)
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] dlavati commented on a change in pull request #876:
HIVE-22585: Clean up catalog/db/table name usage
Posted by GitBox <gi...@apache.org>.
dlavati commented on a change in pull request #876: HIVE-22585: Clean up catalog/db/table name usage
URL: https://github.com/apache/hive/pull/876#discussion_r382026320
##########
File path: ql/src/java/org/apache/hadoop/hive/ql/parse/HiveTableName.java
##########
@@ -38,37 +38,22 @@ public HiveTableName(String catName, String dbName, String tableName) {
* @throws SemanticException
*/
public static TableName of(Table table) throws SemanticException {
- return ofNullable(table.getTableName(), table.getDbName());
+ return ofNullable(table.getTableName(), table.getDbName()); // todo: this shouldn't call nullable
}
/**
- * Set a @{@link Table} object's table and db names based on the provided string.
- * @param dbTable the dbtable string
+ * Set a @{@link Table} object's table and db names based on the provided tableName object.
+ * @param tableName the tableName object
* @param table the table to update
* @return the table
* @throws SemanticException
*/
- public static Table setFrom(String dbTable, Table table) throws SemanticException{
- TableName name = ofNullable(dbTable);
- table.setTableName(name.getTable());
- table.setDbName(name.getDb());
+ public static Table setFrom(TableName tableName, Table table) throws SemanticException{
Review comment:
I went with this because Table is in ql.metadata, while TableName is in storage-api.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] kgyrtkirk commented on a change in pull request #876:
HIVE-22585: Clean up catalog/db/table name usage
Posted by GitBox <gi...@apache.org>.
kgyrtkirk commented on a change in pull request #876: HIVE-22585: Clean up catalog/db/table name usage
URL: https://github.com/apache/hive/pull/876#discussion_r381995705
##########
File path: ql/src/java/org/apache/hadoop/hive/ql/parse/repl/dump/TableExport.java
##########
@@ -152,7 +152,7 @@ private void writeData(PartitionIterable partitions) throws SemanticException {
if (tableSpec.tableHandle.isPartitioned()) {
if (partitions == null) {
throw new IllegalStateException("partitions cannot be null for partitionTable :"
- + tableSpec.getTableName().getTable());
+ + tableSpec.getTableName().getNotEmptyDbTable());
Review comment:
would it make a lot of changes if we would rely on TableName's toString() method for cases like this....I don't think we should retain the old exception messages at any cost....
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org