[CARBONDATA-3273] [CARBONDATA-3274] Fix for SORT_SCOPE in CarbonLoadDataCommand #3103

NamanRastogi · 2019-01-25T10:23:08Z

Problem1: With no SORT_COLUMNS, loading data was taking SORT_SCOPE=LOCAL_SORT instead of NO_SORT.
Solution: Added a check for SORT_COLUMNS in CarbonLoadDataCommand

Problem2: On table with some SORT_COLUMNS and SORT_SCOPE not specified, SORT_SCOPE was not considering CARBON.OPTIONS.SORT.SCOPE for SORT_SCOPE.
Solution: Added checking of CARBON.OPTIONS.SORT.SCOPE while loading.

Any interfaces changed? -> No
Any backward compatibility impacted? -> No
Document update required? -> No
Testing done -> Yes
For large changes, please consider breaking it into sub-tasks under an umbrella JIRA.

CarbonDataQA · 2019-01-25T10:34:43Z

Build Success with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder2.1/2441/

CarbonDataQA · 2019-01-25T11:34:30Z

Build Success with Spark 2.3.2, Please check CI http://136.243.101.176:8080/job/carbondataprbuilder2.3/10699/

CarbonDataQA · 2019-01-25T11:36:29Z

Build Success with Spark 2.2.1, Please check CI http://95.216.28.178:8080/job/ApacheCarbonPRBuilder1/2669/

qiuchenjian · 2019-01-25T11:39:32Z

...src/main/scala/org/apache/spark/sql/execution/command/management/CarbonLoadDataCommand.scala

-      // If there are Sort Columns given for the table and Sort Scope is not specified,
-      // we will take it as whichever sort scope given or LOCAL_SORT as default
+    if (tableProperties.get(CarbonCommonConstants.SORT_COLUMNS) == null ||
+        tableProperties.get(CarbonCommonConstants.SORT_COLUMNS).equals("")) {


Suggested change

tableProperties.get(CarbonCommonConstants.SORT_COLUMNS).equals("")) {

tableProperties.get(CarbonCommonConstants.SORT_COLUMNS).trim.equals("")) {

String with spaces will be caught before reaching this step. So no need to add trim.

use StringUtils.isBlank() to avoid retrieve themember twice

qiuchenjian · 2019-01-25T11:39:48Z

...src/main/scala/org/apache/spark/sql/execution/command/management/CarbonLoadDataCommand.scala

+      optionsFinal.put(CarbonCommonConstants.SORT_SCOPE, "NO_SORT")
+    }
+    else if (tableProperties.get(CarbonCommonConstants.SORT_SCOPE) == null ||
+             tableProperties.get(CarbonCommonConstants.SORT_SCOPE).equals("")) {


Suggested change

tableProperties.get(CarbonCommonConstants.SORT_SCOPE).equals("")) {

tableProperties.get(CarbonCommonConstants.SORT_SCOPE).trim.equals("")) {

String with spaces will be caught before reaching this step. So no need to add trim.

qiuchenjian · 2019-01-25T12:36:46Z

...src/main/scala/org/apache/spark/sql/execution/command/management/CarbonLoadDataCommand.scala

+          table.getDatabaseName + "." + table.getTableName,
+          carbonProperty.getProperty(CarbonLoadOptionConstants.CARBON_OPTIONS_SORT_SCOPE,
+            carbonProperty.getProperty(CarbonCommonConstants.LOAD_SORT_SCOPE,
+              "LOCAL_SORT"))))


why is LOAD_SORT_SCOPE_DEFAULT changed into "NO_SORT" in CarbonCommonConstants and this default value is also "LOCAL_SORT"
the ddl doc also describe that "NO_SORT" is the default value

If the SORT_COLUMNS are set and any SORT_SCOPE is not found, then LOCAL_SORT is used.
If no SORT_COLUMNS are set, then take the SORT_SCOPE as NO_SORT.

The default behaviour is NO_SORT if SORT_COLUMNS are not provided, and LOCAL_SORT if SORT_COLUMNS are provided explicetely in CREATE TABLE command. If nothing is specified in CREATE TABLE command, it takes SORT_SCOPE as NO_SORT and SORT_COLUMNS as none.

please avoid using the plain string here

About the behavior you mentioned above, is it exactly the doc trying to describe? If not, please optimize the doc in this PR to ensure the modifications' integrity.

