FLINK-3579 Improve String concatenation (Ram) #1821
Conversation
|
@fhueske - Not sure how to check the failed test cases. Any chance of a review here? Thank you. |
|
Hi @ramkrish86, I haven't touched the part of the code yet. Need some time to become familiar with the |
|
@ramkrish86 it doesn't look like tests are failing. It's a checkstyle violation in |
|
@vasia |
|
Updated PR. RexNodeTranslator had > 100 character line. That is now corrected. |
…579_new Conflicts: flink-libraries/flink-table/src/test/java/org/apache/flink/api/java/table/test/StringExpressionsITCase.java
|
@tillrohrmann - Ping for reviews!!! |
| Literal(str.toInt) | ||
| } else if (str.endsWith("f") | str.endsWith("F")) { | ||
| Literal(str.toFloat) | ||
| } else if (str.endsWith(".")) { |
There was a problem hiding this comment.
Is it valid SQL standard to have a format like 1.? What happens to .1?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@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.
|
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. |
|
Sorry for the wrong observation. |
|
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 ? |
|
Thanks for updating the PR @ramkrish86. Your proposed approach (creating a new PR for the parser changes) would be fine with me. |
|
Thank you @tillrohrmann . Let me if the changes in the updated PR is fine with you. |
|
@tillrohrmann - Ping to review the PR. |
|
Changes look good to me :-) Good work @ramkrish86! |
|
There is 1 failing check. |
|
I'm merging this. |
7 participants
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.