[GitHub] [carbondata] akashrn5 opened a new pull request #3755: [WIP]refactor MV events

[GitBox]

akashrn5 opened a new pull request #3755:
URL: https://github.com/apache/carbondata/pull/3755


    ### Why is this PR needed?
   
   
    ### What changes were proposed in this PR?
   
       
    ### Does this PR introduce any user interface change?
    - No
    - Yes. (please explain the change and update document)
   
    ### Is any new testcase added?
    - No
    - Yes
   
       
   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]

[GitBox]

CarbonDataQA1 commented on pull request #3755:
URL: https://github.com/apache/carbondata/pull/3755#issuecomment-625443989


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


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]

[GitBox]

CarbonDataQA1 commented on pull request #3755:
URL: https://github.com/apache/carbondata/pull/3755#issuecomment-625444419


   Build Success with Spark 2.4.5, Please check CI http://121.244.95.60:12545/job/ApacheCarbon_PR_Builder_2.4.5/1254/
   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]

[GitBox]

Indhumathi27 commented on a change in pull request #3755:
URL: https://github.com/apache/carbondata/pull/3755#discussion_r422795020



##########
File path: integration/spark/src/main/scala/org/apache/carbondata/view/MVEvents.scala
##########
@@ -20,31 +20,43 @@ package org.apache.carbondata.view
 import org.apache.spark.sql.SparkSession
 import org.apache.spark.sql.catalyst.TableIdentifier
 
-import org.apache.carbondata.events.Event
+import org.apache.carbondata.events.{Event, MVEventsInfo}
 
 /**
- * For handling operation's before start of mv creation
+ * For handling operation's before start of MV creation
  */
-case class CreateMVPreExecutionEvent(sparkSession: SparkSession,
-    tableIdentifier: TableIdentifier) extends Event
+case class CreateMVPreExecutionEvent(
+    sparkSession: SparkSession,
+    storePath: String,

Review comment:
       Can rename `storePath` to `systemDirectoryPath` in all mv events




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]

[GitBox]

Indhumathi27 commented on a change in pull request #3755:
URL: https://github.com/apache/carbondata/pull/3755#discussion_r422795851



