You are viewing a plain text version of this content. The canonical link for it is here.
Posted to codereview@trafodion.apache.org by selvaganesang <gi...@git.apache.org> on 2018/11/15 01:51:37 UTC
[GitHub] trafodion pull request #1746: [TRAFODION-3234] Add support for hive partitio...
GitHub user selvaganesang opened a pull request:
https://github.com/apache/trafodion/pull/1746
[TRAFODION-3234] Add support for hive partitioned tables
Refactored the interaction with hive metastore to use the efficient APIs to obtain the necessary
information to construct the hive table descriptors. This refactoring is expected to reduce the
memory requirements, efficient and high performing even when there are thousands of partitions.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/selvaganesang/trafodion oom_in_hive_metastore
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/trafodion/pull/1746.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #1746
----
commit 923ca5738a24ab2fc60f6afda69eef6ad9a480cb
Author: selvaganesang <se...@...>
Date: 2018-11-13T16:42:37Z
[TRAFODION-3234] Add support for hive partitioned tables
Refactored the interaction with hive metastore to use the efficient APIs to obtain the necessary
information to construct the hive table descriptors. This refactoring is expected to reduce the
memory requirements, efficient and high performing even when there are thousands of partitions.
----
---
[GitHub] trafodion pull request #1746: [TRAFODION-3234] Add support for hive partitio...
Posted by DaveBirdsall <gi...@git.apache.org>.
Github user DaveBirdsall commented on a diff in the pull request:
https://github.com/apache/trafodion/pull/1746#discussion_r235211297
--- Diff: core/sql/sqlcomp/CmpDescribe.cpp ---
@@ -2355,10 +2355,134 @@ short CmpDescribeHiveTable (
NAString colString(buf);
Int32 j = i;
outputColumnLine(space, colString, j);
- }
+ }
outputShortLine(space, " )");
+ // show hive table partitions and buckets defination.
+ // this default schema name is what the Hive default schema is called in SeaHive
--- End diff --
What's SeaHive?
---
[GitHub] trafodion pull request #1746: [TRAFODION-3234] Refactor hive meta calls to b...
Posted by selvaganesang <gi...@git.apache.org>.
Github user selvaganesang commented on a diff in the pull request:
https://github.com/apache/trafodion/pull/1746#discussion_r237714202
--- Diff: core/sql/executor/HiveClient_JNI.cpp ---
@@ -68,6 +68,27 @@ char* HiveClient_JNI::getErrorText(HVC_RetCode errEnum)
return (char*)hvcErrorEnumStr[errEnum-HVC_FIRST-1];
}
+//////////////////////////////////////////////////////////////////////////////
+//
+//////////////////////////////////////////////////////////////////////////////
+HiveClient_JNI* HiveClient_JNI::newInstance(NAHeap *heap, HVC_RetCode &retCode)
+{
+ QRLogger::log(CAT_SQL_HDFS, LL_DEBUG, "HiveClient_JNI::newInstance() called.");
--- End diff --
Let me try to get rid of logging unless it makes sense
---
[GitHub] trafodion pull request #1746: [TRAFODION-3234] Refactor hive meta calls to b...
Posted by selvaganesang <gi...@git.apache.org>.
Github user selvaganesang commented on a diff in the pull request:
https://github.com/apache/trafodion/pull/1746#discussion_r237713928
--- Diff: core/sql/sqlcomp/CmpDescribe.cpp ---
@@ -2355,10 +2355,134 @@ short CmpDescribeHiveTable (
NAString colString(buf);
Int32 j = i;
outputColumnLine(space, colString, j);
- }
+ }
outputShortLine(space, " )");
+ // show hive table partitions and buckets defination.
--- End diff --
Fixed
---
[GitHub] trafodion pull request #1746: [TRAFODION-3234] Add support for hive partitio...
Posted by DaveBirdsall <gi...@git.apache.org>.
Github user DaveBirdsall commented on a diff in the pull request:
https://github.com/apache/trafodion/pull/1746#discussion_r235206424
--- Diff: core/sql/executor/HiveClient_JNI.cpp ---
@@ -42,17 +43,16 @@ static const char* const hvcErrorEnumStr[] =
,"Java exception in close()."
--- End diff --
Should there also be exception text for ,HVC_ERROR_INIT_EXCEPTION?
---
[GitHub] trafodion pull request #1746: [TRAFODION-3234] Refactor hive meta calls to b...
Posted by selvaganesang <gi...@git.apache.org>.
Github user selvaganesang commented on a diff in the pull request:
https://github.com/apache/trafodion/pull/1746#discussion_r237711771
--- Diff: core/sql/executor/HiveClient_JNI.cpp ---
@@ -42,17 +43,16 @@ static const char* const hvcErrorEnumStr[] =
,"Java exception in close()."
--- End diff --
Yes. Fixed it
---
[GitHub] trafodion pull request #1746: [TRAFODION-3234] Add support for hive partitio...
Posted by DaveBirdsall <gi...@git.apache.org>.
Github user DaveBirdsall commented on a diff in the pull request:
https://github.com/apache/trafodion/pull/1746#discussion_r235211551
--- Diff: core/sql/sqlcomp/CmpDescribe.cpp ---
@@ -2355,10 +2355,134 @@ short CmpDescribeHiveTable (
NAString colString(buf);
Int32 j = i;
outputColumnLine(space, colString, j);
- }
+ }
outputShortLine(space, " )");
+ // show hive table partitions and buckets defination.
+ // this default schema name is what the Hive default schema is called in SeaHive
+ HiveMetaData* md = bindWA.getSchemaDB()->getNATableDB()->getHiveMetaDB();
+ NAString defSchema = ActiveSchemaDB()->getDefaults().getValue(HIVE_DEFAULT_SCHEMA);
+ defSchema.toUpper();
+ struct hive_tbl_desc* htbl;
--- End diff --
This variable doesn't seem to be used anywhere. Can we delete it?
---
[GitHub] trafodion pull request #1746: [TRAFODION-3234] Add support for hive partitio...
Posted by DaveBirdsall <gi...@git.apache.org>.
Github user DaveBirdsall commented on a diff in the pull request:
https://github.com/apache/trafodion/pull/1746#discussion_r235211107
--- Diff: core/sql/sqlcomp/CmpDescribe.cpp ---
@@ -2355,10 +2355,134 @@ short CmpDescribeHiveTable (
NAString colString(buf);
Int32 j = i;
outputColumnLine(space, colString, j);
- }
+ }
outputShortLine(space, " )");
+ // show hive table partitions and buckets defination.
--- End diff --
Nit. Typo: defination (should be "definition")
---
[GitHub] trafodion pull request #1746: [TRAFODION-3234] Refactor hive meta calls to b...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/trafodion/pull/1746
---
[GitHub] trafodion pull request #1746: [TRAFODION-3234] Refactor hive meta calls to b...
Posted by selvaganesang <gi...@git.apache.org>.
Github user selvaganesang commented on a diff in the pull request:
https://github.com/apache/trafodion/pull/1746#discussion_r237713985
--- Diff: core/sql/sqlcomp/CmpDescribe.cpp ---
@@ -2355,10 +2355,134 @@ short CmpDescribeHiveTable (
NAString colString(buf);
Int32 j = i;
outputColumnLine(space, colString, j);
- }
+ }
outputShortLine(space, " )");
+ // show hive table partitions and buckets defination.
+ // this default schema name is what the Hive default schema is called in SeaHive
--- End diff --
Changed the comment line entirely
---
[GitHub] trafodion pull request #1746: [TRAFODION-3234] Refactor hive meta calls to b...
Posted by selvaganesang <gi...@git.apache.org>.
Github user selvaganesang commented on a diff in the pull request:
https://github.com/apache/trafodion/pull/1746#discussion_r237711864
--- Diff: core/sql/executor/HiveClient_JNI.cpp ---
@@ -42,17 +43,16 @@ static const char* const hvcErrorEnumStr[] =
,"Java exception in close()."
,"Preparing parameters for exists()."
,"Java exception in exists()."
- ,"Preparing parameters for getHiveTableStr()."
- ,"Java exception in getHiveTableStr()."
- ,"Preparing parameters for getHiveTableParameters()."
- ,"Java exception in getHiveTableParameters()."
,"Preparing parameters for getRedefTime()."
,"Java exception in getRedefTime()."
,"Java exception in getAllSchemas()."
,"Preparing parameters for getAllTables()."
,"Java exception in getAllTables()."
,"Preparing parameters for executeHiveSQL()."
,"Java exception in executeHiveSQL()."
+ ,"Preparing parameters for getHiveTableInfo()."
+ ,"Java exception in getHiveTableInfo()."
+ ,"Error in getHiveTableInfoDetails()."
--- End diff --
Yes. Corrected and ensured that it matches
---
[GitHub] trafodion pull request #1746: [TRAFODION-3234] Refactor hive meta calls to b...
Posted by selvaganesang <gi...@git.apache.org>.
Github user selvaganesang commented on a diff in the pull request:
https://github.com/apache/trafodion/pull/1746#discussion_r237714028
--- Diff: core/sql/sqlcomp/CmpDescribe.cpp ---
@@ -2355,10 +2355,134 @@ short CmpDescribeHiveTable (
NAString colString(buf);
Int32 j = i;
outputColumnLine(space, colString, j);
- }
+ }
outputShortLine(space, " )");
+ // show hive table partitions and buckets defination.
+ // this default schema name is what the Hive default schema is called in SeaHive
+ HiveMetaData* md = bindWA.getSchemaDB()->getNATableDB()->getHiveMetaDB();
+ NAString defSchema = ActiveSchemaDB()->getDefaults().getValue(HIVE_DEFAULT_SCHEMA);
+ defSchema.toUpper();
+ struct hive_tbl_desc* htbl;
--- End diff --
Deleted
---
[GitHub] trafodion pull request #1746: [TRAFODION-3234] Add support for hive partitio...
Posted by DaveBirdsall <gi...@git.apache.org>.
Github user DaveBirdsall commented on a diff in the pull request:
https://github.com/apache/trafodion/pull/1746#discussion_r235207369
--- Diff: core/sql/executor/HiveClient_JNI.cpp ---
@@ -68,6 +68,27 @@ char* HiveClient_JNI::getErrorText(HVC_RetCode errEnum)
return (char*)hvcErrorEnumStr[errEnum-HVC_FIRST-1];
}
+//////////////////////////////////////////////////////////////////////////////
+//
+//////////////////////////////////////////////////////////////////////////////
+HiveClient_JNI* HiveClient_JNI::newInstance(NAHeap *heap, HVC_RetCode &retCode)
+{
+ QRLogger::log(CAT_SQL_HDFS, LL_DEBUG, "HiveClient_JNI::newInstance() called.");
--- End diff --
Just curious: Why do we use CAT_SQL_HDFS in this QRLogger::log call, but elsewhere in this module we use CAT_SQL_HBASE? (Perhaps this use is correct and the others are all wrong?)
---
[GitHub] trafodion pull request #1746: [TRAFODION-3234] Add support for hive partitio...
Posted by DaveBirdsall <gi...@git.apache.org>.
Github user DaveBirdsall commented on a diff in the pull request:
https://github.com/apache/trafodion/pull/1746#discussion_r235206682
--- Diff: core/sql/executor/HiveClient_JNI.cpp ---
@@ -42,17 +43,16 @@ static const char* const hvcErrorEnumStr[] =
,"Java exception in close()."
,"Preparing parameters for exists()."
,"Java exception in exists()."
- ,"Preparing parameters for getHiveTableStr()."
- ,"Java exception in getHiveTableStr()."
- ,"Preparing parameters for getHiveTableParameters()."
- ,"Java exception in getHiveTableParameters()."
,"Preparing parameters for getRedefTime()."
,"Java exception in getRedefTime()."
,"Java exception in getAllSchemas()."
,"Preparing parameters for getAllTables()."
,"Java exception in getAllTables()."
,"Preparing parameters for executeHiveSQL()."
,"Java exception in executeHiveSQL()."
+ ,"Preparing parameters for getHiveTableInfo()."
+ ,"Java exception in getHiveTableInfo()."
+ ,"Error in getHiveTableInfoDetails()."
--- End diff --
Should there also be error text for HVC_ERROR_POPULATE_SDS_ERROR? More generally, the entries in this array of strings do not match in number those of the enum HVC_RetCode in HiveClient_JNI.h.
---
[GitHub] trafodion pull request #1746: [TRAFODION-3234] Add support for hive partitio...
Posted by DaveBirdsall <gi...@git.apache.org>.
Github user DaveBirdsall commented on a diff in the pull request:
https://github.com/apache/trafodion/pull/1746#discussion_r235210675
--- Diff: core/sql/optimizer/hiveHook.h ---
@@ -140,31 +145,42 @@ struct hive_sd_desc
char* nullFormat_;
NABoolean isCompressed_;
+ const char *partitionColValues_;
struct hive_sd_desc* next_;
- hive_sd_desc(Int32 sdID, const char* loc, Int64 creationTS, Int32 buckets,
+ hive_sd_desc(NAHeap *heap, Int32 sdID, const char* loc, Int64 creationTS, Int32 buckets,
const char* ift, const char* of,
const char* nf,
char knd,
struct hive_column_desc* column,
struct hive_skey_desc* skey,
struct hive_bkey_desc* bkey,
char fieldTerminator, char recordTerminator,
- const NABoolean isCompressed
+ const NABoolean isCompressed,
+ const char *pColVals
)
- : sdID_(sdID), buckets_(buckets), kind_(knd), column_(column),
- skey_(skey), bkey_(bkey),
- fieldTerminator_(fieldTerminator),
- recordTerminator_(recordTerminator),
- isCompressed_(isCompressed),
- next_(NULL)
- {
- location_ = strduph(loc, CmpCommon::contextHeap());
- inputFormat_ = strduph(ift, CmpCommon::contextHeap());
- outputFormat_= strduph(of, CmpCommon::contextHeap());
- nullFormat_ = (nf ? strduph(nf, CmpCommon::contextHeap()) : NULL);
- }
+
+ : heap_(heap), sdID_(sdID), creationTS_(creationTS),
+ buckets_(buckets), kind_(knd), column_(column),
+ skey_(skey), bkey_(bkey),
+ fieldTerminator_(fieldTerminator),
+ recordTerminator_(recordTerminator),
+ isCompressed_(isCompressed),
+ next_(NULL)
+ {
+ if (loc != NULL)
+ location_ = strduph(loc, heap_);
+ else
+ location_ = NULL;
+ inputFormat_ = strduph(ift, heap_);
+ outputFormat_= strduph(of, heap_);
+ nullFormat_ = (nf ? strduph(nf, heap_) : NULL);
+ if (pColVals)
+ partitionColValues_ = strduph(pColVals, heap_);
+ else
+ partitionColValues_ = NULL;
--- End diff --
The code is correct. It's interesting that the styles are mixed though: For nf, we use a ternary operator and for loc and pColVals we use if/then/else.
---