[GitHub] carbondata pull request #2113: [CARBONDATA-2347][LUCENE_DATAMAP]load issue i...
Github user asfgit closed the pull request at: https://github.com/apache/carbondata/pull/2113 ---
[GitHub] carbondata pull request #2113: [CARBONDATA-2347][LUCENE_DATAMAP]load issue i...
Github user ravipesala commented on a diff in the pull request:
https://github.com/apache/carbondata/pull/2113#discussion_r181727647
--- Diff:
integration/spark2/src/main/scala/org/apache/spark/sql/execution/command/datamap/CarbonCreateDataMapCommand.scala
---
@@ -69,8 +69,29 @@ case class CarbonCreateDataMapCommand(
}
dataMapSchema = new DataMapSchema(dataMapName, dmClassName)
-if (mainTable != null &&
-mainTable.isStreamingTable &&
+if
(dataMapSchema.getProviderName.equalsIgnoreCase(DataMapClassProvider.LUCENE.toString))
{
--- End diff --
This all validation code should be inside the luceneDatamapFactory init
method. Please don't add any lucene specific code outside of lucene classes
---
[GitHub] carbondata pull request #2113: [CARBONDATA-2347][LUCENE_DATAMAP]load issue i...
Github user ravipesala commented on a diff in the pull request:
https://github.com/apache/carbondata/pull/2113#discussion_r181725781
--- Diff:
core/src/main/java/org/apache/carbondata/core/datamap/IndexDataMapProvider.java
---
@@ -99,25 +103,24 @@ private DataMapFactory
createIndexDataMapFactory(DataMapSchema dataMapSchema)
return dataMapFactory;
}
- private DataMapFactory getDataMapFactoryByShortName(String providerName)
+ public static DataMapFactory getDataMapFactoryByShortName(String
providerName)
throws MalformedDataMapCommandException {
+
DataMapRegistry.registerDataMap(DataMapClassProvider.LUCENE.getClassName(),
--- End diff --
What is the need of specifing the LUCENE here, these classes are supposed
to be independent on any specific datamap.
---
[GitHub] carbondata pull request #2113: [CARBONDATA-2347][LUCENE_DATAMAP]load issue i...
Github user jackylk commented on a diff in the pull request:
https://github.com/apache/carbondata/pull/2113#discussion_r181713494
--- Diff:
datamap/lucene/src/main/java/org/apache/carbondata/datamap/lucene/LuceneDataMapFactoryBase.java
---
@@ -205,9 +234,12 @@ public void clear(Segment segment) {
/**
* Clear all datamaps from memory
*/
- @Override
- public void clear() {
-
+ @Override public void clear() {
--- End diff --
move @override to previous line, please follow this in future
---
[GitHub] carbondata pull request #2113: [CARBONDATA-2347][LUCENE_DATAMAP]load issue i...
Github user jackylk commented on a diff in the pull request:
https://github.com/apache/carbondata/pull/2113#discussion_r181713300
--- Diff:
core/src/main/java/org/apache/carbondata/core/metadata/schema/table/DiskBasedDMSchemaStorageProvider.java
---
@@ -129,9 +130,11 @@ public DiskBasedDMSchemaStorageProvider(String
storePath) {
return dataMapSchemas;
}
- @Override public void dropSchema(String dataMapName) throws IOException {
-String schemaPath =
-storePath + CarbonCommonConstants.FILE_SEPARATOR + dataMapName +
".dmschema";
+ @Override public void dropSchema(String dataMapName, String tableName,
String dataMapProviderName)
+ throws IOException {
+String schemaPath = storePath + CarbonCommonConstants.FILE_SEPARATOR +
tableName
--- End diff --
make an utility function and use it in all place including testcase
---
[GitHub] carbondata pull request #2113: [CARBONDATA-2347][LUCENE_DATAMAP]load issue i...
Github user jackylk commented on a diff in the pull request:
https://github.com/apache/carbondata/pull/2113#discussion_r181666147
--- Diff:
integration/spark2/src/main/scala/org/apache/spark/sql/execution/strategy/DDLStrategy.scala
---
@@ -244,6 +244,13 @@ class DDLStrategy(sparkSession: SparkSession) extends
SparkStrategy {
throw new MalformedCarbonCommandException("Unsupported operation
on unmanaged table")
}
+// TODO remove this limitation after streaming table support
'Lucene' DataMap
+// if the table has 'Lucene' DataMap, it doesn't support streaming
now
+if (carbonTable.hasLuceneDataMap) {
--- End diff --
no need to do this now, do it later
---
[GitHub] carbondata pull request #2113: [CARBONDATA-2347][LUCENE_DATAMAP]load issue i...
Github user jackylk commented on a diff in the pull request: https://github.com/apache/carbondata/pull/2113#discussion_r181665909 --- Diff: integration/spark2/src/main/scala/org/apache/spark/sql/execution/command/mutation/CarbonProjectForUpdateCommand.scala --- @@ -17,6 +17,8 @@ package org.apache.spark.sql.execution.command.mutation +import scala.collection.JavaConverters._ --- End diff -- seems no need to modify this file ---
[GitHub] carbondata pull request #2113: [CARBONDATA-2347][LUCENE_DATAMAP]load issue i...
Github user jackylk commented on a diff in the pull request: https://github.com/apache/carbondata/pull/2113#discussion_r181665850 --- Diff: integration/spark2/src/main/scala/org/apache/spark/sql/execution/command/mutation/CarbonProjectForDeleteCommand.scala --- @@ -17,15 +17,18 @@ package org.apache.spark.sql.execution.command.mutation +import scala.collection.JavaConverters._ --- End diff -- seems no need to modify this file ---
[GitHub] carbondata pull request #2113: [CARBONDATA-2347][LUCENE_DATAMAP]load issue i...
Github user jackylk commented on a diff in the pull request:
https://github.com/apache/carbondata/pull/2113#discussion_r181665755
--- Diff:
integration/spark2/src/main/scala/org/apache/spark/sql/execution/command/datamap/CarbonDataMapShowCommand.scala
---
@@ -40,35 +42,48 @@ case class CarbonDataMapShowCommand(tableIdentifier:
Option[TableIdentifier])
override def output: Seq[Attribute] = {
Seq(AttributeReference("DataMapName", StringType, nullable = false)(),
AttributeReference("ClassName", StringType, nullable = false)(),
- AttributeReference("Associated Table", StringType, nullable =
false)())
+ AttributeReference("Associated Table", StringType, nullable =
false)(),
+ AttributeReference("DMProperties", StringType, nullable = false)())
}
override def processData(sparkSession: SparkSession): Seq[Row] = {
+val finalSchemaList: util.List[DataMapSchema] = new
util.ArrayList[DataMapSchema]()
tableIdentifier match {
case Some(table) =>
Checker.validateTableExists(table.database, table.table,
sparkSession)
val carbonTable = CarbonEnv.getCarbonTable(table)(sparkSession)
if (carbonTable.hasDataMapSchema) {
- val schemaList = carbonTable.getTableInfo.getDataMapSchemaList
- convertToRow(schemaList)
-} else {
-
convertToRow(DataMapStoreManager.getInstance().getAllDataMapSchemas(carbonTable))
+
finalSchemaList.addAll(carbonTable.getTableInfo.getDataMapSchemaList)
+}
+val indexSchemas =
DataMapStoreManager.getInstance().getAllDataMapSchemas(carbonTable)
+if (!indexSchemas.isEmpty) {
+ finalSchemaList.addAll(indexSchemas)
}
+convertToRow(finalSchemaList)
case _ =>
convertToRow(DataMapStoreManager.getInstance().getAllDataMapSchemas)
}
-
}
private def convertToRow(schemaList: util.List[DataMapSchema]) = {
if (schemaList != null && schemaList.size() > 0) {
schemaList.asScala.map { s =>
var table = "(NA)"
val relationIdentifier = s.getRelationIdentifier
-if (relationIdentifier != null) {
+var dmProperties = "(NA)"
+val isFGorCGdm =
--- End diff --
For show datamap command, I think just show the shortname and the main table
And we should have desc datamap command, which I think we should add in
other PR
---
[GitHub] carbondata pull request #2113: [CARBONDATA-2347][LUCENE_DATAMAP]load issue i...
Github user jackylk commented on a diff in the pull request:
https://github.com/apache/carbondata/pull/2113#discussion_r181663757
--- Diff:
integration/spark2/src/main/scala/org/apache/spark/sql/execution/command/datamap/CarbonDataMapShowCommand.scala
---
@@ -40,35 +42,48 @@ case class CarbonDataMapShowCommand(tableIdentifier:
Option[TableIdentifier])
override def output: Seq[Attribute] = {
Seq(AttributeReference("DataMapName", StringType, nullable = false)(),
AttributeReference("ClassName", StringType, nullable = false)(),
- AttributeReference("Associated Table", StringType, nullable =
false)())
+ AttributeReference("Associated Table", StringType, nullable =
false)(),
+ AttributeReference("DMProperties", StringType, nullable = false)())
}
override def processData(sparkSession: SparkSession): Seq[Row] = {
+val finalSchemaList: util.List[DataMapSchema] = new
util.ArrayList[DataMapSchema]()
--- End diff --
rename to datamapSchemaList
---
[GitHub] carbondata pull request #2113: [CARBONDATA-2347][LUCENE_DATAMAP]load issue i...
Github user jackylk commented on a diff in the pull request:
https://github.com/apache/carbondata/pull/2113#discussion_r181663104
--- Diff:
integration/spark-common-test/src/test/scala/org/apache/carbondata/spark/testsuite/datamap/CGDataMapTestCase.scala
---
@@ -402,7 +402,7 @@ class CGDataMapTestCase extends QueryTest with
BeforeAndAfterAll {
sql(s"create datamap test_cg_datamap1 on table datamap_store_test1
using '${classOf[CGDataMapFactory].getName}' as select id, name from
datamap_store_test")
-val loc = CarbonProperties.getInstance().getSystemFolderLocation +
"/test_cg_datamap1.dmschema"
+val loc = CarbonProperties.getInstance().getSystemFolderLocation +
"/datamap_store_test1_test_cg_datamap1.dmschema"
--- End diff --
Please make a utility function to construct the dmschema path
---
[GitHub] carbondata pull request #2113: [CARBONDATA-2347][LUCENE_DATAMAP]load issue i...
Github user jackylk commented on a diff in the pull request:
https://github.com/apache/carbondata/pull/2113#discussion_r181662903
--- Diff:
datamap/lucene/src/test/scala/org/apache/carbondata/datamap/lucene/LuceneFineGrainDataMapSuite.scala
---
@@ -57,7 +65,7 @@ class LuceneFineGrainDataMapSuite extends QueryTest with
BeforeAndAfterAll {
var exception = intercept[MalformedDataMapCommandException](sql(
s"""
| CREATE DATAMAP dm1 ON TABLE datamap_test
- | USING
'org.apache.carbondata.datamap.lucene.LuceneFineGrainDataMapFactory'
+ | USING 'lucenefg'
--- End diff --
please use `lucene` only
---
[GitHub] carbondata pull request #2113: [CARBONDATA-2347][LUCENE_DATAMAP]load issue i...
Github user jackylk commented on a diff in the pull request:
https://github.com/apache/carbondata/pull/2113#discussion_r181662181
--- Diff:
datamap/lucene/src/main/java/org/apache/carbondata/datamap/lucene/LuceneFineGrainDataMapFactory.java
---
@@ -62,7 +62,17 @@
public List getDataMaps(DataMapDistributable
distributable,
ReadCommittedScope readCommittedScope)
throws IOException {
-return getDataMaps(distributable.getSegment(), readCommittedScope);
+List lstDataMap = new ArrayList<>();
+FineGrainDataMap dataMap = new LuceneFineGrainDataMap(analyzer);
+String indexPath = ((LuceneDataMapDistributable)
distributable).getIndexPath();
+try {
+ dataMap.init(new DataMapModel(indexPath));
+} catch (MemoryException e) {
+ LOGGER.error("failed to get lucene datamap , detail is {}" +
e.getMessage());
--- End diff --
you can use String.format() to format the log message
---
[GitHub] carbondata pull request #2113: [CARBONDATA-2347][LUCENE_DATAMAP]load issue i...
Github user jackylk commented on a diff in the pull request: https://github.com/apache/carbondata/pull/2113#discussion_r181661716 --- Diff: datamap/lucene/pom.xml --- @@ -141,6 +141,34 @@ + --- End diff -- Can you move the testcase to spark2 module and add `test` for spark2 in the pom ---
[GitHub] carbondata pull request #2113: [CARBONDATA-2347][LUCENE_DATAMAP]load issue i...
Github user jackylk commented on a diff in the pull request:
https://github.com/apache/carbondata/pull/2113#discussion_r181661362
--- Diff:
core/src/main/java/org/apache/carbondata/core/metadata/schema/table/DiskBasedDMSchemaStorageProvider.java
---
@@ -129,9 +130,9 @@ public DiskBasedDMSchemaStorageProvider(String
storePath) {
return dataMapSchemas;
}
- @Override public void dropSchema(String dataMapName) throws IOException {
-String schemaPath =
-storePath + CarbonCommonConstants.FILE_SEPARATOR + dataMapName +
".dmschema";
+ @Override public void dropSchema(String dataMapName,String tableName)
throws IOException {
+String schemaPath = storePath + CarbonCommonConstants.FILE_SEPARATOR +
tableName
++ CarbonCommonConstants.UNDERSCORE + dataMapName + ".dmschema";
--- End diff --
I think it is better to record the datamap short name in the path also, so
that we can know what kind of datamap it is by looking at the file name
---
[GitHub] carbondata pull request #2113: [CARBONDATA-2347][LUCENE_DATAMAP]load issue i...
Github user jackylk commented on a diff in the pull request:
https://github.com/apache/carbondata/pull/2113#discussion_r181660460
--- Diff:
core/src/main/java/org/apache/carbondata/core/metadata/schema/table/DataMapSchema.java
---
@@ -143,11 +142,12 @@ public void setChildSchema(TableSchema childSchema) {
/**
* Return true if this datamap is an Index DataMap
+ *
--- End diff --
no need to modify
---
[GitHub] carbondata pull request #2113: [CARBONDATA-2347][LUCENE_DATAMAP]load issue i...
Github user jackylk commented on a diff in the pull request:
https://github.com/apache/carbondata/pull/2113#discussion_r181660484
--- Diff:
core/src/main/java/org/apache/carbondata/core/metadata/schema/table/DataMapSchema.java
---
@@ -143,11 +142,12 @@ public void setChildSchema(TableSchema childSchema) {
/**
* Return true if this datamap is an Index DataMap
+ *
* @return
*/
public boolean isIndexDataMap() {
-if
(providerName.equalsIgnoreCase(DataMapClassProvider.PREAGGREGATE.getShortName())
||
-
providerName.equalsIgnoreCase(DataMapClassProvider.TIMESERIES.getShortName())) {
+if
(providerName.equalsIgnoreCase(DataMapClassProvider.PREAGGREGATE.getShortName())
+||
providerName.equalsIgnoreCase(DataMapClassProvider.TIMESERIES.getShortName())) {
--- End diff --
no need to modify
---
[GitHub] carbondata pull request #2113: [CARBONDATA-2347][LUCENE_DATAMAP]load issue i...
Github user jackylk commented on a diff in the pull request:
https://github.com/apache/carbondata/pull/2113#discussion_r181660289
--- Diff:
core/src/main/java/org/apache/carbondata/core/metadata/schema/table/CarbonTable.java
---
@@ -856,6 +858,25 @@ public boolean hasAggregationDataMap() {
return false;
}
+ /**
+ * whether this table has Lucene DataMap or not
+ */
+ public boolean hasLuceneDataMap() {
--- End diff --
remove this now, we should add blocking feature in other PR
---
[GitHub] carbondata pull request #2113: [CARBONDATA-2347][LUCENE_DATAMAP]load issue i...
Github user jackylk commented on a diff in the pull request:
https://github.com/apache/carbondata/pull/2113#discussion_r181659113
--- Diff:
core/src/main/java/org/apache/carbondata/core/metadata/SegmentFileStore.java ---
@@ -681,10 +681,10 @@ private static void
deletePhysicalPartition(List partitionSpecs,
FileFactory.deleteAllCarbonFilesOfDir(FileFactory.getCarbonFile(location.toString()));
}
} else {
-// delete the segment folder if it is empty
+// delete the segment folder
CarbonFile file = FileFactory.getCarbonFile(location.toString());
--- End diff --
change `file` to `segmentPath`
---
[GitHub] carbondata pull request #2113: [CARBONDATA-2347][LUCENE_DATAMAP]load issue i...
Github user jackylk commented on a diff in the pull request: https://github.com/apache/carbondata/pull/2113#discussion_r181658482 --- Diff: core/src/main/java/org/apache/carbondata/core/constants/CarbonCommonConstants.java --- @@ -1642,6 +1642,16 @@ public static final String CARBON_SEARCH_MODE_THREAD_DEFAULT = "3"; + /** + * compression mode used by lucene for index writing + */ + public static final String CARBON_LUCENE_COMPRESSION_MODE = "carbon.lucene.compression.mode"; --- End diff -- mention it in the description in line 1646 ---
[GitHub] carbondata pull request #2113: [CARBONDATA-2347][LUCENE_DATAMAP]load issue i...
Github user ravipesala commented on a diff in the pull request:
https://github.com/apache/carbondata/pull/2113#discussion_r181629595
--- Diff:
core/src/main/java/org/apache/carbondata/core/datamap/DataMapStoreManager.java
---
@@ -246,17 +247,24 @@ public TableDataMap getDataMap(CarbonTable table,
DataMapSchema dataMapSchema) {
public TableDataMap createAndRegisterDataMap(CarbonTable table,
DataMapSchema dataMapSchema) throws
MalformedDataMapCommandException, IOException {
DataMapFactory dataMapFactory;
-try {
- // try to create datamap by reflection to test whether it is a valid
DataMapFactory class
- Class factoryClass =
- (Class)
Class.forName(dataMapSchema.getProviderName());
- dataMapFactory = factoryClass.newInstance();
-} catch (ClassNotFoundException e) {
- throw new MalformedDataMapCommandException(
- "DataMap '" + dataMapSchema.getProviderName() + "' not found");
-} catch (Throwable e) {
- throw new MetadataProcessException(
- "failed to create DataMap '" + dataMapSchema.getProviderName() +
"'", e);
+if (dataMapSchema.getProviderName()
+.equalsIgnoreCase(DataMapClassProvider.LUCENEFG.getShortName()) ||
dataMapSchema
+
.getProviderName().equalsIgnoreCase(DataMapClassProvider.LUCENECG.getShortName()))
{
--- End diff --
Why is this special check? why can't it instantiated from old code?
---
[GitHub] carbondata pull request #2113: [CARBONDATA-2347][LUCENE_DATAMAP]load issue i...
Github user akashrn5 commented on a diff in the pull request:
https://github.com/apache/carbondata/pull/2113#discussion_r181439768
--- Diff:
integration/spark2/src/main/scala/org/apache/spark/sql/CarbonEnv.scala ---
@@ -173,6 +174,10 @@ object CarbonEnv {
.addListener(classOf[AlterTableDropPartitionPostStatusEvent],
AlterTableDropPartitionPostStatusListener)
.addListener(classOf[AlterTableDropPartitionMetaEvent],
AlterTableDropPartitionMetaListener)
+ .addListener(classOf[AlterTableRenamePreEvent],
LuceneRenameTablePreListener)
--- End diff --
this listener class is added to block alter operation on lucene datamap, if
we are blocking alter operation for all the datamaps, then this may not be
required.
---
[GitHub] carbondata pull request #2113: [CARBONDATA-2347][LUCENE_DATAMAP]load issue i...
Github user akashrn5 commented on a diff in the pull request: https://github.com/apache/carbondata/pull/2113#discussion_r181438240 --- Diff: datamap/lucene/pom.xml --- @@ -141,6 +141,34 @@ + --- End diff -- this was added to include test suite in main CI ---
[GitHub] carbondata pull request #2113: [CARBONDATA-2347][LUCENE_DATAMAP]load issue i...
Github user akashrn5 commented on a diff in the pull request: https://github.com/apache/carbondata/pull/2113#discussion_r181437595 --- Diff: core/src/main/java/org/apache/carbondata/core/constants/CarbonCommonConstants.java --- @@ -1642,6 +1642,16 @@ public static final String CARBON_SEARCH_MODE_THREAD_DEFAULT = "3"; + /** + * compression mode used by lucene for index writing + */ + public static final String CARBON_LUCENE_COMPRESSION_MODE = "carbon.lucene.compression.mode"; --- End diff -- SPEED and COMPRESSION, by default the property value will be SPEED ---
[GitHub] carbondata pull request #2113: [CARBONDATA-2347][LUCENE_DATAMAP]load issue i...
Github user jackylk commented on a diff in the pull request:
https://github.com/apache/carbondata/pull/2113#discussion_r181399425
--- Diff:
integration/spark2/src/main/scala/org/apache/spark/sql/execution/command/datamap/CarbonCreateDataMapCommand.scala
---
@@ -69,11 +69,33 @@ case class CarbonCreateDataMapCommand(
}
dataMapSchema = new DataMapSchema(dataMapName, dmClassName)
-if (mainTable != null &&
-mainTable.isStreamingTable &&
-
!(dataMapSchema.getProviderName.equalsIgnoreCase(DataMapClassProvider.PREAGGREGATE.toString)
- || dataMapSchema.getProviderName
-.equalsIgnoreCase(DataMapClassProvider.TIMESERIES.toString))) {
+if
(dataMapSchema.getProviderName.equalsIgnoreCase(DataMapClassProvider.LUCENEFG.toString)
||
--- End diff --
I think we should abstract interface for it. We can not add if check for
every new datamap added
---
[GitHub] carbondata pull request #2113: [CARBONDATA-2347][LUCENE_DATAMAP]load issue i...
Github user jackylk commented on a diff in the pull request:
https://github.com/apache/carbondata/pull/2113#discussion_r181397988
--- Diff:
integration/spark2/src/main/scala/org/apache/spark/sql/CarbonEnv.scala ---
@@ -173,6 +174,10 @@ object CarbonEnv {
.addListener(classOf[AlterTableDropPartitionPostStatusEvent],
AlterTableDropPartitionPostStatusListener)
.addListener(classOf[AlterTableDropPartitionMetaEvent],
AlterTableDropPartitionMetaListener)
+ .addListener(classOf[AlterTableRenamePreEvent],
LuceneRenameTablePreListener)
--- End diff --
Is this required? Ideally, lucene datamap is a separate module which should
not have intrusive modification in other modules
---
[GitHub] carbondata pull request #2113: [CARBONDATA-2347][LUCENE_DATAMAP]load issue i...
Github user jackylk commented on a diff in the pull request: https://github.com/apache/carbondata/pull/2113#discussion_r181396888 --- Diff: datamap/lucene/pom.xml --- @@ -141,6 +141,34 @@ + --- End diff -- I realize that in this pom, it should not depend on carbon-spark2, please modify the dependency in this pom ---
[GitHub] carbondata pull request #2113: [CARBONDATA-2347][LUCENE_DATAMAP]load issue i...
Github user jackylk commented on a diff in the pull request: https://github.com/apache/carbondata/pull/2113#discussion_r181394048 --- Diff: core/src/main/java/org/apache/carbondata/core/constants/CarbonCommonConstants.java --- @@ -1642,6 +1642,16 @@ public static final String CARBON_SEARCH_MODE_THREAD_DEFAULT = "3"; + /** + * compression mode used by lucene for index writing + */ + public static final String CARBON_LUCENE_COMPRESSION_MODE = "carbon.lucene.compression.mode"; --- End diff -- what are the options available for this property? ---
