You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@flink.apache.org by sunjincheng121 <gi...@git.apache.org> on 2017/07/06 16:48:42 UTC
[GitHub] flink pull request #4274: [FLINK-6975][table]Add CONCAT/CONCAT_WS supported ...
GitHub user sunjincheng121 opened a pull request:
https://github.com/apache/flink/pull/4274
[FLINK-6975][table]Add CONCAT/CONCAT_WS supported in TableAPI
In this PR. I have Add CONCAT/CONCAT_WS supported in TableAPI.
- [x] General
- The pull request references the related JIRA issue ("[FLINK-6975][table]Add CONCAT/CONCAT_WS supported in TableAPI")
- The pull request addresses only one issue
- Each commit in the PR has a meaningful commit message (including the JIRA id)
- [x] Documentation
- Documentation has been added for new functionality
- Old documentation affected by the pull request has been updated
- JavaDoc for public methods has been added
- [x] Tests & Build
- Functionality added by the pull request is covered by tests
- `mvn clean verify` has been executed successfully locally or a Travis build has passed
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/sunjincheng121/flink FLINK-6975-PR
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/flink/pull/4274.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 #4274
----
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #4274: [FLINK-6975][table]Add CONCAT/CONCAT_WS supported ...
Posted by wuchong <gi...@git.apache.org>.
Github user wuchong commented on a diff in the pull request:
https://github.com/apache/flink/pull/4274#discussion_r126871619
--- Diff: flink-libraries/flink-table/src/main/scala/org/apache/flink/table/expressions/stringExpressions.scala ---
@@ -277,3 +278,47 @@ case class Overlay(
position.toRexNode)
}
}
+
+/**
+ * Returns the string that results from concatenating the arguments.
+ * Returns NULL if any argument is NULL.
+ */
+case class Concat(strings: Seq[Expression]) extends Expression with InputTypeSpec {
+
+ override private[flink] def children: Seq[Expression] = strings
+
+ override private[flink] def resultType: TypeInformation[_] = BasicTypeInfo.STRING_TYPE_INFO
+
+ override private[flink] def expectedTypes: Seq[TypeInformation[_]] = STRING_TYPE_INFO :: Nil
+
+ override def toString: String = s"Concat($strings)"
+
+
--- End diff --
remove empty line
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #4274: [FLINK-6975][table]Add CONCAT/CONCAT_WS supported ...
Posted by wuchong <gi...@git.apache.org>.
Github user wuchong commented on a diff in the pull request:
https://github.com/apache/flink/pull/4274#discussion_r126870651
--- Diff: flink-libraries/flink-table/src/main/scala/org/apache/flink/table/api/scala/expressionDsl.scala ---
@@ -982,4 +982,27 @@ object randInteger {
}
}
+/**
+ * Returns the string that results from concatenating the arguments.
+ * Returns NULL if any argument is NULL.
+ */
+object concat {
+ def apply(strings: Expression*): Expression = {
--- End diff --
I would suggest to change this signature to `apply(first: Expression, second: Expression, rest: Expression*)` to avoid misusage such as `concat()`.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink issue #4274: [FLINK-6975][table]Add CONCAT/CONCAT_WS supported in Tabl...
Posted by sunjincheng121 <gi...@git.apache.org>.
Github user sunjincheng121 commented on the issue:
https://github.com/apache/flink/pull/4274
Hi @wuchong I appreciate if you can take look at the changes of this PR's.
Thanks, Jincheng
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink issue #4274: [FLINK-6975][table]Add CONCAT/CONCAT_WS supported in Tabl...
Posted by wuchong <gi...@git.apache.org>.
Github user wuchong commented on the issue:
https://github.com/apache/flink/pull/4274
+1 to merge.
I will merge this in this weekend.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #4274: [FLINK-6975][table]Add CONCAT/CONCAT_WS supported ...
Posted by wuchong <gi...@git.apache.org>.
Github user wuchong commented on a diff in the pull request:
https://github.com/apache/flink/pull/4274#discussion_r126871598
--- Diff: flink-libraries/flink-table/src/main/scala/org/apache/flink/table/expressions/stringExpressions.scala ---
@@ -277,3 +278,47 @@ case class Overlay(
position.toRexNode)
}
}
+
+/**
+ * Returns the string that results from concatenating the arguments.
+ * Returns NULL if any argument is NULL.
+ */
+case class Concat(strings: Seq[Expression]) extends Expression with InputTypeSpec {
+
+ override private[flink] def children: Seq[Expression] = strings
+
+ override private[flink] def resultType: TypeInformation[_] = BasicTypeInfo.STRING_TYPE_INFO
+
+ override private[flink] def expectedTypes: Seq[TypeInformation[_]] = STRING_TYPE_INFO :: Nil
+
+ override def toString: String = s"Concat($strings)"
+
+
+ override private[flink] def toRexNode(implicit relBuilder: RelBuilder): RexNode = {
+ relBuilder.call(ScalarSqlFunctions.CONCAT, children.map(_.toRexNode))
+ }
+}
+
+/**
+ * Returns the string that results from concatenating the arguments and separator.
+ * Returns NULL If the separator is NULL.
+ *
+ * Note: this user-defined function does not skip empty strings. However, it does skip any NULL
+ * values after the separator argument.
+ **/
+case class ConcatWs(separator: Expression, strings: Seq[Expression])
+ extends Expression with InputTypeSpec {
+
+ override private[flink] def children: Seq[Expression] = Seq(separator) ++ strings
+
+ override private[flink] def resultType: TypeInformation[_] = BasicTypeInfo.STRING_TYPE_INFO
+
+ override private[flink] def expectedTypes: Seq[TypeInformation[_]] = STRING_TYPE_INFO :: Nil
+
+ override def toString: String = s"ConcatWs($separator, $strings)"
+
+
--- End diff --
remove empty line
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #4274: [FLINK-6975][table]Add CONCAT/CONCAT_WS supported ...
Posted by wuchong <gi...@git.apache.org>.
Github user wuchong commented on a diff in the pull request:
https://github.com/apache/flink/pull/4274#discussion_r127382075
--- Diff: flink-libraries/flink-table/src/main/scala/org/apache/flink/table/expressions/stringExpressions.scala ---
@@ -277,3 +278,47 @@ case class Overlay(
position.toRexNode)
}
}
+
+/**
+ * Returns the string that results from concatenating the arguments.
+ * Returns NULL if any argument is NULL.
+ */
+case class Concat(strings: Seq[Expression]) extends Expression with InputTypeSpec {
+
+ override private[flink] def children: Seq[Expression] = strings
+
+ override private[flink] def resultType: TypeInformation[_] = BasicTypeInfo.STRING_TYPE_INFO
+
+ override private[flink] def expectedTypes: Seq[TypeInformation[_]] =
+ children.map(_ => STRING_TYPE_INFO)
+
+ override def toString: String = s"concat($strings)"
+
+ override private[flink] def toRexNode(implicit relBuilder: RelBuilder): RexNode = {
+ relBuilder.call(ScalarSqlFunctions.CONCAT, children.map(_.toRexNode))
+ }
+}
+
+/**
+ * Returns the string that results from concatenating the arguments and separator.
+ * Returns NULL If the separator is NULL.
+ *
+ * Note: this user-defined function does not skip empty strings. However, it does skip any NULL
+ * values after the separator argument.
+ **/
+case class ConcatWs(separator: Expression, strings: Seq[Expression])
--- End diff --
What about make this signature to `args: Seq[Expression]`, which combines `separator` and `strings` before construct `ConcatWs`. So that we do not need to change the FunctionCatalog. I think it's fine, because `ConcatWs` is not used by users.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #4274: [FLINK-6975][table]Add CONCAT/CONCAT_WS supported ...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/flink/pull/4274
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #4274: [FLINK-6975][table]Add CONCAT/CONCAT_WS supported ...
Posted by wuchong <gi...@git.apache.org>.
Github user wuchong commented on a diff in the pull request:
https://github.com/apache/flink/pull/4274#discussion_r126870614
--- Diff: flink-libraries/flink-table/src/main/scala/org/apache/flink/table/api/scala/expressionDsl.scala ---
@@ -982,4 +982,27 @@ object randInteger {
}
}
+/**
+ * Returns the string that results from concatenating the arguments.
+ * Returns NULL if any argument is NULL.
+ */
+object concat {
+ def apply(strings: Expression*): Expression = {
+ new Concat(strings)
+ }
+}
+
+/**
+ * Returns the string that results from concatenating the arguments and separator.
+ * Returns NULL If the separator is NULL.
+ *
+ * Note: this user-defined function does not skip empty strings. However, it does skip any NULL
+ * values after the separator argument.
+ **/
+object concat_ws {
+ def apply(separator: Expression, strings: Expression*): Expression = {
--- End diff --
I would suggest to change this signature to `apply(separator: Expression, first: Expression, second: Expression, rest: Expression*)` to avoid misusage such as `concat_ws(",")`.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink issue #4274: [FLINK-6975][table]Add CONCAT/CONCAT_WS supported in Tabl...
Posted by sunjincheng121 <gi...@git.apache.org>.
Github user sunjincheng121 commented on the issue:
https://github.com/apache/flink/pull/4274
@wuchong Thanks for your reviewing. I have update the PR according your comments.
Thanks, Jincheng
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #4274: [FLINK-6975][table]Add CONCAT/CONCAT_WS supported ...
Posted by wuchong <gi...@git.apache.org>.
Github user wuchong commented on a diff in the pull request:
https://github.com/apache/flink/pull/4274#discussion_r126881444
--- Diff: flink-libraries/flink-table/src/main/scala/org/apache/flink/table/expressions/stringExpressions.scala ---
@@ -277,3 +278,47 @@ case class Overlay(
position.toRexNode)
}
}
+
+/**
+ * Returns the string that results from concatenating the arguments.
+ * Returns NULL if any argument is NULL.
+ */
+case class Concat(strings: Seq[Expression]) extends Expression with InputTypeSpec {
+
+ override private[flink] def children: Seq[Expression] = strings
+
+ override private[flink] def resultType: TypeInformation[_] = BasicTypeInfo.STRING_TYPE_INFO
+
+ override private[flink] def expectedTypes: Seq[TypeInformation[_]] = STRING_TYPE_INFO :: Nil
--- End diff --
The `expectedTypes` should be the same size as children with `STRING_TYPE_INFO`.
All the tests passed, because there is a bug in `InputTypeSpec#validateInput` which do not check the size of `expectedTypes` and `children`. It would be great if we can fix it in this PR.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #4274: [FLINK-6975][table]Add CONCAT/CONCAT_WS supported ...
Posted by wuchong <gi...@git.apache.org>.
Github user wuchong commented on a diff in the pull request:
https://github.com/apache/flink/pull/4274#discussion_r126896677
--- Diff: flink-libraries/flink-table/src/main/scala/org/apache/flink/table/validate/FunctionCatalog.scala ---
@@ -107,6 +107,18 @@ class FunctionCatalog {
val function = aggregateFunction.getFunction
AggFunctionCall(function, children)
+ // concat_ws
+ case concat_ws if classOf[ConcatWs].isAssignableFrom(concat_ws) =>
--- End diff --
I'm not sure whether this is a good way. I prefer to add an additional try (for constructor `classOf[Expression], classOf[Seq[_]]`) in general expression construction.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #4274: [FLINK-6975][table]Add CONCAT/CONCAT_WS supported ...
Posted by wuchong <gi...@git.apache.org>.
Github user wuchong commented on a diff in the pull request:
https://github.com/apache/flink/pull/4274#discussion_r126881414
--- Diff: flink-libraries/flink-table/src/main/scala/org/apache/flink/table/expressions/stringExpressions.scala ---
@@ -277,3 +278,47 @@ case class Overlay(
position.toRexNode)
}
}
+
+/**
+ * Returns the string that results from concatenating the arguments.
+ * Returns NULL if any argument is NULL.
+ */
+case class Concat(strings: Seq[Expression]) extends Expression with InputTypeSpec {
+
+ override private[flink] def children: Seq[Expression] = strings
+
+ override private[flink] def resultType: TypeInformation[_] = BasicTypeInfo.STRING_TYPE_INFO
+
+ override private[flink] def expectedTypes: Seq[TypeInformation[_]] = STRING_TYPE_INFO :: Nil
+
+ override def toString: String = s"Concat($strings)"
+
+
+ override private[flink] def toRexNode(implicit relBuilder: RelBuilder): RexNode = {
+ relBuilder.call(ScalarSqlFunctions.CONCAT, children.map(_.toRexNode))
+ }
+}
+
+/**
+ * Returns the string that results from concatenating the arguments and separator.
+ * Returns NULL If the separator is NULL.
+ *
+ * Note: this user-defined function does not skip empty strings. However, it does skip any NULL
+ * values after the separator argument.
+ **/
+case class ConcatWs(separator: Expression, strings: Seq[Expression])
+ extends Expression with InputTypeSpec {
+
+ override private[flink] def children: Seq[Expression] = Seq(separator) ++ strings
+
+ override private[flink] def resultType: TypeInformation[_] = BasicTypeInfo.STRING_TYPE_INFO
+
+ override private[flink] def expectedTypes: Seq[TypeInformation[_]] = STRING_TYPE_INFO :: Nil
--- End diff --
The `expectedTypes` should be the same size as children with `STRING_TYPE_INFO`.
All the tests passed, because there is a bug in `InputTypeSpec#validateInput` which do not check the size of `expectedTypes` and `children`. It would be great if we can fix it in this PR.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---