Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[CARBONDATA-2359] Support applicable load options and table properties for Non-Transactional table #2190

Closed
wants to merge 1 commit into from
Closed

[CARBONDATA-2359] Support applicable load options and table properties for Non-Transactional table #2190

wants to merge 1 commit into from

Conversation

ajantha-bhat
Copy link
Member

[CARBONDATA-2359] Support applicable load options and table properties for a Non-Transactional table
And blocked clean files for a Non-Transactional table.
your contribution quickly and easily:

  • Any interfaces changed? No. Added new interfaces. Didn't modified any.

  • Any backward compatibility impacted? No

  • Document update required? yes, will be handled in separate PR

  • Testing done.
    please refer TestUnmanagedCarbonTable.scala

  • For large changes, please consider breaking it into sub-tasks under an umbrella JIRA. Done.

@ajantha-bhat
Copy link
Member Author

Retest this please

@CarbonDataQA
Copy link

Build Failed with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder1/5214/

@CarbonDataQA
Copy link

Build Failed with Spark 2.2.1, Please check CI http://88.99.58.216:8080/job/ApacheCarbonPRBuilder/4033/

@ajantha-bhat ajantha-bhat force-pushed the unmanaged_table branch 2 times, most recently from 71d9e66 to bf9ee9f Compare April 20, 2018 06:24
@ajantha-bhat
Copy link
Member Author

Retest this please

@CarbonDataQA
Copy link

Build Failed with Spark 2.2.1, Please check CI http://88.99.58.216:8080/job/ApacheCarbonPRBuilder/4043/

@ajantha-bhat
Copy link
Member Author

Retest this please

@CarbonDataQA
Copy link

Build Failed with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder1/5232/

@CarbonDataQA
Copy link

Build Failed with Spark 2.2.1, Please check CI http://88.99.58.216:8080/job/ApacheCarbonPRBuilder/4055/

@ajantha-bhat
Copy link
Member Author

Retest this please

@CarbonDataQA
Copy link

Build Failed with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder1/5242/

@CarbonDataQA
Copy link

Build Failed with Spark 2.2.1, Please check CI http://88.99.58.216:8080/job/ApacheCarbonPRBuilder/4068/

@ajantha-bhat
Copy link
Member Author

Retest this Please

@CarbonDataQA
Copy link

Build Failed with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder1/5255/

@CarbonDataQA
Copy link

Build Success with Spark 2.2.1, Please check CI http://88.99.58.216:8080/job/ApacheCarbonPRBuilder/4074/

@ajantha-bhat
Copy link
Member Author

Retest this please

@CarbonDataQA
Copy link

Build Success with Spark 2.2.1, Please check CI http://88.99.58.216:8080/job/ApacheCarbonPRBuilder/4076/

@CarbonDataQA
Copy link

Build Success with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder1/5257/

gvramana
gvramana reviewed Apr 22, 2018
View reviewed changes
core/src/main/java/org/apache/carbondata/core/scan/executor/util/RestructureUtil.java Outdated
* @return measures present in the block
*/
public static List<ProjectionMeasure> createMeasureInfoAndGetCurrentBlockQueryMeasures(
BlockExecutionInfo blockExecutionInfo, List<ProjectionMeasure> queryMeasures,
List<CarbonMeasure> currentBlockMeasures, boolean isUnManagedTable) {
List<CarbonMeasure> currentBlockMeasures) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For non transactional table columnid check should not be persent during read. As files can be created some where else and can be copied.

gvramana
gvramana reviewed Apr 22, 2018
View reviewed changes
store/sdk/src/main/java/org/apache/carbondata/sdk/file/CarbonWriterBuilder.java Outdated
tableSchemaBuilder = tableSchemaBuilder.isUnmanagedTable(isUnManagedTable);
}

List<String> sortColumnsList = new ArrayList<>();
Copy link
Contributor

@gvramana gvramana Apr 22, 2018 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sort options behaviour should be consistent as create table
If empty sort columns list, then it should be no sort.
If sort column list not passed, default behaviour should be same as create table.
Also mention this behaviour in SDK java doc

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok. done

gvramana
gvramana reviewed Apr 22, 2018
View reviewed changes
core/src/main/java/org/apache/carbondata/core/metadata/schema/table/TableSchemaBuilder.java Outdated
@@ -119,7 +126,18 @@ public TableSchemaBuilder addColumn(StructField field, boolean isSortColumn) {
}
newColumn.setSchemaOrdinal(ordinal++);
newColumn.setColumnar(true);
newColumn.setColumnUniqueId(UUID.randomUUID().toString());

