[GitHub] incubator-griffin pull request #397: Removed bug
Github user asfgit closed the pull request at: https://github.com/apache/incubator-griffin/pull/397 ---
[GitHub] incubator-griffin pull request #396: set the fields in configuration classes...
Github user grant-xuexu commented on a diff in the pull request:
https://github.com/apache/incubator-griffin/pull/396#discussion_r210924616
--- Diff:
measure/src/main/scala/org/apache/griffin/measure/datasource/connector/DataConnectorFactory.scala
---
@@ -91,11 +91,12 @@ object DataConnectorFactory extends Loggable {
): KafkaStreamingDataConnector = {
val KeyType = "key.type"
val ValueType = "value.type"
-val config = dcParam.config
+val config = dcParam.getConfig
val keyType = config.getOrElse(KeyType, "java.lang.String").toString
val valueType = config.getOrElse(ValueType,
"java.lang.String").toString
-(getClassTag(keyType), getClassTag(valueType)) match {
- case (ClassTag(k: Class[String]), ClassTag(v: Class[String])) => {
+
+(keyType, valueType) match {
+ case ("java.lang.String", "java.lang.String") => {
KafkaStreamingStringDataConnector(sparkSession, ssc, dcParam,
tmstCache, streamingCacheClientOpt)
--- End diff --
Or we could move the type checking logic to configuration validation
---
[GitHub] incubator-griffin pull request #396: set the fields in configuration classes...
Github user grant-xuexu commented on a diff in the pull request:
https://github.com/apache/incubator-griffin/pull/396#discussion_r210923398
--- Diff:
measure/src/main/scala/org/apache/griffin/measure/datasource/connector/DataConnectorFactory.scala
---
@@ -91,11 +91,12 @@ object DataConnectorFactory extends Loggable {
): KafkaStreamingDataConnector = {
val KeyType = "key.type"
val ValueType = "value.type"
-val config = dcParam.config
+val config = dcParam.getConfig
val keyType = config.getOrElse(KeyType, "java.lang.String").toString
val valueType = config.getOrElse(ValueType,
"java.lang.String").toString
-(getClassTag(keyType), getClassTag(valueType)) match {
- case (ClassTag(k: Class[String]), ClassTag(v: Class[String])) => {
+
+(keyType, valueType) match {
+ case ("java.lang.String", "java.lang.String") => {
KafkaStreamingStringDataConnector(sparkSession, ssc, dcParam,
tmstCache, streamingCacheClientOpt)
--- End diff --
the logic here seems assume the key type and value type are both String, or
throw the exception.
And the type erasure makes `case (ClassTag(k: Class[String]), ClassTag(v:
Class[String])) => {` already true
---
[GitHub] incubator-griffin pull request #397: Removed bug
GitHub user SparshSinghalHM opened a pull request: https://github.com/apache/incubator-griffin/pull/397 Removed bug Updated property to remove space. You can merge this pull request into a Git repository by running: $ git pull https://github.com/SparshSinghalHM/incubator-griffin patch-2 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/incubator-griffin/pull/397.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #397 commit 6e4912936debd7326547274ee25ba35d2ab733d0 Author: Sparsh Singhal Date: 2018-08-17T13:20:16Z Removed bug ---
[GitHub] incubator-griffin pull request #396: set the fields in configuration classes...
Github user guoyuepeng commented on a diff in the pull request:
https://github.com/apache/incubator-griffin/pull/396#discussion_r210855056
--- Diff:
measure/src/main/scala/org/apache/griffin/measure/step/builder/dsl/expr/TreeNode.scala
---
@@ -18,28 +18,42 @@ under the License.
*/
package org.apache.griffin.measure.step.builder.dsl.expr
+import scala.reflect.ClassTag
+
trait TreeNode extends Serializable {
var children = Seq[TreeNode]()
def addChild(expr: TreeNode) = { children :+= expr }
def addChildren(exprs: Seq[TreeNode]) = { children ++= exprs }
- def preOrderTraverseDepthFirst[T, A <: TreeNode](z: T)(seqOp: (A, T) =>
T, combOp: (T, T) => T): T = {
-if (this.isInstanceOf[A]) {
- val tv = seqOp(this.asInstanceOf[A], z)
- children.foldLeft(combOp(z, tv)) { (ov, tn) =>
-combOp(ov, tn.preOrderTraverseDepthFirst(z)(seqOp, combOp))
+ def preOrderTraverseDepthFirst[T, A <: TreeNode](z: T)(seqOp: (A, T) =>
T, combOp: (T, T) => T)(implicit tag: ClassTag[A]): T = {
+
+val clazz = tag.runtimeClass
+
+this.getClass match {
+ case `clazz` => {
--- End diff --
@bhlx3lyx7 @grant-xuexu we will append a test unit for this.
---
[GitHub] incubator-griffin pull request #396: set the fields in configuration classes...
Github user guoyuepeng commented on a diff in the pull request:
https://github.com/apache/incubator-griffin/pull/396#discussion_r210854573
--- Diff:
measure/src/main/scala/org/apache/griffin/measure/step/builder/dsl/expr/TreeNode.scala
---
@@ -18,28 +18,42 @@ under the License.
*/
package org.apache.griffin.measure.step.builder.dsl.expr
+import scala.reflect.ClassTag
+
trait TreeNode extends Serializable {
var children = Seq[TreeNode]()
def addChild(expr: TreeNode) = { children :+= expr }
def addChildren(exprs: Seq[TreeNode]) = { children ++= exprs }
- def preOrderTraverseDepthFirst[T, A <: TreeNode](z: T)(seqOp: (A, T) =>
T, combOp: (T, T) => T): T = {
-if (this.isInstanceOf[A]) {
- val tv = seqOp(this.asInstanceOf[A], z)
- children.foldLeft(combOp(z, tv)) { (ov, tn) =>
-combOp(ov, tn.preOrderTraverseDepthFirst(z)(seqOp, combOp))
+ def preOrderTraverseDepthFirst[T, A <: TreeNode](z: T)(seqOp: (A, T) =>
T, combOp: (T, T) => T)(implicit tag: ClassTag[A]): T = {
+
+val clazz = tag.runtimeClass
+
+this.getClass match {
+ case `clazz` => {
--- End diff --
we cannot use this patch, please remove this .
---
[GitHub] incubator-griffin pull request #396: set the fields in configuration classes...
Github user bhlx3lyx7 commented on a diff in the pull request:
https://github.com/apache/incubator-griffin/pull/396#discussion_r210843748
--- Diff:
measure/src/main/scala/org/apache/griffin/measure/step/builder/dsl/expr/TreeNode.scala
---
@@ -18,28 +18,42 @@ under the License.
*/
package org.apache.griffin.measure.step.builder.dsl.expr
+import scala.reflect.ClassTag
+
trait TreeNode extends Serializable {
var children = Seq[TreeNode]()
def addChild(expr: TreeNode) = { children :+= expr }
def addChildren(exprs: Seq[TreeNode]) = { children ++= exprs }
- def preOrderTraverseDepthFirst[T, A <: TreeNode](z: T)(seqOp: (A, T) =>
T, combOp: (T, T) => T): T = {
-if (this.isInstanceOf[A]) {
- val tv = seqOp(this.asInstanceOf[A], z)
- children.foldLeft(combOp(z, tv)) { (ov, tn) =>
-combOp(ov, tn.preOrderTraverseDepthFirst(z)(seqOp, combOp))
+ def preOrderTraverseDepthFirst[T, A <: TreeNode](z: T)(seqOp: (A, T) =>
T, combOp: (T, T) => T)(implicit tag: ClassTag[A]): T = {
+
+val clazz = tag.runtimeClass
+
+this.getClass match {
+ case `clazz` => {
--- End diff --
The code means this.getClass and T must be the same class. But in the
usage, it traverses among different types of tree nodes, their concrete classes
can't be matched to their parent class, which is not what we want.
I doubt the modification of this file will destroy the process, I suggest
you write a unit test for this.
---
[GitHub] incubator-griffin pull request #396: set the fields in configuration classes...
Github user bhlx3lyx7 commented on a diff in the pull request:
https://github.com/apache/incubator-griffin/pull/396#discussion_r210831681
--- Diff:
measure/src/main/scala/org/apache/griffin/measure/datasource/connector/DataConnector.scala
---
@@ -33,7 +33,7 @@ import org.apache.spark.sql.functions._
trait DataConnector extends Loggable with Serializable {
- @transient val sparkSession: SparkSession
+ val sparkSession: SparkSession
--- End diff --
SparkSession is Serializable, so it should works here.
---
[GitHub] incubator-griffin pull request #396: set the fields in configuration classes...
Github user bhlx3lyx7 commented on a diff in the pull request:
https://github.com/apache/incubator-griffin/pull/396#discussion_r210831530
--- Diff:
measure/src/main/scala/org/apache/griffin/measure/datasource/connector/DataConnectorFactory.scala
---
@@ -91,11 +91,12 @@ object DataConnectorFactory extends Loggable {
): KafkaStreamingDataConnector = {
val KeyType = "key.type"
val ValueType = "value.type"
-val config = dcParam.config
+val config = dcParam.getConfig
val keyType = config.getOrElse(KeyType, "java.lang.String").toString
val valueType = config.getOrElse(ValueType,
"java.lang.String").toString
-(getClassTag(keyType), getClassTag(valueType)) match {
- case (ClassTag(k: Class[String]), ClassTag(v: Class[String])) => {
+
+(keyType, valueType) match {
+ case ("java.lang.String", "java.lang.String") => {
KafkaStreamingStringDataConnector(sparkSession, ssc, dcParam,
tmstCache, streamingCacheClientOpt)
--- End diff --
I think match with literal string is too strong with limitation, why not
match with the class type?
---
[GitHub] incubator-griffin issue #393: Update deploy-guide.md
Github user bhlx3lyx7 commented on the issue: https://github.com/apache/incubator-griffin/pull/393 seems to be updated with latest versions, thanks @SparshSinghalHM ---