besides, please check the comments of this method and ensure its correctness after your modification.

@xuchuanyin
Changed plain string to enum.
Updated the doc as well. It now includes the priority of Sort Scope checking.
Comments above are correct.

CarbonDataQA · 2019-01-25T12:52:24Z

Build Success with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder2.1/2445/

CarbonDataQA · 2019-01-25T13:12:39Z

Build Success with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder2.1/2448/

CarbonDataQA · 2019-01-25T14:02:58Z

Build Success with Spark 2.2.1, Please check CI http://95.216.28.178:8080/job/ApacheCarbonPRBuilder1/2675/

CarbonDataQA · 2019-01-25T14:22:32Z

Build Success with Spark 2.3.2, Please check CI http://136.243.101.176:8080/job/carbondataprbuilder2.3/10705/

xuchuanyin · 2019-01-27T10:30:46Z

...src/main/scala/org/apache/spark/sql/execution/command/management/CarbonLoadDataCommand.scala

-      // If there are Sort Columns given for the table and Sort Scope is not specified,
-      // we will take it as whichever sort scope given or LOCAL_SORT as default
+    if (tableProperties.get(CarbonCommonConstants.SORT_COLUMNS) == null ||
+        tableProperties.get(CarbonCommonConstants.SORT_COLUMNS).equals("")) {


use StringUtils.isBlank() to avoid retrieve themember twice

xuchuanyin · 2019-01-27T10:31:43Z

...src/main/scala/org/apache/spark/sql/execution/command/management/CarbonLoadDataCommand.scala

+    if (tableProperties.get(CarbonCommonConstants.SORT_COLUMNS) == null ||
+        tableProperties.get(CarbonCommonConstants.SORT_COLUMNS).equals("")) {
+      // If tableProperties.SORT_COLUMNS is null
+      optionsFinal.put(CarbonCommonConstants.SORT_SCOPE, "NO_SORT")


Instead of plain string 'NO_SORT', do we have Constant Variable or enum for this?

Done.
Used SortScopeOptions.SortScope.LOCAL_SORT.name for string "LOCAL_SORT"

xuchuanyin · 2019-01-27T10:32:17Z

...src/main/scala/org/apache/spark/sql/execution/command/management/CarbonLoadDataCommand.scala

+      // If tableProperties.SORT_COLUMNS is null
+      optionsFinal.put(CarbonCommonConstants.SORT_SCOPE, "NO_SORT")
+    }
+    else if (tableProperties.get(CarbonCommonConstants.SORT_SCOPE) == null ||


move to the previous line.
Please be careful with the code stype

xuchuanyin · 2019-01-27T10:33:55Z

...src/main/scala/org/apache/spark/sql/execution/command/management/CarbonLoadDataCommand.scala

+    }
+    else if (tableProperties.get(CarbonCommonConstants.SORT_SCOPE) == null ||
+             tableProperties.get(CarbonCommonConstants.SORT_SCOPE).equals("")) {
+      // If tableProperties.SORT_COLUMNS is not null


please optimize the comment here:
in case SORT_COLUMNS is not set and SORT_SCOPE is set

Comment

else if (StringUtils.isBlank(tableProperties.get(CarbonCommonConstants.SORT_SCOPE))) { // If tableProperties.SORT_COLUMNS is not null // and tableProperties.SORT_SCOPE is null optionsFinal.put(CarbonCommonConstants.SORT_SCOPE,

is already there.

By saying ‘set’ or 'not set', it refers to the business logic while ‘null’ or 'not null' does not refer to that logic directly.

xuchuanyin · 2019-01-27T10:35:27Z

...src/main/scala/org/apache/spark/sql/execution/command/management/CarbonLoadDataCommand.scala

+          table.getDatabaseName + "." + table.getTableName,
+          carbonProperty.getProperty(CarbonLoadOptionConstants.CARBON_OPTIONS_SORT_SCOPE,
+            carbonProperty.getProperty(CarbonCommonConstants.LOAD_SORT_SCOPE,
+              "LOCAL_SORT"))))


please avoid using the plain string here

xuchuanyin · 2019-01-27T10:39:35Z

...src/main/scala/org/apache/spark/sql/execution/command/management/CarbonLoadDataCommand.scala

+          table.getDatabaseName + "." + table.getTableName,
+          carbonProperty.getProperty(CarbonLoadOptionConstants.CARBON_OPTIONS_SORT_SCOPE,
+            carbonProperty.getProperty(CarbonCommonConstants.LOAD_SORT_SCOPE,
+              "LOCAL_SORT"))))


