[CARBONDATA-3216] Fix enableLocalDictionary with false issue in CSDK #3035
Conversation
1.enableLocalDictionary can' t set false
|
Build Success with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder2.1/2074/ |
|
Build Success with Spark 2.2.1, Please check CI http://95.216.28.178:8080/job/ApacheCarbonPRBuilder1/2279/ |
|
Build Success with Spark 2.3.2, Please check CI http://136.243.101.176:8080/job/carbondataprbuilder2.3/10328/ |
|
@KanakaKumar @ajantha-bhat @jackylk @ravipesala Please review it. |
store/CSDK/test/main.cpp
Outdated
| @@ -853,7 +854,7 @@ int main(int argc, char *argv[]) { | |||
| } else { | |||
| int batch = 32000; | |||
| int printNum = 32000; | |||
|
|
|||
| // | |||
There was a problem hiding this comment.
remove unwanted changes
| @@ -291,9 +291,6 @@ void CarbonWriter::localDictionaryThreshold(int localDictionaryThreshold) { | |||
| } | |||
|
|
|||
| void CarbonWriter::enableLocalDictionary(bool enableLocalDictionary) { | |||
| if (enableLocalDictionary == NULL) { | |||
There was a problem hiding this comment.
Can you give some description in the PR, how removing this solving the issue?
There was a problem hiding this comment.
@KanakaKumar : In CPP, boolean false value is same as NULL. Both are zero. Hence when we pass false, it always throws exception due to this wrong code. Hence he removed.
@xubo245 : This is just two removal, no need separate PR. you can do with #3048
| @@ -546,7 +546,7 @@ bool testWriteData(JNIEnv *env, char *path, int argc, char *argv[]) { | |||
| writer.withCsvInput(jsonSchema); | |||
| writer.withLoadOption("complex_delimiter_level_1", "#"); | |||
| writer.writtenBy("CSDK"); | |||
| writer.taskNo(185); | |||
| writer.taskNo(15541554.81); | |||
There was a problem hiding this comment.
This change not related to fix
There was a problem hiding this comment.
There are some different between CPP and java: java taskNo don't support double, but CPP will convert double to long.
| @@ -709,6 +709,7 @@ bool testWithTableProperty(JNIEnv *env, char *path, int argc, char **argv) { | |||
| writer.outputPath(path); | |||
| writer.withCsvInput(jsonSchema); | |||
| writer.withTableProperty("sort_columns", "shortField"); | |||
| writer.enableLocalDictionary(false); | |||
There was a problem hiding this comment.
Can you test if there is any possibility to give NULL in C and how it is sent to java layer ? true or false?
There was a problem hiding this comment.
add test code for it. In CPP, it will convert NULL to false when user invoke CarbonWriter::enableLocalDictionary.
|
Build Success with Spark 2.2.1, Please check CI http://95.216.28.178:8080/job/ApacheCarbonPRBuilder1/2359/ |
|
Build Success with Spark 2.3.2, Please check CI http://136.243.101.176:8080/job/carbondataprbuilder2.3/10402/ |
|
Build Success with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder2.1/2149/ |
|
@ajantha-bhat @KanakaKumar Please review it. |
|
LGTM |
xubo245
changed the title
1.enableLocalDictionary can' t set false
Be sure to do all of the following checklist to help us incorporate
your contribution quickly and easily:
No
NO
No
No
JIRA2951