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-3609][CARBONDATA-3610] Remove preaggregate and timeseries datamap #3522

Closed
wants to merge 4 commits into from
Closed

[CARBONDATA-3609][CARBONDATA-3610] Remove preaggregate and timeseries datamap #3522

wants to merge 4 commits into from

Conversation

jackylk
Copy link
Contributor

@jackylk jackylk commented Dec 20, 2019

Be sure to do all of the following checklist to help us incorporate
your contribution quickly and easily:

  • Any interfaces changed?

  • Any backward compatibility impacted?

  • Document update required?

  • Testing done
    Please provide details on
    - Whether new unit test cases have been added or why no new tests are required?
    - How it is tested? Please attach test report.
    - Is it a performance related change? Please attach the performance test report.
    - Any additional information to help reviewers in testing this change.

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

@CarbonDataQA1
Copy link

Build Failed with Spark 2.1.0, Please check CI http://121.244.95.60:12545/job/ApacheCarbonPRBuilder2.1/1228/

@CarbonDataQA1
Copy link

Build Failed with Spark 2.2.1, Please check CI http://121.244.95.60:12545/job/ApacheCarbonPRBuilder2.2/1238/

@CarbonDataQA1
Copy link

Build Failed with Spark 2.3.4, Please check CI http://121.244.95.60:12545/job/ApacheCarbonPRBuilder2.3/1247/

@CarbonDataQA1
Copy link

Build Success with Spark 2.1.0, Please check CI http://121.244.95.60:12545/job/ApacheCarbonPRBuilder2.1/1231/

@CarbonDataQA1
Copy link

Build Failed with Spark 2.2.1, Please check CI http://121.244.95.60:12545/job/ApacheCarbonPRBuilder2.2/1241/

@CarbonDataQA1
Copy link

Build Failed with Spark 2.3.4, Please check CI http://121.244.95.60:12545/job/ApacheCarbonPRBuilder2.3/1250/

@CarbonDataQA1
Copy link

Build Success with Spark 2.1.0, Please check CI http://121.244.95.60:12545/job/ApacheCarbonPRBuilder2.1/1233/

@CarbonDataQA1
Copy link

Build Failed with Spark 2.2.1, Please check CI http://121.244.95.60:12545/job/ApacheCarbonPRBuilder2.2/1243/

@CarbonDataQA1
Copy link

Build Failed with Spark 2.3.4, Please check CI http://121.244.95.60:12545/job/ApacheCarbonPRBuilder2.3/1252/

@jackylk jackylk changed the title [WIP] Remove preaggregate and timeseries datamap [CARBONDATA-3609][CARBONDATA-3610] Remove preaggregate and timeseries datamap Dec 21, 2019
@CarbonDataQA1
Copy link

Build Failed with Spark 2.2.1, Please check CI http://121.244.95.60:12545/job/ApacheCarbonPRBuilder2.2/1245/

@CarbonDataQA1
Copy link

Build Failed with Spark 2.3.4, Please check CI http://121.244.95.60:12545/job/ApacheCarbonPRBuilder2.3/1254/

@CarbonDataQA1
Copy link

Build Success with Spark 2.1.0, Please check CI http://121.244.95.60:12545/job/ApacheCarbonPRBuilder2.1/1235/

@CarbonDataQA1
Copy link

Build Success with Spark 2.1.0, Please check CI http://121.244.95.60:12545/job/ApacheCarbonPRBuilder2.1/1239/

@CarbonDataQA1
Copy link

Build Failed with Spark 2.2.1, Please check CI http://121.244.95.60:12545/job/ApacheCarbonPRBuilder2.2/1249/

@CarbonDataQA1
Copy link

Build Success with Spark 2.3.4, Please check CI http://121.244.95.60:12545/job/ApacheCarbonPRBuilder2.3/1258/

@jackylk
Copy link
Contributor Author

jackylk commented Dec 22, 2019

retest this please

@CarbonDataQA1
Copy link

Build Success with Spark 2.1.0, Please check CI http://121.244.95.60:12545/job/ApacheCarbonPRBuilder2.1/1241/

@CarbonDataQA1
Copy link

Build Success with Spark 2.2.1, Please check CI http://121.244.95.60:12545/job/ApacheCarbonPRBuilder2.2/1251/

@CarbonDataQA1
Copy link

Build Success with Spark 2.3.4, Please check CI http://121.244.95.60:12545/job/ApacheCarbonPRBuilder2.3/1260/

@ajantha-bhat
Copy link
Member