About the behavior you mentioned above, is it exactly the doc trying to describe? If not, please optimize the doc in this PR to ensure the modifications' integrity.

xuchuanyin · 2019-01-27T10:40:36Z

...src/main/scala/org/apache/spark/sql/execution/command/management/CarbonLoadDataCommand.scala

+          table.getDatabaseName + "." + table.getTableName,
+          carbonProperty.getProperty(CarbonLoadOptionConstants.CARBON_OPTIONS_SORT_SCOPE,
+            carbonProperty.getProperty(CarbonCommonConstants.LOAD_SORT_SCOPE,
+              "LOCAL_SORT"))))


besides, please check the comments of this method and ensure its correctness after your modification.

NamanRastogi · 2019-01-28T06:45:23Z

@xuchuanyin I have updated the Doc as well. Added Priority for Sort_Scope checking.

CarbonDataQA · 2019-01-28T06:54:32Z

Build Success with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder2.1/2472/

CarbonDataQA · 2019-01-28T08:03:23Z

Build Success with Spark 2.3.2, Please check CI http://136.243.101.176:8080/job/carbondataprbuilder2.3/10730/

CarbonDataQA · 2019-01-28T08:19:15Z

Build Success with Spark 2.2.1, Please check CI http://95.216.28.178:8080/job/ApacheCarbonPRBuilder1/2700/

kumarvishal09 · 2019-01-28T10:53:20Z

retest this please

CarbonDataQA · 2019-01-28T11:05:13Z

Build Success with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder2.1/2479/

CarbonDataQA · 2019-01-28T12:16:40Z

Build Success with Spark 2.3.2, Please check CI http://136.243.101.176:8080/job/carbondataprbuilder2.3/10737/

CarbonDataQA · 2019-01-28T12:30:53Z

Build Success with Spark 2.2.1, Please check CI http://95.216.28.178:8080/job/ApacheCarbonPRBuilder1/2708/

ravipesala · 2019-01-28T14:20:46Z

...src/main/scala/org/apache/spark/sql/execution/command/management/CarbonLoadDataCommand.scala

