-
Notifications
You must be signed in to change notification settings - Fork 4.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
HIVE-22010 - Clean up ShowCreateTableOperation #732
Conversation
@@ -186,6 +186,8 @@ static void internalBeforeClassSetup(Map<String, String> additionalProperties, b | |||
}); | |||
|
|||
MetaStoreTestUtils.startMetaStoreWithRetry(hconf); | |||
// re set the WAREHOUSE property to the test dir, as the previous command added a random port to it | |||
hconf.set(MetastoreConf.ConfVars.WAREHOUSE.getVarname(), System.getProperty("test.warehouse.dir", "/tmp")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume we expect the property to exist hence /tmp
would not make a difference?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes it wouldn't in theory, still the class already references this property 4 times before this change, twice specifying /tmp as fallback value, twice not. I thought that we should be consistent, and it can't hurt to have it there so I put it here, and also fixed the missing one in line 3273 - but missed to do so in the 4th occasion at line 3293. So we can have it at all occasions,, or remove it from all, just be consistent. What do you suggest?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consistency is good idd... We can include tmp
it in all occurrences.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree, fixed.
@@ -50,20 +53,26 @@ | |||
import org.apache.hive.common.util.HiveStringUtils; | |||
import org.stringtemplate.v4.ST; | |||
|
|||
import com.google.common.collect.ImmutableSet; | |||
|
|||
import avro.shaded.com.google.common.collect.Sets; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably you did not want to use the shaded guava here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks fixed.
createTabCommand.append("CREATE <" + TEMPORARY + "><" + EXTERNAL + ">TABLE `"); | ||
createTabCommand.append(desc.getTableName() + "`(\n"); | ||
createTabCommand.append("<" + LIST_COLUMNS + ">)\n"); | ||
createTabCommand.append("<" + TBL_COMMENT + ">\n"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems we have removed some of the information we were printing, e.g., TBL_COMMENT
or TBL_LOCATION
. Is this because they will be included in the table properties?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They are still there, just removed the unnecessary TBL_ prefix - they are all for the table :)
e1e4ec9
to
a60679c
Compare
a60679c
to
2a09c8d
Compare
Rebased after recent chnages + fixed checkstyle issue. |
Also fixed small checkstyle issues and removed unused codes from some other ddl files.