// For unmanagedTable, multiple sdk writer output with same column name can be placed in
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change un managed to transactional table in all PR specific code added.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok. done

gvramana
gvramana reviewed Apr 22, 2018
View reviewed changes
store/sdk/src/main/java/org/apache/carbondata/sdk/file/CarbonWriterBuilder.java Outdated
* e. timestampformat -- same as JAVA SimpleDateFormat
* @return updated CarbonWriterBuilder
*/
public CarbonWriterBuilder withLoadOptions(Map<String, String> options) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

complex type level delimiters should be supported, quote char, escape character needs to be supported, as required for complex types parsing.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok. done

gvramana
gvramana reviewed Apr 22, 2018
View reviewed changes
store/sdk/src/test/java/org/apache/carbondata/sdk/file/CarbonReaderTest.java Outdated
Assert.assertEquals("robot" + (i % 10), row[0]);
Assert.assertEquals(i, row[1]);
Object[] row = (Object[]) reader.readNextRow();
// TODO: Default sort column is applied for dimensions. So, need to validate accordingly
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TODO: to be corrected

gvramana
gvramana reviewed Apr 22, 2018
View reviewed changes
store/sdk/src/main/java/org/apache/carbondata/sdk/file/CarbonWriterBuilder.java Outdated
* To support the load options for sdk writer
* @param options key,value pair of load options.
* supported keys values are
* a. bad_records_logger_enable -- true, false
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please follow standard documentation format

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok

@CarbonDataQA
Copy link

Build Success with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder1/5296/

@CarbonDataQA
Copy link

Build Failed with Spark 2.2.1, Please check CI http://88.99.58.216:8080/job/ApacheCarbonPRBuilder/4117/

@ajantha-bhat
Copy link
Member Author

Retest this please

@CarbonDataQA
Copy link

Build Failed with Spark 2.2.1, Please check CI http://88.99.58.216:8080/job/ApacheCarbonPRBuilder/4119/

@CarbonDataQA
Copy link

Build Success with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder1/5299/

@ajantha-bhat
Copy link
Member Author

Retest this please

@CarbonDataQA
Copy link

Build Failed with Spark 2.2.1, Please check CI http://88.99.58.216:8080/job/ApacheCarbonPRBuilder/4121/

@CarbonDataQA
Copy link

Build Failed with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder1/5301/

@ajantha-bhat
Copy link
Member Author

Retest this please

@CarbonDataQA
Copy link

Build Failed with Spark 2.2.1, Please check CI http://88.99.58.216:8080/job/ApacheCarbonPRBuilder/4123/

@CarbonDataQA
Copy link

Build Failed with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder1/5303/

gvramana
gvramana reviewed Apr 22, 2018
View reviewed changes
core/src/main/java/org/apache/carbondata/core/metadata/schema/table/TableSchemaBuilder.java Outdated
@@ -75,6 +78,11 @@ public TableSchemaBuilder tableName(String tableName) {
return this;
}

public TableSchemaBuilder resetTransactionalTable(boolean isTransactionalTable) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

change function name to setTransactionTable , there nothing resetting happening in this function

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok

gvramana
gvramana reviewed Apr 22, 2018
View reviewed changes
store/sdk/src/main/java/org/apache/carbondata/sdk/file/CarbonWriterBuilder.java Outdated
List<String> sortColumnsList;
if (sortColumns != null) {
sortColumnsList = Arrays.asList(sortColumns);
if (!isTransactionalTable) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

always directly set value to tableSchemaBuilder

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok

@ajantha-bhat
[CARBONDATA-2359] Support applicable load options and table propertie…
d52b3f9 
…s for Non-Transactional table

Support read multiple sdk writer placed at same path
@ajantha-bhat
Copy link
Member Author

retest this please

@CarbonDataQA
Copy link

Build Success with Spark 2.2.1, Please check CI http://88.99.58.216:8080/job/ApacheCarbonPRBuilder/4128/

@CarbonDataQA
Copy link

Build Success with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder1/5308/

@gvramana
Copy link
Contributor

LGTM

@asfgit asfgit closed this in 5f32647 Apr 23, 2018
anubhav100 pushed a commit to anubhav100/incubator-carbondata that referenced this pull request Jun 22, 2018
@ajantha-bhat @anubhav100
[CARBONDATA-2359][SDK] Support applicable load options and table prop…
b9773e1 
…erties for Non-Transactional table

Support read multiple sdk writer placed at same path

This closes apache#2190
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gvramana gvramana gvramana left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@ajantha-bhat @CarbonDataQA @gvramana