Do I understand correctly, that this fix has never been extended for the views?

Backported to branch-0.7

@Carl, it will great if you can do it. Thanks.

@Yongqiang: This needs to get backported to 0.7.0. Let me know if you want me to do it. Thanks.

Committed! Thanks Siying and Carl!

Passed HBase tests.

HBaseStorageHandler used to use table name as Hbase table name if it is not assigned. Now we have a choice to use dbName.tabName or tabName. To keep it kind of compatible, Now we use tabName if DB Name is "default" and use dbName.tabName if not.

I think that should be fine.

I tried some simple way to make it work. It doesn't seem to be such a simple task. Instead, I added this to block it:

<pre>
// TODO: add support to referencing views in foreign databases.
if (!tab.getDbName().equals(db.getCurrentDatabase()))

I'll add the view support as a follow-up patch.

Fine with me.

I'll figure out the view support now.

John, how about for this JIRA, we just drop support (I'll modify the codes to report error if the view is referenced as "db.view_name") for referencing foreign view and we do it as a follow-up?

Hey Siying,

This case from my original comment way at the top of this JIRA issue is not handled by your patch.

create database db1;
use db1;
create table t(i int);

create view v as select * from t;

create database db2;
use db2;
select * from db1.v;

This will take some work in the unparse when we store the view definition.

Resolve the conflict after rebasing. John asked me to try to reboot my machine before running the Hbase tests again. I'll try to do that too.

HBase tests are passing in Hudson. Siying and Yongqiang, can you guys try rebooting your dev boxes? There were issues with port conflicts with FB dev boxes before, so maybe you still have some lingering processes/ports.

Sorry for so many updates. I fixed lineage. The lineage info seems to have some issues with "table tablesample.." even now. I didn't changed this behavior and keep it as it is today.

Yongqiang verified that the HBase tests also fail on his machine.

I fixed multiple tests. Two tests always fail:

TestHBaseCliDriver
TestHBaseMinimrCliDriver

They fail even without the patch. So I guess it is unrelated.

Yongqiang, please notice that I updated a latest one that fixed a couple of contrib tests.

running tests, will commit after tests pass.

+1

For some reason, the tests are failing in my environment.
While I fix the problem, Yongqiang, can you commit ?

This patch fixed a couple of test outputs for TestContriCliTest.

not huge difference though. I fixed two test outputs but it doesn't seem to be related to Namit's test failures. Namit, can you do a "ant clean" and then "ant package" and then run the tests again?

fix test outputs of two new added tests after rebasing.

When I sync up new codes and rebase, some tests break. Will fix it.

I applied that patch to a clean directory and I am running the tests. It is still running but archive.q already passed. Maybe try to do a clean before test?

archive.q is the first test that fails - it works fine when run stand alone.
I havent debugged further

The code changes look good - but I am getting a lot of errors while running tests.
Can you run tests again ?

Sorry, looked at the code again - your locking changes are good - will review the remaining patch

Ideally, /<PREFIX>/db should be created as part of locking. You dont need to explicitly lock the db.

Namit, Driver.java just lock db of the table when locking any table, which should be the same reason as you explained "DROP DATABASE does not work while any table/partition is
being accessed/modified". Also, zookeeper path are something like /<PREFIX>/db/table_name. Without locking db in advance, parent path is missing and locking the tables will fail.

The changes look good - but I dont understand the need for Driver.java changes.
The concurerency should work

We need to lock the current database, so that DROP DATABASE does not work while any table/partition is
being accessed/modified

https://reviews.apache.org/r/413/diff/
for better browsing.

Update from HIVE.1517.3.patch:

Separate DB name and table name to different tokens and make respective changes.

Unfortunately, most syntax tree changes so that most test outputs are changed.

Looks like we have some trouble with printing token location in error message. new CommonToken(int, String) doesn't include location information there, so that the error message will always be "line 0:-1". Maybe we have to move from this approach to separate tokens for db and tab name.

https://reviews.apache.org/r/413/diff/
to better browse the patch.

Modify based on Carl's previous patch:
1. fix concurrency issue but locking DB for every table too
2. fix CREATE TABLE AS and CREATE TABLE LIKE. Also, CREATE TABLE AS to share the same codes to generate default path as CREATE TABLE
3. fix TABLESAMPLE, DROP TABLE, DROP TABLE, ALTER TABLE DROP PARTITION, etc.
4. fix the same table names in different databases in the same query. (PartitionPruner's key's problem)
5. fix character escaping problem.
6. fix TABLE ANALYZE to mistakely allow xxx.xxx
7. fix DynamicSerDe just to get unit test pass.
8. lots of test case results changes, since in DESCRIBE EXTENDED, table names are printed to be default.tab instead of tab. I found the possible way not to print "default" to make less change to test outputs is more risky. So I didn't try that. But I ever implemented it on my machine and make sure those test output modifications are right.

Some issues:
1. I didn't figure out the necessity to modify SemanticAnalyzer.processTable() not to use aliasIndex in Carl's patch, so I revert them.
2. DESCRIBE a foreign table is not supported and we don't give good error message.
3. `db.tab` is not blocked so far since I found it's a pretty complicated issue and might need more thinking.
4. UnparseTranslator becomes a little bit urgly now to unescape identifiers. I found it's really hard to keep Carl's syntax rules and still support it in a clean way. Maybe as a follow-up to make DB and table two different tokens, instead of parsing 'xxx.xxx' to semantic analyzer.

I'm still running test suites from the beginning to the end. There still can be something broken.

> `db.table` Should be considered ilegal. Is that right?

I think that's correct. See the following page for a complete discussion: http://dev.mysql.com/doc/refman/5.0/en/identifiers.html

It also looks like we need to tighten up the way the grammar handles Identifiers:

Identifier
    :
    (Letter | Digit) (Letter | Digit | '_')*
    | '`' RegexComponent+ '`'
    ;

RegexComponent
    : 'a'..'z' | 'A'..'Z' | '0'..'9' | '_'
    | PLUS | STAR | QUESTION | MINUS | DOT
    | LPAREN | RPAREN | LSQUARE | RSQUARE | LCURLY | RCURLY
    | BITWISEXOR | BITWISEOR | DOLLAR
    ;

Defining quoted identifiers in terms of RegexComponent permits a lot of illegal characters.
I think we actually want something like this:

Identifier
    : (Letter | Digit) (Letter | Digit | '_')*
    | '`' (Letter | Digit) (Letter | Digit | '_')* '`'
    ;

Double check about character escaping. For db.table, we want the correct escaping format to be `db`.`table`. `db.table` Should be considered ilegal. Is that right?

The concurrency issue seems to be that in the codes of acquiring locks, we always lock current database but when we locking every table or partition, we don't lock database of them, so that it breaks when we reference table from other databases. When I lock database for every table/partitions, the test passes.

Namit, do you think I should remove the codes to lock the current database? Is there a reason we always lock current database?

The syntax for DESCRIBE is broken. It should be:

DESCRIBE [EXTENDED] [database DOT]table [column]

but is actually

DESCRIBE [EXTENDED] table[DOT col_name]

Ref: http://dev.mysql.com/doc/refman/5.0/en/describe.html

One solution (which I'm not eager to see us take) is to extend Hive's non-standard as follows:

DESCRIBE [EXTENDED] [database] table[DOT col_name]

Anyway, I think it's OK to fix this as a followup.

Looks like ANALYZE TABLE doesn't need the table.xxx syntax. I'll change that. Then the only problem left is DESCRIBE.

Sorry, I mean DESCRIBE. DROP TABLE is fine.
You can do something like 'desc extended src.key;'

For ANALYZE TABLE, the syntax is similar to DESCRIBE, though I'm not sure how it is used.

Those are the two that I found you didn't replace Identification to tableName.

I'm not sure I understand the problem with DROP TABLE. How is it ambiguous?

As for ANALYZE TABLE, it looks like the definition of the tabTypeExpr
rule in the grammar is more complicated than it needs to be? Isn't this
just tableName from my patch?

I notice that Carl's patch added cross database support for 'create table', 'load', etc, but now 'drop table' and 'analyze table', which are very hard since they currently support dots as other meaning and would cause the ambiguity that we are not ready to handle. Also, it's hard for 'drop table' or 'analyze table' to give accurate error message if people try to use db.table. Do you think we want to keep it or we should remove all supports for DDL queries?

Quick note about the patch: the tests fail unless database.q is prepended with hive.support.concurrency = false. I didn't have time to workout the locking issues.

Attaching a rebased version of my other patch. This patch implements the ability to select across databases, and also updates a variety of DDL statements so that they are aware of canonical table names.

@Carl, are you working on this ?
We really need it asap. If you dont have time, can we take it over ?

Blocker for 0.7

We would like to use it right away

@Namit: Yes, I'm still working on this. I have everything working except for views. When do you need this done by?

@Carl, are you working on it ?

We might need it internally at facebook, so wanted to know if you are working on it currently.

@John: Yes, it's a checkpoint patch. Moving this to 0.7.0.

@Carl: this is just a checkpoint patch, right? Since this didn't make the Friday cut, can we move it to 0.7?

This case needs to work:

use db1;
create table t(...);

create view v as select * from t;

use db2;
select * from db1.v;

What this means is that when we store the definition of v, we need to store a fully-qualified db1.t reference. Otherwise, when we expand the view in the later query, we'll look for t in db2 (incorrect) rather than db1 (correct).