By removing preaggregate we lose preaggregate + streaming feature. But we can implement it on mv (currently doesn't support)

@akashrn5, @kumarvishal09 : please check the changes once. I will also review.

@ajantha-bhat
Copy link
Member

@akashrn5 , @Indhumathi27 : Is all pre-aggreagte and timeseries testcase scenario we have converted to MV test cases? If not instead of removing pre-aggregate test case, may be we can convert them to mv test case.

@Indhumathi27
Copy link
Contributor

@ajantha-bhat I have checked MV test cases. It contains all preaggregate scenarios. So, I think Preaggregate test cases can be removed.

ajantha-bhat
ajantha-bhat reviewed Dec 23, 2019
View reviewed changes
...src/main/scala/org/apache/spark/sql/execution/command/datamap/CarbonDropDataMapCommand.scala
@@ -33,7 +32,6 @@ import org.apache.carbondata.core.datamap.{DataMapProvider, DataMapStoreManager}
import org.apache.carbondata.core.datamap.status.DataMapStatusManager
import org.apache.carbondata.core.locks.{CarbonLockUtil, ICarbonLock, LockUsage}
import org.apache.carbondata.core.metadata.AbsoluteTableIdentifier
import org.apache.carbondata.core.metadata.converter.ThriftWrapperSchemaConverterImpl
Copy link
Member

Choose a reason for hiding this comment

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

In Schema.Thrift
DataMapSchema has childTableIdentifier, childTableSchema.

I think we can check and remove those if it is not used for MV

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This will break compatibility, better do not delete them

ajantha-bhat
ajantha-bhat reviewed Dec 23, 2019
View reviewed changes
...rk2/src/main/scala/org/apache/spark/sql/execution/command/table/CarbonDropTableCommand.scala
@@ -69,7 +69,7 @@ case class CarbonDropTableCommand(
CarbonLockUtil.getLockObject(identifier, lock)
}
// check for directly drop datamap table
if (carbonTable.isChildTable && !dropChildTable) {
if (carbonTable.isChildTableForMV && !dropChildTable) {
Copy link
Member

Choose a reason for hiding this comment

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

This file has a big section for pre-aggregate, we can remove that code

if (carbonTable.getTableInfo.getParentRelationIdentifiers.size() == 1) {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

ajantha-bhat
ajantha-bhat reviewed Dec 23, 2019
View reviewed changes
hadoop/src/main/java/org/apache/carbondata/hadoop/api/CarbonTableInputFormat.java
}
} else {
List<Segment> filteredNormalSegments =
getFilteredNormalSegments(segments.getValidSegments(),
Copy link
Member

Choose a reason for hiding this comment

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

getFilteredNormalSegments definition also we can remove, it is unused now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

ajantha-bhat
ajantha-bhat reviewed Dec 23, 2019
View reviewed changes
README.md
@@ -58,8 +58,6 @@ CarbonData is built using Apache Maven, to [build CarbonData](https://github.com
* [CarbonData DataMap Management](https://github.com/apache/carbondata/blob/master/docs/datamap/datamap-management.md)
* [CarbonData BloomFilter DataMap](https://github.com/apache/carbondata/blob/master/docs/datamap/bloomfilter-datamap-guide.md)
* [CarbonData Lucene DataMap](https://github.com/apache/carbondata/blob/master/docs/datamap/lucene-datamap-guide.md)
* [CarbonData Pre-aggregate DataMap](https://github.com/apache/carbondata/blob/master/docs/datamap/preaggregate-datamap-guide.md)
* [CarbonData Timeseries DataMap](https://github.com/apache/carbondata/blob/master/docs/datamap/timeseries-datamap-guide.md)
Copy link
Member

Choose a reason for hiding this comment

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

In introduction.md also need to remove.

please search for "preAgg", "pre Agg", "pre-Agg".
I still see few more places need to removed and modified. Can handle comment section also

Copy link
Contributor Author

Choose a reason for hiding this comment

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

searched all place and fixed

@CarbonDataQA1
Copy link

Build Failed with Spark 2.1.0, Please check CI http://121.244.95.60:12545/job/ApacheCarbonPRBuilder2.1/1257/

@CarbonDataQA1
Copy link

Build Failed with Spark 2.3.4, Please check CI http://121.244.95.60:12545/job/ApacheCarbonPRBuilder2.3/1278/

@CarbonDataQA1
Copy link

Build Failed with Spark 2.2.1, Please check CI http://121.244.95.60:12545/job/ApacheCarbonPRBuilder2.2/1267/

@CarbonDataQA1
Copy link

Build Success with Spark 2.1.0, Please check CI http://121.244.95.60:12545/job/ApacheCarbonPRBuilder2.1/1259/

@CarbonDataQA1
Copy link

Build Success with Spark 2.2.1, Please check CI http://121.244.95.60:12545/job/ApacheCarbonPRBuilder2.2/1269/

@CarbonDataQA1
Copy link

Build Success with Spark 2.3.4, Please check CI http://121.244.95.60:12545/job/ApacheCarbonPRBuilder2.3/1280/

@ajantha-bhat
Copy link
Member

LGTM

@asfgit asfgit closed this in 4a603d6 Dec 24, 2019
MarvinLitt pushed a commit to MarvinLitt/carbondata that referenced this pull request Jan 3, 2020
@jackylk @MarvinLitt
[CARBONDATA-3609][CARBONDATA-3610] Remove preaggregate and timeseries…
8c535cb 
… datamap

Remove preaggregate and timeseries datamap as the same features are
handled in MV.

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

@ajantha-bhat ajantha-bhat ajantha-bhat 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
4 participants
@jackylk @CarbonDataQA1 @ajantha-bhat @Indhumathi27