##########
File path: integration/spark/src/main/scala/org/apache/spark/sql/execution/command/view/CarbonRefreshMVCommand.scala
##########
@@ -33,33 +33,33 @@ import org.apache.carbondata.view.{MVManagerInSpark, MVRefresher, RefreshMVPostE
  */
 case class CarbonRefreshMVCommand(
     databaseNameOption: Option[String],
-    name: String) extends DataCommand {
+    mvName: String) extends DataCommand {
 
   override def processData(session: SparkSession): Seq[Row] = {
     val databaseName =
       databaseNameOption.getOrElse(session.sessionState.catalog.getCurrentDatabase)
     val viewManager = MVManagerInSpark.get(session)
     val schema = try {
-      viewManager.getSchema(databaseName, name)
+      viewManager.getSchema(databaseName, mvName)
     } catch {
       case _: NoSuchMVException =>
         throw new MalformedMVCommandException(
-          s"Materialized view ${ databaseName }.${ name } does not exist")
+          s"Materialized view ${ databaseName }.${ mvName } does not exist")
     }
-    val table = CarbonEnv.getCarbonTable(Option(databaseName), name)(session)
+    val table = CarbonEnv.getCarbonTable(Option(databaseName), mvName)(session)
     setAuditTable(table)
 
     MVRefresher.refresh(schema, session)
 
     // After rebuild successfully enable the MV table.
-    val identifier = TableIdentifier(name, Option(databaseName))
+    val identifier = TableIdentifier(mvName, Option(databaseName))
     val operationContext = new OperationContext()
     OperationListenerBus.getInstance().fireEvent(
       RefreshMVPreExecutionEvent(session, identifier),
       operationContext)
     viewManager.setStatus(schema.getIdentifier, MVStatus.ENABLED)
     OperationListenerBus.getInstance().fireEvent(
-      RefreshMVPostExecutionEvent(session, identifier),
+      RefreshMVPostExecutionEvent(session, identifier, mvName),

Review comment:
       Please check if  `RefreshMVPreExecutionEvent` is used. If not used, please remove




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]

[GitBox]

akashrn5 commented on a change in pull request #3755:
URL: https://github.com/apache/carbondata/pull/3755#discussion_r422800116



##########
File path: integration/spark/src/main/scala/org/apache/carbondata/view/MVEvents.scala
##########
@@ -20,31 +20,43 @@ package org.apache.carbondata.view
 import org.apache.spark.sql.SparkSession
 import org.apache.spark.sql.catalyst.TableIdentifier
 
-import org.apache.carbondata.events.Event
+import org.apache.carbondata.events.{Event, MVEventsInfo}
 
 /**
- * For handling operation's before start of mv creation
+ * For handling operation's before start of MV creation
  */
-case class CreateMVPreExecutionEvent(sparkSession: SparkSession,
-    tableIdentifier: TableIdentifier) extends Event
+case class CreateMVPreExecutionEvent(
+    sparkSession: SparkSession,
+    storePath: String,

Review comment:
       done




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]

[GitBox]

akashrn5 commented on a change in pull request #3755:
URL: https://github.com/apache/carbondata/pull/3755#discussion_r422800151



##########
File path: integration/spark/src/main/scala/org/apache/spark/sql/execution/command/view/CarbonRefreshMVCommand.scala
##########
@@ -33,33 +33,33 @@ import org.apache.carbondata.view.{MVManagerInSpark, MVRefresher, RefreshMVPostE
  */
 case class CarbonRefreshMVCommand(
     databaseNameOption: Option[String],
-    name: String) extends DataCommand {
+    mvName: String) extends DataCommand {
 
   override def processData(session: SparkSession): Seq[Row] = {
     val databaseName =
       databaseNameOption.getOrElse(session.sessionState.catalog.getCurrentDatabase)
     val viewManager = MVManagerInSpark.get(session)
     val schema = try {
-      viewManager.getSchema(databaseName, name)
+      viewManager.getSchema(databaseName, mvName)
     } catch {
       case _: NoSuchMVException =>
         throw new MalformedMVCommandException(
-          s"Materialized view ${ databaseName }.${ name } does not exist")
+          s"Materialized view ${ databaseName }.${ mvName } does not exist")
     }
-    val table = CarbonEnv.getCarbonTable(Option(databaseName), name)(session)
+    val table = CarbonEnv.getCarbonTable(Option(databaseName), mvName)(session)
     setAuditTable(table)
 
     MVRefresher.refresh(schema, session)
 
     // After rebuild successfully enable the MV table.
-    val identifier = TableIdentifier(name, Option(databaseName))
+    val identifier = TableIdentifier(mvName, Option(databaseName))
     val operationContext = new OperationContext()
     OperationListenerBus.getInstance().fireEvent(
       RefreshMVPreExecutionEvent(session, identifier),
       operationContext)
     viewManager.setStatus(schema.getIdentifier, MVStatus.ENABLED)
     OperationListenerBus.getInstance().fireEvent(
-      RefreshMVPostExecutionEvent(session, identifier),
+      RefreshMVPostExecutionEvent(session, identifier, mvName),

Review comment:
       its being used




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]

[GitBox]

Indhumathi27 commented on a change in pull request #3755:
URL: https://github.com/apache/carbondata/pull/3755#discussion_r422802043



##########
File path: integration/spark/src/main/scala/org/apache/spark/sql/execution/command/view/CarbonRefreshMVCommand.scala
##########
@@ -33,33 +33,33 @@ import org.apache.carbondata.view.{MVManagerInSpark, MVRefresher, RefreshMVPostE
  */
 case class CarbonRefreshMVCommand(
     databaseNameOption: Option[String],
-    name: String) extends DataCommand {
+    mvName: String) extends DataCommand {
 
   override def processData(session: SparkSession): Seq[Row] = {
     val databaseName =
       databaseNameOption.getOrElse(session.sessionState.catalog.getCurrentDatabase)
     val viewManager = MVManagerInSpark.get(session)
     val schema = try {
-      viewManager.getSchema(databaseName, name)
+      viewManager.getSchema(databaseName, mvName)
     } catch {
       case _: NoSuchMVException =>
         throw new MalformedMVCommandException(
-          s"Materialized view ${ databaseName }.${ name } does not exist")
+          s"Materialized view ${ databaseName }.${ mvName } does not exist")
     }
-    val table = CarbonEnv.getCarbonTable(Option(databaseName), name)(session)
+    val table = CarbonEnv.getCarbonTable(Option(databaseName), mvName)(session)
     setAuditTable(table)
 
     MVRefresher.refresh(schema, session)
 
     // After rebuild successfully enable the MV table.
-    val identifier = TableIdentifier(name, Option(databaseName))
+    val identifier = TableIdentifier(mvName, Option(databaseName))
     val operationContext = new OperationContext()
     OperationListenerBus.getInstance().fireEvent(
       RefreshMVPreExecutionEvent(session, identifier),
       operationContext)
     viewManager.setStatus(schema.getIdentifier, MVStatus.ENABLED)
     OperationListenerBus.getInstance().fireEvent(
-      RefreshMVPostExecutionEvent(session, identifier),
+      RefreshMVPostExecutionEvent(session, identifier, mvName),

Review comment:
       mvName is present in TableIdentifier itself. Can remove mvName from RefreshMVPostExecutionEvent




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]

[GitBox]

akashrn5 commented on a change in pull request #3755:
URL: https://github.com/apache/carbondata/pull/3755#discussion_r422806575



##########
File path: integration/spark/src/main/scala/org/apache/spark/sql/execution/command/view/CarbonRefreshMVCommand.scala
##########
@@ -33,33 +33,33 @@ import org.apache.carbondata.view.{MVManagerInSpark, MVRefresher, RefreshMVPostE
  */
 case class CarbonRefreshMVCommand(
     databaseNameOption: Option[String],
-    name: String) extends DataCommand {
+    mvName: String) extends DataCommand {
 
   override def processData(session: SparkSession): Seq[Row] = {
     val databaseName =
       databaseNameOption.getOrElse(session.sessionState.catalog.getCurrentDatabase)
     val viewManager = MVManagerInSpark.get(session)
     val schema = try {
-      viewManager.getSchema(databaseName, name)
+      viewManager.getSchema(databaseName, mvName)
     } catch {
       case _: NoSuchMVException =>
         throw new MalformedMVCommandException(
-          s"Materialized view ${ databaseName }.${ name } does not exist")
+          s"Materialized view ${ databaseName }.${ mvName } does not exist")
     }
-    val table = CarbonEnv.getCarbonTable(Option(databaseName), name)(session)
+    val table = CarbonEnv.getCarbonTable(Option(databaseName), mvName)(session)
     setAuditTable(table)
 
     MVRefresher.refresh(schema, session)
 
     // After rebuild successfully enable the MV table.
-    val identifier = TableIdentifier(name, Option(databaseName))
+    val identifier = TableIdentifier(mvName, Option(databaseName))
     val operationContext = new OperationContext()
     OperationListenerBus.getInstance().fireEvent(
       RefreshMVPreExecutionEvent(session, identifier),
       operationContext)
     viewManager.setStatus(schema.getIdentifier, MVStatus.ENABLED)
     OperationListenerBus.getInstance().fireEvent(
-      RefreshMVPostExecutionEvent(session, identifier),
+      RefreshMVPostExecutionEvent(session, identifier, mvName),

Review comment:
       changed




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]

[GitBox]

niuge01 commented on a change in pull request #3755:
URL: https://github.com/apache/carbondata/pull/3755#discussion_r422812384



##########
File path: integration/spark/src/main/scala/org/apache/carbondata/events/Events.scala
##########
@@ -195,7 +195,7 @@ trait CreateCarbonRelationEventInfo {
  */
 trait MVEventsInfo {
   val sparkSession: SparkSession
-  val storePath: String
+  val systemDirectoryPath: String

Review comment:
       The field name changed, but i can not found any code which used the field has be changed, it's will be OK？




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]

[GitBox]

akashrn5 commented on a change in pull request #3755:
URL: https://github.com/apache/carbondata/pull/3755#discussion_r422813403



##########
File path: integration/spark/src/main/scala/org/apache/carbondata/events/Events.scala
##########
@@ -195,7 +195,7 @@ trait CreateCarbonRelationEventInfo {
  */
 trait MVEventsInfo {
   val sparkSession: SparkSession
-  val storePath: String
+  val systemDirectoryPath: String

Review comment:
       yes, this is just the event, if any user wants to implement a listener for this event, then he will use these variable. So it is ok




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]

[GitBox]

CarbonDataQA1 commented on pull request #3755:
URL: https://github.com/apache/carbondata/pull/3755#issuecomment-626512421


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


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]

[GitBox]

akashrn5 commented on pull request #3755:
URL: https://github.com/apache/carbondata/pull/3755#issuecomment-626513349


   retest this please


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]

[GitBox]

CarbonDataQA1 commented on pull request #3755:
URL: https://github.com/apache/carbondata/pull/3755#issuecomment-626581844


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


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]

[GitBox]

CarbonDataQA1 commented on pull request #3755:
URL: https://github.com/apache/carbondata/pull/3755#issuecomment-626583695


   Build Success with Spark 2.4.5, Please check CI http://121.244.95.60:12545/job/ApacheCarbon_PR_Builder_2.4.5/1271/
   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]