[CARBONDATA-276]add trim property for cols #200

lion-x · 2016-09-27T01:07:52Z

Why raise this PR?

Add trim property for cols.
trim a col or not can be set during create table time, like,

CREATE TABLE IF NOT EXISTS t3 (ID Int, date Timestamp, country String, name String, phonetype String, serialname String, salary Int)
STORED BY 'carbondata'
TBLPROPERTIES("trim"="country")

In order to delete some meaningless white space in some fields, we do this PR. It will also improve the index finding performance with shorter string.

How to test?

Pass all the test cases.

lion-x · 2016-09-27T01:24:11Z

CI passed!
http://136.243.101.176:8080/job/ApacheCarbonManualPRBuilder/498/

sujith71955 · 2016-09-30T05:43:55Z

hadoop/src/test/java/org/apache/carbondata/hadoop/test/util/StoreCreator.java

@@ -465,6 +466,7 @@ private static void generateGraph(IDataProcessStatus schmaModel, SchemaInfo info
    model.setEscapeCharacter(schmaModel.getEscapeCharacter());
    model.setQuoteCharacter(schmaModel.getQuoteCharacter());
    model.setCommentCharacter(schmaModel.getCommentCharacter());
+    model.setTrim(schmaModel.getTrim());


why we need to set this in schemamodel?

Not clear about the usecase, can you make it more clear by providing more details

In some cases, users want to trim some string field in csv, he set in Load DML option, and we need to handle to in csvinput step. So we the trim should be transformed to CSVinput step.

For Example, in some usecases, the LeadingWhiteSpace in a String, like " fsdfsd" maybe meaningful. Users want to keep the LeadingWhiteSpace. So we add this option for users, let them choose.

QiangCai · 2016-10-11T03:08:09Z

I think the dictionary generation is the same with the CSVInput, not ignore any white space.
It is unneccessary to trim data.

lion-x · 2016-10-11T09:18:19Z

Trimming data feature is used to support some users requirements. There is no causality between the dictionary generation is the same with the CSVInput and triming data or not.

sujith71955 · 2016-10-11T09:46:37Z

processing/src/main/java/org/apache/carbondata/processing/csvreaderstep/UnivocityCsvParser.java

@@ -102,8 +102,8 @@ public void initialize() throws IOException {
    parserSettings.setMaxColumns(
        getMaxColumnsForParsing(csvParserVo.getNumberOfColumns(), csvParserVo.getMaxColumns()));
    parserSettings.setNullValue("");
-    parserSettings.setIgnoreLeadingWhitespaces(false);
-    parserSettings.setIgnoreTrailingWhitespaces(false);
+    parserSettings.setIgnoreLeadingWhitespaces(csvParserVo.getTrim());


Same you need to handle in CarbonFilters.scala also, since while processing filter expressions the spaces are not getting trimmed, so here also you need to take care based on your feature

hi sujith
i am thinking if user already trim the data with the option setting,then when user query with some space filter,it would no getting result.

hi sujith,
I agree with Eason, when user query with some space filter, it should be considered as forbidden action.

Guys i think if while data loading we are reading from configuration inorder to trim it or not same we need to do while doing filter also, based on configuration value decide.
ex: while loading i enabled trim property, so system will trim and load the data, now in filter query also if user provides while space then it needs to be trimmed while creating filter model. This will provide more system consistentency. if user enable trim then we wont trim it while loading and also while querying.

Also one more point it will be better to set this property in column level while creating the table itself as its column properties , this will avoid user to provide this option every time while data loading

pros of this approach will be suppose in one load user loaded with dirty data and suddenly he realizes no i need to trim then in the next load he will enable the option and load the data, this will increase the dictionary space also, also in query dictionary lookup overhead will increase.

So better to set this while creating the table as column properties metadata

jackylk · 2016-10-09T14:51:41Z

...c/test/scala/org/apache/carbondata/spark/testsuite/dataload/TestDataLoadWithTrimOption.scala

+import org.apache.spark.sql.Row
+
+/**
+  * Created by x00381807 on 2016/9/26.


please remove

Oh, my fault

lion-x · 2016-10-27T11:56:19Z

@sujith71955 @jackylk pls review again. The code has been updated. The trim property for cols is set in Create.

jackylk · 2016-10-27T15:51:46Z

Can you provide more information about this PR, like what kind of query and why is it a bug?

lion-x · 2016-10-28T01:01:20Z

@jackylk there is no bug, I update my description.

sujith71955 · 2016-11-01T15:50:37Z

processing/src/main/java/org/apache/carbondata/processing/graphgenerator/GraphGenerator.java

+                                GraphConfigurationInfo graphConfig) {
+    List<Boolean> isUseTrimList = new ArrayList<Boolean>();
+    for (CarbonDimension dimension : dims) {
+      if (dimension.isUseTrim()) {


Can we add this trim option as property of the column, i mean inside a column property rather than setting directly as CarbonDimension property, Already in CarbonColumn there is a property map which holds column related properties, i think we can use that, please check the feasibility.

sujith71955 · 2016-11-01T15:52:26Z

...g/apache/carbondata/processing/surrogatekeysgenerator/csvbased/CarbonCSVBasedSeqGenMeta.java

+  public void setIsUseTrim(Boolean[] isUseTrim) {
+    for (Boolean flag: isUseTrim) {
+      if (flag) {
+        this.isUseTrim += "T";


Use TRUE/FALSE for better readability

sujith71955 · 2016-11-01T15:54:12Z

...g/apache/carbondata/processing/surrogatekeysgenerator/csvbased/CarbonCSVBasedSeqGenStep.java

@@ -472,6 +475,7 @@ public boolean processRow(StepMetaInterface smi, StepDataInterface sdi) throws K
              break;
          }
        }
+<<<<<<< HEAD


is this file is having any conflict?

bill1208 · 2016-11-01T18:17:56Z

I suggestion the trim should not be the properties of the table.
because in actually scenario the table designer is in front, then data of the this table is generated.
so the designer may don not consider which column need trim that may only can know after data generated.
So I suggestion this trim operation should be consider when data load.
during the data load user can set all column trim or not and also set trim for special column both for datalod command and insert command

sujith71955 · 2016-11-02T06:44:29Z

@Bill, i think while creating table the user shall know the entire schema which he is trying to generate and what kind of data/datatype is required for his table based on the expected data, if we provide the support in data load there are few drawbacks which we discussed in the prior discussion thread, please check.

sujith71955 · 2016-11-02T06:45:19Z

i will repeat the cons again if we set this property while data loading.
cons of this approach will be suppose in one load user loaded with dirty data and suddenly he realizes no i need to trim then in the next load he will enable the option and load the data, this will increase the dictionary space also, also in query dictionary lookup overhead will increase.

sujith71955 · 2016-11-02T06:45:53Z

just a thinking you can add your points :)

bill1208 · 2016-11-02T16:55:33Z

@sujith I got your point.
but I think trim data should belong to the step when process the data, which not belong to the table properties.
the scenario what's your des is exist even if specify it when create table.
For example when create the table they donot think it need trim the column1, but when actually data come or some business changed it need trim it. how to process this scenario? recreate the table or alter the table properties? :)

sujith71955 · 2016-11-04T05:05:01Z

@Bill , I agree to your valid points but i am thinking data load is a frequent operation compare to Create, and more over once you create table with the column properties specified, it will remain the same throughout the table life cycle, if we give in load command there is always a scope of inconsistency by keeping few loads with trimmed data and few without. i think user shall able to judge/decide if data should contain white spaces or it shall be trimmed for particular column if he knows his schema well .
And more over if the initial load the column data is having white space and as you told above he thinks to trim it, for that again user will perform data loading, but in this case his column count/distinct count and all will be different compare to the actual and also he has to see more query result with the data he is not interested, now if he delete the particular load also again he needs to reload the particular load data which will be again a cumbersome process. we shall try to make user to make his steps right and keep it simple.
You can put a view on my above points

lion-x · 2016-11-07T06:27:21Z

@sujith71955 @bill1208
actually, in my opinion, trimming data should belong to data preprocessing. If we must put trim property in carbon, I think it is better to put trim option in loading data part. If user put unprepocessing data into table, it is user's fault.

ravipesala · 2016-12-05T13:35:02Z

Can one of the admins verify this patch?

chenliang613 · 2016-12-11T03:13:06Z

@lion-x please rebase it.

remove exporter label at cleanup.sh

lion-x changed the title ~~addtrimoption~~ add trim option Sep 27, 2016

lion-x changed the title ~~add trim option~~ [CARBONDATA-276]add trim option Sep 27, 2016

sujith71955 reviewed Sep 30, 2016

View reviewed changes

sujith71955 reviewed Oct 11, 2016

View reviewed changes

jackylk reviewed Oct 12, 2016

View reviewed changes

lion-x changed the title ~~[CARBONDATA-276]add trim option~~ [CARBONDATA-276]add trim property for cols Oct 27, 2016

sujith71955 reviewed Nov 1, 2016

View reviewed changes

supportTrimForCols

f44b7dd

lion-x closed this Dec 12, 2016

lion-x deleted the addTrimOption branch December 13, 2016 06:59

asfbot mentioned this pull request May 27, 2017

[CARBONDATA-1099] Fixed bug for carbon-spark-shell in spark2 environment #964

Closed

Beyyes pushed a commit to Beyyes/carbondata that referenced this pull request Jul 12, 2018

Merge pull request apache#200 from Microsoft/yanjga/exporter-test

d5cffa5

remove exporter label at cleanup.sh

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[CARBONDATA-276]add trim property for cols #200

[CARBONDATA-276]add trim property for cols #200

lion-x commented Sep 27, 2016 •

edited

Loading

lion-x commented Sep 27, 2016 •

edited

Loading

sujith71955 Sep 30, 2016

sujith71955 Sep 30, 2016

lion-x Sep 30, 2016

QiangCai commented Oct 11, 2016

lion-x commented Oct 11, 2016

sujith71955 Oct 11, 2016

eason-lyx Oct 12, 2016

lion-x Oct 12, 2016

sujith71955 Oct 12, 2016

sujith71955 Oct 12, 2016

sujith71955 Oct 12, 2016

sujith71955 Oct 12, 2016

jackylk Oct 9, 2016

lion-x Oct 12, 2016

lion-x commented Oct 27, 2016

jackylk commented Oct 27, 2016

lion-x commented Oct 28, 2016

sujith71955 Nov 1, 2016

sujith71955 Nov 1, 2016

sujith71955 Nov 1, 2016

bill1208 commented Nov 1, 2016

sujith71955 commented Nov 2, 2016

sujith71955 commented Nov 2, 2016

sujith71955 commented Nov 2, 2016

bill1208 commented Nov 2, 2016

sujith71955 commented Nov 4, 2016

lion-x commented Nov 7, 2016

ravipesala commented Dec 5, 2016

chenliang613 commented Dec 11, 2016

[CARBONDATA-276]add trim property for cols #200

[CARBONDATA-276]add trim property for cols #200

Conversation

lion-x commented Sep 27, 2016 • edited Loading

Why raise this PR?

How to test?

lion-x commented Sep 27, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

QiangCai commented Oct 11, 2016

lion-x commented Oct 11, 2016

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

lion-x commented Oct 27, 2016

jackylk commented Oct 27, 2016

lion-x commented Oct 28, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bill1208 commented Nov 1, 2016

sujith71955 commented Nov 2, 2016

sujith71955 commented Nov 2, 2016

sujith71955 commented Nov 2, 2016

bill1208 commented Nov 2, 2016

sujith71955 commented Nov 4, 2016

lion-x commented Nov 7, 2016

ravipesala commented Dec 5, 2016

chenliang613 commented Dec 11, 2016

lion-x commented Sep 27, 2016 •

edited

Loading

lion-x commented Sep 27, 2016 •

edited

Loading