Skip to content

HIVE-21818#663

Closed
jcamachor wants to merge 2 commits intoapache:masterfrom
jcamachor:HIVE-21818
Closed

HIVE-21818#663
jcamachor wants to merge 2 commits intoapache:masterfrom
jcamachor:HIVE-21818

Conversation

@jcamachor
Copy link
Contributor

No description provided.

@@ -2105,7 +2105,13 @@ private void getMetaData(QB qb, ReadEntity parentInput)
Table tab = getTableObjectByName(tabName, false);
if (tab != null) {
// do a deep copy, in case downstream changes it.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we update the comment since this is no longer deep copy?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This has not changed, it is still creating a deep copy of the metastore table.

...tab.getTTable().deepCopy()...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The constraints are not deep copied though, you are right. Those should not change in any case; I will change the comment accordingly.


@Override
protected Table getTableObjectByName(String tableName, boolean throwException) throws HiveException {
if (!tabNameToTabObject.containsKey(tableName)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens if although tableName is same but they are different tables belonging to different database?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the qualified table name (see L1226 in Hive.java).

private Table getTable(String tableName) {
if (!tablesCache.containsKey(tableName)) {
try {
Table table = db.getTable(tableName);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Null check for db? HiveMaterializedViewsRegistry.java is passing NULL for db.

Copy link
Contributor Author

@jcamachor jcamachor Jun 6, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You do not need the null check; this will never be called for materialized views, since currently you cannot define constraints on them. Not sure if you mean that?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

L:393 in HiveMaterializedViewsRegistry creates new RelOptHiveTable by passing NULL for db. getTable here is using db to call getTable. I meant it should be checked if db is null or not before calling getTable on it

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefer that it fails instead of ignoring the retrieval silently, as it should never be null unless you know what you are doing, i.e. materialized views. I can add a comment in the table creation in HiveMaterializedViewsRegistry.

@asfgit asfgit closed this in de9a678 Jun 7, 2019
b-slim pushed a commit to b-slim/hive that referenced this pull request Jul 23, 2019
… traffic (Jesus Camacho Rodriguez, reviewed by Vineet Garg)

Close apache#663

Change-Id: Icc15e8944e6110d8d40d2816ee451a2e6a980c03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

Comments