[GitHub] flink pull request: FLINK-3579 Improve String concatenation (Ram)
Github user asfgit closed the pull request at: https://github.com/apache/flink/pull/1821 --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] flink pull request: FLINK-3579 Improve String concatenation (Ram)
Github user vasia commented on the pull request: https://github.com/apache/flink/pull/1821#issuecomment-208354607 I'm merging this. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] flink pull request: FLINK-3579 Improve String concatenation (Ram)
Github user ramkrish86 commented on the pull request: https://github.com/apache/flink/pull/1821#issuecomment-206399165 There is 1 failing check. SavepointCoordinatorTest.testAbortOnCheckpointTimeout:517 expected:<0> but was:<1> Seems unrelated to this change. Thanks for all the reviews @tillrohrmann . Should be good to merge then. Thank you. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] flink pull request: FLINK-3579 Improve String concatenation (Ram)
Github user tillrohrmann commented on the pull request: https://github.com/apache/flink/pull/1821#issuecomment-206257315 Changes look good to me :-) Good work @ramkrish86! --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] flink pull request: FLINK-3579 Improve String concatenation (Ram)
Github user ramkrish86 commented on the pull request: https://github.com/apache/flink/pull/1821#issuecomment-206137193 @tillrohrmann - Ping to review the PR. https://issues.apache.org/jira/browse/FLINK-3086 is already available for the support of CAST and abs() with ints. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] flink pull request: FLINK-3579 Improve String concatenation (Ram)
Github user ramkrish86 commented on the pull request: https://github.com/apache/flink/pull/1821#issuecomment-205186160 Thank you @tillrohrmann . Let me if the changes in the updated PR is fine with you. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] flink pull request: FLINK-3579 Improve String concatenation (Ram)
Github user tillrohrmann commented on the pull request: https://github.com/apache/flink/pull/1821#issuecomment-205177788 Thanks for updating the PR @ramkrish86. Your proposed approach (creating a new PR for the parser changes) would be fine with me. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] flink pull request: FLINK-3579 Improve String concatenation (Ram)
Github user ramkrish86 commented on the pull request: https://github.com/apache/flink/pull/1821#issuecomment-205139553 I have done the changes only to support the String concat operation. The cast() and abs() grammar change looks more complex and I need to understand things better in parser to support it. Suggest we do it in a seperate JIRA? Is that fine @tillrohrmann ? --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] flink pull request: FLINK-3579 Improve String concatenation (Ram)
Github user ramkrish86 commented on a diff in the pull request:
https://github.com/apache/flink/pull/1821#discussion_r58191031
--- Diff:
flink-libraries/flink-table/src/main/scala/org/apache/flink/api/table/expressions/ExpressionParser.scala
---
@@ -59,6 +59,8 @@ object ExpressionParser extends JavaTokenParsers with
PackratParsers {
Literal(str.toInt)
} else if (str.endsWith("f") | str.endsWith("F")) {
Literal(str.toFloat)
+} else if (str.endsWith(".")) {
--- End diff --
@tillrohrmann
This '.' related change I think I need more time to understand how this
parser works. So for this PR can i only handle the string concat part. I have
ensured that 42+a+b or a + b + 42 works fine.
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request: FLINK-3579 Improve String concatenation (Ram)
Github user tillrohrmann commented on a diff in the pull request:
https://github.com/apache/flink/pull/1821#discussion_r58029854
--- Diff:
flink-libraries/flink-table/src/main/scala/org/apache/flink/api/table/expressions/ExpressionParser.scala
---
@@ -59,6 +59,8 @@ object ExpressionParser extends JavaTokenParsers with
PackratParsers {
Literal(str.toInt)
} else if (str.endsWith("f") | str.endsWith("F")) {
Literal(str.toFloat)
+} else if (str.endsWith(".")) {
--- End diff --
Of course both are valid expressions but `1.` is simply not a valid number
literal. Instead `1` would be valid. I guess you should change your grammar
such that `1.abs()` will parsed into something like
`functionApplication(NumberLiteral(1), abs)` and the `.` indicates the function
application but is not part of the number literal token.
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request: FLINK-3579 Improve String concatenation (Ram)
Github user ramkrish86 commented on a diff in the pull request:
https://github.com/apache/flink/pull/1821#discussion_r57930954
--- Diff:
flink-libraries/flink-table/src/main/scala/org/apache/flink/api/table/expressions/ExpressionParser.scala
---
@@ -59,6 +59,8 @@ object ExpressionParser extends JavaTokenParsers with
PackratParsers {
Literal(str.toInt)
} else if (str.endsWith("f") | str.endsWith("F")) {
Literal(str.toFloat)
+} else if (str.endsWith(".")) {
--- End diff --
So in that sense what is the valid number expression? I cannot say
1.CAST(STRING) then?
If that is not valid then 1.abs() is also not vaild right?
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request: FLINK-3579 Improve String concatenation (Ram)
Github user ramkrish86 commented on the pull request:
https://github.com/apache/flink/pull/1821#issuecomment-203530162
Sorry for the wrong observation.
I can see that 'a + 42l.abs().cast(STRING)' works fine.
But 'a + 42.3.abs().cast(STRING)' does not work.
It fails here
` case DOUBLE =>
val decimal = BigDecimal(value.asInstanceOf[java.math.BigDecimal])
if (decimal.isValidDouble) {
generateNonNullLiteral(resultType, decimal.doubleValue().toString)
}
else {
throw new CodeGenException("Decimal can not be converted to
double.")
}`
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request: FLINK-3579 Improve String concatenation (Ram)
Github user ramkrish86 commented on a diff in the pull request:
https://github.com/apache/flink/pull/1821#discussion_r57921166
--- Diff:
flink-libraries/flink-table/src/test/java/org/apache/flink/api/java/table/test/StringExpressionsITCase.java
---
@@ -121,6 +120,66 @@ public void testNonWorkingSubstring2() throws
Exception {
resultSet.collect();
}
+ @Test
+ public void testStringConcat() throws Exception {
+ ExecutionEnvironment env =
ExecutionEnvironment.getExecutionEnvironment();
+ TableEnvironment tableEnv = new TableEnvironment();
+
+ DataSet> ds = env.fromElements(
+ new Tuple2<>("ABCD", 3),
+ new Tuple2<>("ABCD", 2));
+
+ Table in = tableEnv.fromDataSet(ds, "a, b");
+
+ Table result = in
+ .select("a + b + 42");
--- End diff --
`What happens if you do "42 + a" or even "42 + b + a"?`
Am able to make the above work.
`a + 42.abs().cast(STRING)`
I think as you said 42.abs() also does not work leave alone the .CAST added
to it.
abs() does not work because it is not added in the ExpressionParser.scala
itself. Reading the comment on ImplicitExpressionOperations i can see that the
expressionDsl.scala and ExpressionParser should be in sync. You want me to add
support for abs in this patch only? Let me know.
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request: FLINK-3579 Improve String concatenation (Ram)
Github user tillrohrmann commented on a diff in the pull request:
https://github.com/apache/flink/pull/1821#discussion_r57896997
--- Diff:
flink-libraries/flink-table/src/test/java/org/apache/flink/api/java/table/test/StringExpressionsITCase.java
---
@@ -121,6 +120,66 @@ public void testNonWorkingSubstring2() throws
Exception {
resultSet.collect();
}
+ @Test
+ public void testStringConcat() throws Exception {
+ ExecutionEnvironment env =
ExecutionEnvironment.getExecutionEnvironment();
+ TableEnvironment tableEnv = new TableEnvironment();
+
+ DataSet> ds = env.fromElements(
+ new Tuple2<>("ABCD", 3),
+ new Tuple2<>("ABCD", 2));
+
+ Table in = tableEnv.fromDataSet(ds, "a, b");
+
+ Table result = in
+ .select("a + b + 42");
--- End diff --
Just out of curiosity and because I'm at a loss here, why does `a +
42.abs().cast(STRING)` does not work?
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request: FLINK-3579 Improve String concatenation (Ram)
Github user tillrohrmann commented on a diff in the pull request:
https://github.com/apache/flink/pull/1821#discussion_r57896311
--- Diff:
flink-libraries/flink-table/src/main/scala/org/apache/flink/api/table/expressions/ExpressionParser.scala
---
@@ -59,6 +59,8 @@ object ExpressionParser extends JavaTokenParsers with
PackratParsers {
Literal(str.toInt)
} else if (str.endsWith("f") | str.endsWith("F")) {
Literal(str.toFloat)
+} else if (str.endsWith(".")) {
--- End diff --
Seems a little bit like a hack to me. Ideally, you should design your
grammar such that you won't see the token `1.` if it is not a valid number
expression.
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request: FLINK-3579 Improve String concatenation (Ram)
Github user twalthr commented on a diff in the pull request:
https://github.com/apache/flink/pull/1821#discussion_r57876138
--- Diff:
flink-libraries/flink-table/src/test/java/org/apache/flink/api/java/table/test/StringExpressionsITCase.java
---
@@ -121,6 +120,66 @@ public void testNonWorkingSubstring2() throws
Exception {
resultSet.collect();
}
+ @Test
+ public void testStringConcat() throws Exception {
+ ExecutionEnvironment env =
ExecutionEnvironment.getExecutionEnvironment();
+ TableEnvironment tableEnv = new TableEnvironment();
+
+ DataSet> ds = env.fromElements(
+ new Tuple2<>("ABCD", 3),
+ new Tuple2<>("ABCD", 2));
+
+ Table in = tableEnv.fromDataSet(ds, "a, b");
+
+ Table result = in
+ .select("a + b + 42");
--- End diff --
Maybe you need to rethink your approach. Expressions like `a +
42.abs().cast(STRING)` also do not work. It might be necessary to introduce a
modified grammar where the dot is special symbol followed by symbols for
`cast`, functions etc. without a dot.
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request: FLINK-3579 Improve String concatenation (Ram)
Github user ramkrish86 commented on a diff in the pull request:
https://github.com/apache/flink/pull/1821#discussion_r57844972
--- Diff:
flink-libraries/flink-table/src/main/scala/org/apache/flink/api/table/expressions/ExpressionParser.scala
---
@@ -59,6 +59,8 @@ object ExpressionParser extends JavaTokenParsers with
PackratParsers {
Literal(str.toInt)
} else if (str.endsWith("f") | str.endsWith("F")) {
Literal(str.toFloat)
+} else if (str.endsWith(".")) {
--- End diff --
Since the 'cast()' function parsing is accepting '.' as '.cast()' we need
to do this way. Else the parser does not know that '.' is specific to that
operation of 'cast'. But I am not so well versed with this parser so
suggestions welcome.
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request: FLINK-3579 Improve String concatenation (Ram)
Github user ramkrish86 commented on a diff in the pull request:
https://github.com/apache/flink/pull/1821#discussion_r57844837
--- Diff:
flink-libraries/flink-table/src/main/scala/org/apache/flink/api/table/expressions/ExpressionParser.scala
---
@@ -116,7 +118,18 @@ object ExpressionParser extends JavaTokenParsers with
PackratParsers {
atom <~ ".cast(BOOL)" ^^ { e => Cast(e,
BasicTypeInfo.BOOLEAN_TYPE_INFO) } |
atom <~ ".cast(BOOLEAN)" ^^ { e => Cast(e,
BasicTypeInfo.BOOLEAN_TYPE_INFO) } |
atom <~ ".cast(STRING)" ^^ { e => Cast(e,
BasicTypeInfo.STRING_TYPE_INFO) } |
-atom <~ ".cast(DATE)" ^^ { e => Cast(e, BasicTypeInfo.DATE_TYPE_INFO) }
+atom <~ ".cast(DATE)" ^^ { e => Cast(e, BasicTypeInfo.DATE_TYPE_INFO)
} |
+// When an integer is directly casted.
+atom <~ "cast(BYTE)" ^^ { e => Cast(e, BasicTypeInfo.BYTE_TYPE_INFO) }
|
+atom <~ "cast(SHORT)" ^^ { e => Cast(e, BasicTypeInfo.SHORT_TYPE_INFO)
} |
+atom <~ "cast(INT)" ^^ { e => Cast(e, BasicTypeInfo.INT_TYPE_INFO) } |
+atom <~ "cast(LONG)" ^^ { e => Cast(e, BasicTypeInfo.LONG_TYPE_INFO) }
|
+atom <~ "cast(FLOAT)" ^^ { e => Cast(e, BasicTypeInfo.FLOAT_TYPE_INFO)
} |
+atom <~ "cast(DOUBLE)" ^^ { e => Cast(e,
BasicTypeInfo.DOUBLE_TYPE_INFO) } |
+atom <~ "cast(BOOL)" ^^ { e => Cast(e,
BasicTypeInfo.BOOLEAN_TYPE_INFO) } |
+atom <~ "cast(BOOLEAN)" ^^ { e => Cast(e,
BasicTypeInfo.BOOLEAN_TYPE_INFO) } |
+atom <~ "cast(STRING)" ^^ { e => Cast(e,
BasicTypeInfo.STRING_TYPE_INFO) } |
+atom <~ "cast(DATE)" ^^ { e => Cast(e, BasicTypeInfo.DATE_TYPE_INFO) }
--- End diff --
Since I am handling the '.' operator specifically I needed this combination
where the cast could come without '.' in it.
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request: FLINK-3579 Improve String concatenation (Ram)
Github user ramkrish86 commented on a diff in the pull request:
https://github.com/apache/flink/pull/1821#discussion_r57844753
--- Diff:
flink-libraries/flink-table/src/test/java/org/apache/flink/api/java/table/test/StringExpressionsITCase.java
---
@@ -121,6 +120,66 @@ public void testNonWorkingSubstring2() throws
Exception {
resultSet.collect();
}
+ @Test
+ public void testStringConcat() throws Exception {
+ ExecutionEnvironment env =
ExecutionEnvironment.getExecutionEnvironment();
+ TableEnvironment tableEnv = new TableEnvironment();
+
+ DataSet> ds = env.fromElements(
+ new Tuple2<>("ABCD", 3),
+ new Tuple2<>("ABCD", 2));
+
+ Table in = tableEnv.fromDataSet(ds, "a, b");
+
+ Table result = in
+ .select("a + b + 42");
--- End diff --
Let me check. I think it goes with the previous comment. If that is done
this should work too.
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request: FLINK-3579 Improve String concatenation (Ram)
Github user ramkrish86 commented on a diff in the pull request:
https://github.com/apache/flink/pull/1821#discussion_r57844716
--- Diff:
flink-libraries/flink-table/src/main/scala/org/apache/flink/api/table/plan/RexNodeTranslator.scala
---
@@ -134,7 +136,13 @@ object RexNodeTranslator {
case Plus(left, right) =>
val l = toRexNode(left, relBuilder)
val r = toRexNode(right, relBuilder)
-relBuilder.call(SqlStdOperatorTable.PLUS, l, r)
+if(SqlTypeName.STRING_TYPES.contains(l.getType.getSqlTypeName)) {
--- End diff --
Valid point. I thought of handling both sides. Let me check this.
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request: FLINK-3579 Improve String concatenation (Ram)
Github user fhueske commented on a diff in the pull request:
https://github.com/apache/flink/pull/1821#discussion_r57815729
--- Diff:
flink-libraries/flink-table/src/main/scala/org/apache/flink/api/table/expressions/ExpressionParser.scala
---
@@ -116,7 +118,18 @@ object ExpressionParser extends JavaTokenParsers with
PackratParsers {
atom <~ ".cast(BOOL)" ^^ { e => Cast(e,
BasicTypeInfo.BOOLEAN_TYPE_INFO) } |
atom <~ ".cast(BOOLEAN)" ^^ { e => Cast(e,
BasicTypeInfo.BOOLEAN_TYPE_INFO) } |
atom <~ ".cast(STRING)" ^^ { e => Cast(e,
BasicTypeInfo.STRING_TYPE_INFO) } |
-atom <~ ".cast(DATE)" ^^ { e => Cast(e, BasicTypeInfo.DATE_TYPE_INFO) }
+atom <~ ".cast(DATE)" ^^ { e => Cast(e, BasicTypeInfo.DATE_TYPE_INFO)
} |
+// When an integer is directly casted.
+atom <~ "cast(BYTE)" ^^ { e => Cast(e, BasicTypeInfo.BYTE_TYPE_INFO) }
|
+atom <~ "cast(SHORT)" ^^ { e => Cast(e, BasicTypeInfo.SHORT_TYPE_INFO)
} |
+atom <~ "cast(INT)" ^^ { e => Cast(e, BasicTypeInfo.INT_TYPE_INFO) } |
+atom <~ "cast(LONG)" ^^ { e => Cast(e, BasicTypeInfo.LONG_TYPE_INFO) }
|
+atom <~ "cast(FLOAT)" ^^ { e => Cast(e, BasicTypeInfo.FLOAT_TYPE_INFO)
} |
+atom <~ "cast(DOUBLE)" ^^ { e => Cast(e,
BasicTypeInfo.DOUBLE_TYPE_INFO) } |
+atom <~ "cast(BOOL)" ^^ { e => Cast(e,
BasicTypeInfo.BOOLEAN_TYPE_INFO) } |
+atom <~ "cast(BOOLEAN)" ^^ { e => Cast(e,
BasicTypeInfo.BOOLEAN_TYPE_INFO) } |
--- End diff --
This is about the Table API. SQL will be parsed by Calcite. So it is up to
us what we accept.
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request: FLINK-3579 Improve String concatenation (Ram)
Github user tillrohrmann commented on the pull request: https://github.com/apache/flink/pull/1821#issuecomment-202819063 As far as I can tell the changes look good. I had some inline comments where I'm not sure whether the implementation is exhaustive. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] flink pull request: FLINK-3579 Improve String concatenation (Ram)
Github user tillrohrmann commented on a diff in the pull request:
https://github.com/apache/flink/pull/1821#discussion_r57699432
--- Diff:
flink-libraries/flink-table/src/test/java/org/apache/flink/api/java/table/test/StringExpressionsITCase.java
---
@@ -121,6 +120,66 @@ public void testNonWorkingSubstring2() throws
Exception {
resultSet.collect();
}
+ @Test
+ public void testStringConcat() throws Exception {
+ ExecutionEnvironment env =
ExecutionEnvironment.getExecutionEnvironment();
+ TableEnvironment tableEnv = new TableEnvironment();
+
+ DataSet> ds = env.fromElements(
+ new Tuple2<>("ABCD", 3),
+ new Tuple2<>("ABCD", 2));
+
+ Table in = tableEnv.fromDataSet(ds, "a, b");
+
+ Table result = in
+ .select("a + b + 42");
--- End diff --
What happens if you do `"42 + a"` or even `"42 + b + a"`?
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request: FLINK-3579 Improve String concatenation (Ram)
Github user tillrohrmann commented on a diff in the pull request:
https://github.com/apache/flink/pull/1821#discussion_r57699238
--- Diff:
flink-libraries/flink-table/src/main/scala/org/apache/flink/api/table/plan/RexNodeTranslator.scala
---
@@ -134,7 +136,13 @@ object RexNodeTranslator {
case Plus(left, right) =>
val l = toRexNode(left, relBuilder)
val r = toRexNode(right, relBuilder)
-relBuilder.call(SqlStdOperatorTable.PLUS, l, r)
+if(SqlTypeName.STRING_TYPES.contains(l.getType.getSqlTypeName)) {
--- End diff --
What if `r` is a String type and `l` would have to be casted to `String`?
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request: FLINK-3579 Improve String concatenation (Ram)
Github user tillrohrmann commented on a diff in the pull request:
https://github.com/apache/flink/pull/1821#discussion_r57699015
--- Diff:
flink-libraries/flink-table/src/main/scala/org/apache/flink/api/table/expressions/ExpressionParser.scala
---
@@ -116,7 +118,18 @@ object ExpressionParser extends JavaTokenParsers with
PackratParsers {
atom <~ ".cast(BOOL)" ^^ { e => Cast(e,
BasicTypeInfo.BOOLEAN_TYPE_INFO) } |
atom <~ ".cast(BOOLEAN)" ^^ { e => Cast(e,
BasicTypeInfo.BOOLEAN_TYPE_INFO) } |
atom <~ ".cast(STRING)" ^^ { e => Cast(e,
BasicTypeInfo.STRING_TYPE_INFO) } |
-atom <~ ".cast(DATE)" ^^ { e => Cast(e, BasicTypeInfo.DATE_TYPE_INFO) }
+atom <~ ".cast(DATE)" ^^ { e => Cast(e, BasicTypeInfo.DATE_TYPE_INFO)
} |
+// When an integer is directly casted.
+atom <~ "cast(BYTE)" ^^ { e => Cast(e, BasicTypeInfo.BYTE_TYPE_INFO) }
|
+atom <~ "cast(SHORT)" ^^ { e => Cast(e, BasicTypeInfo.SHORT_TYPE_INFO)
} |
+atom <~ "cast(INT)" ^^ { e => Cast(e, BasicTypeInfo.INT_TYPE_INFO) } |
+atom <~ "cast(LONG)" ^^ { e => Cast(e, BasicTypeInfo.LONG_TYPE_INFO) }
|
+atom <~ "cast(FLOAT)" ^^ { e => Cast(e, BasicTypeInfo.FLOAT_TYPE_INFO)
} |
+atom <~ "cast(DOUBLE)" ^^ { e => Cast(e,
BasicTypeInfo.DOUBLE_TYPE_INFO) } |
+atom <~ "cast(BOOL)" ^^ { e => Cast(e,
BasicTypeInfo.BOOLEAN_TYPE_INFO) } |
+atom <~ "cast(BOOLEAN)" ^^ { e => Cast(e,
BasicTypeInfo.BOOLEAN_TYPE_INFO) } |
--- End diff --
Is it valid SQL to have both `cast(BOOL)` and `cast(BOOLEAN)`?
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request: FLINK-3579 Improve String concatenation (Ram)
Github user tillrohrmann commented on a diff in the pull request:
https://github.com/apache/flink/pull/1821#discussion_r57698940
--- Diff:
flink-libraries/flink-table/src/main/scala/org/apache/flink/api/table/expressions/ExpressionParser.scala
---
@@ -116,7 +118,18 @@ object ExpressionParser extends JavaTokenParsers with
PackratParsers {
atom <~ ".cast(BOOL)" ^^ { e => Cast(e,
BasicTypeInfo.BOOLEAN_TYPE_INFO) } |
atom <~ ".cast(BOOLEAN)" ^^ { e => Cast(e,
BasicTypeInfo.BOOLEAN_TYPE_INFO) } |
atom <~ ".cast(STRING)" ^^ { e => Cast(e,
BasicTypeInfo.STRING_TYPE_INFO) } |
-atom <~ ".cast(DATE)" ^^ { e => Cast(e, BasicTypeInfo.DATE_TYPE_INFO) }
+atom <~ ".cast(DATE)" ^^ { e => Cast(e, BasicTypeInfo.DATE_TYPE_INFO)
} |
+// When an integer is directly casted.
+atom <~ "cast(BYTE)" ^^ { e => Cast(e, BasicTypeInfo.BYTE_TYPE_INFO) }
|
+atom <~ "cast(SHORT)" ^^ { e => Cast(e, BasicTypeInfo.SHORT_TYPE_INFO)
} |
+atom <~ "cast(INT)" ^^ { e => Cast(e, BasicTypeInfo.INT_TYPE_INFO) } |
+atom <~ "cast(LONG)" ^^ { e => Cast(e, BasicTypeInfo.LONG_TYPE_INFO) }
|
+atom <~ "cast(FLOAT)" ^^ { e => Cast(e, BasicTypeInfo.FLOAT_TYPE_INFO)
} |
+atom <~ "cast(DOUBLE)" ^^ { e => Cast(e,
BasicTypeInfo.DOUBLE_TYPE_INFO) } |
+atom <~ "cast(BOOL)" ^^ { e => Cast(e,
BasicTypeInfo.BOOLEAN_TYPE_INFO) } |
+atom <~ "cast(BOOLEAN)" ^^ { e => Cast(e,
BasicTypeInfo.BOOLEAN_TYPE_INFO) } |
+atom <~ "cast(STRING)" ^^ { e => Cast(e,
BasicTypeInfo.STRING_TYPE_INFO) } |
+atom <~ "cast(DATE)" ^^ { e => Cast(e, BasicTypeInfo.DATE_TYPE_INFO) }
--- End diff --
this rule was already added at the top of the list
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request: FLINK-3579 Improve String concatenation (Ram)
Github user tillrohrmann commented on a diff in the pull request:
https://github.com/apache/flink/pull/1821#discussion_r57698703
--- Diff:
flink-libraries/flink-table/src/main/scala/org/apache/flink/api/table/expressions/ExpressionParser.scala
---
@@ -59,6 +59,8 @@ object ExpressionParser extends JavaTokenParsers with
PackratParsers {
Literal(str.toInt)
} else if (str.endsWith("f") | str.endsWith("F")) {
Literal(str.toFloat)
+} else if (str.endsWith(".")) {
--- End diff --
Is it valid SQL standard to have a format like `1.`? What happens to `.1`?
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request: FLINK-3579 Improve String concatenation (Ram)
Github user ramkrish86 commented on the pull request: https://github.com/apache/flink/pull/1821#issuecomment-202320161 @tillrohrmann - Ping for reviews!!! --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] flink pull request: FLINK-3579 Improve String concatenation (Ram)
Github user ramkrish86 commented on the pull request: https://github.com/apache/flink/pull/1821#issuecomment-199900035 Updated PR. RexNodeTranslator had > 100 character line. That is now corrected. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] flink pull request: FLINK-3579 Improve String concatenation (Ram)
Github user ramkrish86 commented on the pull request: https://github.com/apache/flink/pull/1821#issuecomment-199897376 @vasia Thanks for the info. Let me check that and update the 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] flink pull request: FLINK-3579 Improve String concatenation (Ram)
Github user vasia commented on the pull request: https://github.com/apache/flink/pull/1821#issuecomment-199858172 @ramkrish86 it doesn't look like tests are failing. It's a checkstyle violation in `RexNodeTranslator`. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] flink pull request: FLINK-3579 Improve String concatenation (Ram)
Github user fhueske commented on the pull request: https://github.com/apache/flink/pull/1821#issuecomment-199704347 Hi @ramkrish86, I haven't touched the part of the code yet. Need some time to become familiar with the `ExpressionParser`. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] flink pull request: FLINK-3579 Improve String concatenation (Ram)
Github user ramkrish86 commented on the pull request: https://github.com/apache/flink/pull/1821#issuecomment-199642088 @fhueske - Not sure how to check the failed test cases. Any chance of a review here? Thank you. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] flink pull request: FLINK-3579 Improve String concatenation (Ram)
GitHub user ramkrish86 opened a pull request: https://github.com/apache/flink/pull/1821 FLINK-3579 Improve String concatenation (Ram) Updated PR against branch after the deletion of tableOnCalcite. It was suggested over in the old PR an d in JIRA to create a new PR as the old one was closed automatically. @fhueske and @tillrohrmann - Pls review. You can merge this pull request into a Git repository by running: $ git pull https://github.com/ramkrish86/flink FLINK-3579_new Alternatively you can review and apply these changes as the patch at: https://github.com/apache/flink/pull/1821.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 #1821 commit 58f52ae62726f717590e0f5f560a06afcd5e1f9c Author: ramkrishna Date: 2016-03-21T07:22:31Z FLINK-3579 Improve String concatenation (Ram) --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