+      // If tableProperties.SORT_COLUMNS is not null
+      // and tableProperties.SORT_SCOPE is null
+      optionsFinal.put(CarbonCommonConstants.SORT_SCOPE,
+        carbonProperty.getProperty(CarbonLoadOptionConstants.CARBON_TABLE_LOAD_SORT_SCOPE +


I think the first priority should be load options. Please check it.

1. With SORT_COLUMNS=null, loading data was resulting in SORT_SCOPE=LOCAL_SORT. 2. With SORT_COLUMNS!=null and SORT_SCOPE not provided, loading data was not checking CARBON.OPTIONS.SORT.SCOPE.

ravipesala · 2019-01-28T14:47:19Z

LGTM

CarbonDataQA · 2019-01-28T16:17:46Z

Build Success with Spark 2.3.2, Please check CI http://136.243.101.176:8080/job/carbondataprbuilder2.3/10746/

CarbonDataQA · 2019-01-28T16:39:27Z

Build Failed with Spark 2.2.1, Please check CI http://95.216.28.178:8080/job/ApacheCarbonPRBuilder1/2718/

CarbonDataQA · 2019-01-28T17:05:38Z

Build Success with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder2.1/2490/

xuchuanyin · 2019-01-29T00:50:48Z

retest this please

CarbonDataQA · 2019-01-29T01:00:49Z

Build Success with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder2.1/2494/

CarbonDataQA · 2019-01-29T01:59:45Z

Build Success with Spark 2.3.2, Please check CI http://136.243.101.176:8080/job/carbondataprbuilder2.3/10752/

CarbonDataQA · 2019-01-29T02:01:20Z

Build Success with Spark 2.2.1, Please check CI http://95.216.28.178:8080/job/ApacheCarbonPRBuilder1/2723/

…ataCommand Problem1: With no SORT_COLUMNS, loading data was taking SORT_SCOPE=LOCAL_SORT instead of NO_SORT. Solution: Added a check for SORT_COLUMNS in CarbonLoadDataCommand Problem2: On table with some SORT_COLUMNS and SORT_SCOPE not specified, SORT_SCOPE was not considering CARBON.OPTIONS.SORT.SCOPE for SORT_SCOPE. Solution: Added checking of CARBON.OPTIONS.SORT.SCOPE while loading. This closes #3103

…ataCommand Problem1: With no SORT_COLUMNS, loading data was taking SORT_SCOPE=LOCAL_SORT instead of NO_SORT. Solution: Added a check for SORT_COLUMNS in CarbonLoadDataCommand Problem2: On table with some SORT_COLUMNS and SORT_SCOPE not specified, SORT_SCOPE was not considering CARBON.OPTIONS.SORT.SCOPE for SORT_SCOPE. Solution: Added checking of CARBON.OPTIONS.SORT.SCOPE while loading. This closes apache#3103

qiuchenjian reviewed Jan 25, 2019

View reviewed changes

NamanRastogi force-pushed the load_sort_scope_fix branch from 37488c0 to 74ff915 Compare January 25, 2019 12:10

NamanRastogi changed the title ~~[CARBONDATA-3273] Fix SORT_SCOPE with no SORT_COLUMNS~~ [CARBONDATA-3273] Fix for SORT_SCOPE in CarbonLoadDataCommand Jan 25, 2019

NamanRastogi changed the title ~~[CARBONDATA-3273] Fix for SORT_SCOPE in CarbonLoadDataCommand~~ [CARBONDATA-3273] [CARBONDATA-3274] Fix for SORT_SCOPE in CarbonLoadDataCommand Jan 25, 2019

NamanRastogi force-pushed the load_sort_scope_fix branch 2 times, most recently from a36a71c to 8b73763 Compare January 25, 2019 12:26

qiuchenjian reviewed Jan 25, 2019

View reviewed changes

xuchuanyin reviewed Jan 27, 2019

View reviewed changes

NamanRastogi force-pushed the load_sort_scope_fix branch from 8b73763 to a33121e Compare January 28, 2019 06:41

ravipesala reviewed Jan 28, 2019

View reviewed changes

Fix SORT_SCOPE in CarbonLoadDataCommand

eaa42f7

1. With SORT_COLUMNS=null, loading data was resulting in SORT_SCOPE=LOCAL_SORT. 2. With SORT_COLUMNS!=null and SORT_SCOPE not provided, loading data was not checking CARBON.OPTIONS.SORT.SCOPE.

NamanRastogi force-pushed the load_sort_scope_fix branch from a33121e to eaa42f7 Compare January 28, 2019 14:28

asfgit closed this in b82cd1b Jan 29, 2019

	tableProperties.get(CarbonCommonConstants.SORT_COLUMNS).equals("")) {
	tableProperties.get(CarbonCommonConstants.SORT_COLUMNS).trim.equals("")) {

[CARBONDATA-3273] [CARBONDATA-3274] Fix for SORT_SCOPE in CarbonLoadDataCommand #3103

[CARBONDATA-3273] [CARBONDATA-3274] Fix for SORT_SCOPE in CarbonLoadDataCommand #3103

Conversation

NamanRastogi commented Jan 25, 2019 • edited

CarbonDataQA commented Jan 25, 2019

CarbonDataQA commented Jan 25, 2019

CarbonDataQA commented Jan 25, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NamanRastogi Jan 25, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CarbonDataQA commented Jan 25, 2019

CarbonDataQA commented Jan 25, 2019

CarbonDataQA commented Jan 25, 2019

CarbonDataQA commented Jan 25, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NamanRastogi commented Jan 28, 2019 • edited

CarbonDataQA commented Jan 28, 2019

CarbonDataQA commented Jan 28, 2019

CarbonDataQA commented Jan 28, 2019

kumarvishal09 commented Jan 28, 2019

CarbonDataQA commented Jan 28, 2019

CarbonDataQA commented Jan 28, 2019

CarbonDataQA commented Jan 28, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ravipesala commented Jan 28, 2019

CarbonDataQA commented Jan 28, 2019

CarbonDataQA commented Jan 28, 2019

CarbonDataQA commented Jan 28, 2019

xuchuanyin commented Jan 29, 2019

CarbonDataQA commented Jan 29, 2019

CarbonDataQA commented Jan 29, 2019

CarbonDataQA commented Jan 29, 2019

NamanRastogi commented Jan 25, 2019 •

edited

NamanRastogi Jan 25, 2019 •

edited

NamanRastogi commented Jan 28, 2019 •

edited