You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@flink.apache.org by Xpray <gi...@git.apache.org> on 2018/05/17 03:31:28 UTC
[GitHub] flink pull request #6027: [FLINK-7814][TableAPI && SQL] Add BETWEEN and NOT ...
GitHub user Xpray opened a pull request:
https://github.com/apache/flink/pull/6027
[FLINK-7814][TableAPI && SQL] Add BETWEEN and NOT BETWEEN expression to Table API
## What is the purpose of the change
Add BETWEEN expression to Table API
## Brief change log
* add 'between' and 'not between' to TableAPI
## Verifying this change
This change added tests and can be verified as follows:
```org.apache.flink.table.expressions.ScalarOperatorsTest```
## Does this pull request potentially affect one of the following parts:
- Dependencies (does it add or upgrade a dependency): no
- The public API, i.e., is any changed class annotated with `@Public(Evolving)`: no
- The serializers: no
- The runtime per-record code paths (performance sensitive): no
- Anything that affects deployment or recovery: JobManager (and its components), Checkpointing, Yarn/Mesos, ZooKeeper: no
- The S3 file system connector: no
## Documentation
- Does this pull request introduce a new feature? no
- If yes, how is the feature documented? not documented
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/Xpray/flink FLINK-7814
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/flink/pull/6027.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 #6027
----
commit d2908cc91c4c0b9cdd24f30870dfd9f8e1c64543
Author: Xpray <le...@...>
Date: 2018-05-17T03:25:12Z
[FLINK-7814][TableAPI && SQL] Add BETWEEN expression to Table API
----
---
[GitHub] flink pull request #6027: [FLINK-7814][TableAPI && SQL] Add BETWEEN and NOT ...
Posted by xccui <gi...@git.apache.org>.
Github user xccui commented on a diff in the pull request:
https://github.com/apache/flink/pull/6027#discussion_r188891077
--- Diff: flink-libraries/flink-table/src/main/scala/org/apache/flink/table/api/scala/expressionDsl.scala ---
@@ -809,6 +809,23 @@ trait ImplicitExpressionOperations {
*/
def sha2(hashLength: Expression) = Sha2(expr, hashLength)
+ /**
+ * Returns the Between with lower bound and upper bound.
--- End diff --
Add a new line here and describe whether the bound values are included in the interval.
---
[GitHub] flink pull request #6027: [FLINK-7814][TableAPI && SQL] Add BETWEEN and NOT ...
Posted by xccui <gi...@git.apache.org>.
Github user xccui commented on a diff in the pull request:
https://github.com/apache/flink/pull/6027#discussion_r188895005
--- Diff: flink-libraries/flink-table/src/test/scala/org/apache/flink/table/expressions/ScalarOperatorsTest.scala ---
@@ -406,5 +406,26 @@ class ScalarOperatorsTest extends ScalarOperatorsTestBase {
"((((true) === true) || false).cast(STRING) + 'X ').trim",
"trueX")
testTableApi(12.isNull, "12.isNull", "false")
+
+ // between
+ testTableApi(2.between(1, 3), "2.BETWEEN(1, 3)", "true")
--- End diff --
Would be better to add some tests for boundaries here.
---
[GitHub] flink pull request #6027: [FLINK-7814][TableAPI && SQL] Add BETWEEN and NOT ...
Posted by twalthr <gi...@git.apache.org>.
Github user twalthr commented on a diff in the pull request:
https://github.com/apache/flink/pull/6027#discussion_r189895702
--- Diff: flink-libraries/flink-table/src/main/scala/org/apache/flink/table/expressions/logic.scala ---
@@ -105,3 +105,75 @@ case class If(
}
}
}
+
+abstract class BetweenProperty(
--- End diff --
Move this to `comparison.scala`.
---
[GitHub] flink pull request #6027: [FLINK-7814][TableAPI && SQL] Add BETWEEN and NOT ...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/flink/pull/6027
---
[GitHub] flink pull request #6027: [FLINK-7814][TableAPI && SQL] Add BETWEEN and NOT ...
Posted by xccui <gi...@git.apache.org>.
Github user xccui commented on a diff in the pull request:
https://github.com/apache/flink/pull/6027#discussion_r188891227
--- Diff: flink-libraries/flink-table/src/main/scala/org/apache/flink/table/api/scala/expressionDsl.scala ---
@@ -809,6 +809,23 @@ trait ImplicitExpressionOperations {
*/
def sha2(hashLength: Expression) = Sha2(expr, hashLength)
+ /**
+ * Returns the Between with lower bound and upper bound.
+ * @param lowerBound
+ * @param upperBound
+ * @return Returns the Between with lower bound and upper bound
+ */
+ def between(lowerBound: Expression, upperBound: Expression) =
+ Between(expr, lowerBound, upperBound)
+
+ /**
+ * Returns the not Between with lower bound and upper bound.
--- End diff --
Same with the comment above.
---
[GitHub] flink pull request #6027: [FLINK-7814][TableAPI && SQL] Add BETWEEN and NOT ...
Posted by twalthr <gi...@git.apache.org>.
Github user twalthr commented on a diff in the pull request:
https://github.com/apache/flink/pull/6027#discussion_r189895031
--- Diff: flink-libraries/flink-table/src/main/scala/org/apache/flink/table/expressions/ExpressionParser.scala ---
@@ -210,6 +212,14 @@ object ExpressionParser extends JavaTokenParsers with PackratParsers {
case agg ~ _ ~ windowRef => UnresolvedOverCall(agg, windowRef)
}
+ lazy val between: PackratParser[Expression] =
+ expression ~ "." ~ (BETWEEN | NOT_BETWEEN) ~ "(" ~ expression ~ "," ~ expression ~ ")" ^^ {
--- End diff --
The changes in `ExpressionParser` should not be necessary if you add the expression classes to `org.apache.flink.table.validate.FunctionCatalog`. What happens if you remove these changes?
---
[GitHub] flink pull request #6027: [FLINK-7814][TableAPI && SQL] Add BETWEEN and NOT ...
Posted by xccui <gi...@git.apache.org>.
Github user xccui commented on a diff in the pull request:
https://github.com/apache/flink/pull/6027#discussion_r188894442
--- Diff: flink-libraries/flink-table/src/main/scala/org/apache/flink/table/expressions/logic.scala ---
@@ -105,3 +105,75 @@ case class If(
}
}
}
+
+abstract class BetweenProperty(
+ expr: Expression,
+ lowerBound: Expression,
+ upperBound: Expression) extends Expression {
+
+ override private[flink] def resultType: TypeInformation[_] = BasicTypeInfo.BOOLEAN_TYPE_INFO
+
+ override private[flink] def children: Seq[Expression] = Seq(expr, lowerBound, upperBound)
+
+ override private[flink] def validateInput(): ValidationResult = {
+ (expr.resultType, lowerBound.resultType, upperBound.resultType) match {
+ case (exprType, lowerType, upperType)
+ if isNumeric(exprType) && isNumeric(lowerType) && isNumeric(upperType)
+ => ValidationSuccess
+ case (exprType, lowerType, upperType)
+ if isComparable(exprType) && exprType == lowerType && exprType == upperType
+ => ValidationSuccess
+ case (exprType, lowerType, upperType) =>
+ ValidationFailure(
+ s"Between is only supported for numeric types and " +
+ s"comparable types of same type, got $exprType, $lowerType and $upperType"
--- End diff --
Between is only supported for numeric types and identical comparable types, but got $exprType, $lowerType and $upperType.
---
[GitHub] flink pull request #6027: [FLINK-7814][TableAPI && SQL] Add BETWEEN and NOT ...
Posted by twalthr <gi...@git.apache.org>.
Github user twalthr commented on a diff in the pull request:
https://github.com/apache/flink/pull/6027#discussion_r189896704
--- Diff: docs/dev/table/tableApi.md ---
@@ -1885,6 +1885,32 @@ BOOLEAN.isNotFalse
</td>
</tr>
+ <tr>
+ <td>
+ {% highlight java %}
+ ANY.BETWEEN(LOWER_BOUND, UPPER_BOUND)
--- End diff --
Please use camel-case syntax here.
---
[GitHub] flink issue #6027: [FLINK-7814][TableAPI && SQL] Add BETWEEN and NOT BETWEEN...
Posted by twalthr <gi...@git.apache.org>.
Github user twalthr commented on the issue:
https://github.com/apache/flink/pull/6027
Thanks for the update @Xpray. I will merge this...
---
[GitHub] flink pull request #6027: [FLINK-7814][TableAPI && SQL] Add BETWEEN and NOT ...
Posted by xccui <gi...@git.apache.org>.
Github user xccui commented on a diff in the pull request:
https://github.com/apache/flink/pull/6027#discussion_r188895939
--- Diff: flink-libraries/flink-table/src/test/scala/org/apache/flink/table/expressions/ScalarOperatorsTest.scala ---
@@ -406,5 +406,26 @@ class ScalarOperatorsTest extends ScalarOperatorsTestBase {
"((((true) === true) || false).cast(STRING) + 'X ').trim",
"trueX")
testTableApi(12.isNull, "12.isNull", "false")
+
--- End diff --
I think we can use a separate test method for `Between`. Besides, some validation tests can be added to `org.apache.flink.table.expressions.validation.ScalarOperatorsValidationTest`.
---
[GitHub] flink issue #6027: [FLINK-7814][TableAPI && SQL] Add BETWEEN and NOT BETWEEN...
Posted by Xpray <gi...@git.apache.org>.
Github user Xpray commented on the issue:
https://github.com/apache/flink/pull/6027
Thanks for the review @xccui , I've updated the PR.
Best.
---
[GitHub] flink issue #6027: [FLINK-7814][TableAPI && SQL] Add BETWEEN and NOT BETWEEN...
Posted by Xpray <gi...@git.apache.org>.
Github user Xpray commented on the issue:
https://github.com/apache/flink/pull/6027
@twalthr I've updated the PR.
Best.
---