[GitHub] flink issue #2653: [FLINK-4469] [table] Add support for user defined table f...

[twalthr]

Github user twalthr commented on the issue:

https://github.com/apache/flink/pull/2653
  
Thanks. Merging...


---
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 issue #2653: [FLINK-4469] [table] Add support for user defined table f...

[wuchong]

Github user wuchong commented on the issue:

https://github.com/apache/flink/pull/2653
  
Thanks @twalthr , I will add the documentation ASAP.


---
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 issue #2653: [FLINK-4469] [table] Add support for user defined table f...

[twalthr]

Github user twalthr commented on the issue:

https://github.com/apache/flink/pull/2653
  
@wuchong thanks for updating the PR! It looks good now. I will rebase it, 
quickly scan over the code again and merge it tomorrow.

However, we should fix FLINK-5223 as soon as possible. It would be great if 
someone could do that before the 1.2 release.


---
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 issue #2653: [FLINK-4469] [table] Add support for user defined table f...

[wuchong]

Github user wuchong commented on the issue:

https://github.com/apache/flink/pull/2653
  
Hi @fhueske @twalthr ,  I have updated the PR, please review it again when 
you are available.


---
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 issue #2653: [FLINK-4469] [table] Add support for user defined table f...

[twalthr]

Github user twalthr commented on the issue:

https://github.com/apache/flink/pull/2653
  
I have already reviewed half of the new code. I will review the rest 
tomorrow. 


---
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 issue #2653: [FLINK-4469] [table] Add support for user defined table f...

[wuchong]

Github user wuchong commented on the issue:

https://github.com/apache/flink/pull/2653
  
Hi @fhueske  @twalthr , could you have a look at this PR again ? I have fix 
the conflicts again and squashed the commits. 


---
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 issue #2653: [FLINK-4469] [table] Add support for user defined table f...

[wuchong]

Github user wuchong commented on the issue:

https://github.com/apache/flink/pull/2653
  
Hi @fhueske @twalthr , I have addressed all the comments and made the 
following changes:

1. Forbid TableFunction implemented by Scala object, since the `collect` is 
called on a singleton. It will be error-prone in some concurrent cases.
2. Check correct errors if a SQL query refers to a function in FROM that 
has not been registered or which is a ScalarFunction.
3. Make `TableFunction` and `ScalarFunction` clean, **remove 
`UserDefinedFunction`**, and move eval relative functions and 
`createSqlFunction` to utils.
4. Rename `ScalarFunctions` to `SqlFunctions` because contains also 
TableFunction logic.
5. Restructure tests. Test Java Table API by comparing the RelNode of two 
tables. And check SQL API's DataSetRel and DataStreamRel via `TableTestBase` 
utils. The tests reduced into `stream/UserDefinedTableFunctionITCase`, 
`batch/UserDefinedTableFunctionITCase`, and 
`stream/UserDefinedTableFunctionTest` , `batch/UserDefinedTableFunctionTest`.
6. Scala Table API implicitly convert `TableFunction` into 
`TableFunctionCall`. `TableFunctionCall` is not an `Expression` or 
`LogicalNode`, but is like `GroupWindow`, can be visible to the users (contains 
API such `as(...)`).
7. Fix the hierarchy type extraction problem.
8. Rebase the code and fix conflicts.
9. ...




---
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 issue #2653: [FLINK-4469] [table] Add support for user defined table f...

[fhueske]

Github user fhueske commented on the issue:

https://github.com/apache/flink/pull/2653
  
Hi @wuchong, I think @twalthr's main concerns are not about the parser, but 
rather that expressions and logical nodes are mixed, especially in the Scala 
Table API where the `TableFunction` is implicitly converted into a 
`LogicalNode` and not into an `Expression`. 
If I got it right, @twalthr would like to implicitly convert the 
`TableFunction` into an `Expression` at the API level and later internally into 
a `LogicalNode`.

Please correct me if I didn't get that right @twalthr.


---
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 issue #2653: [FLINK-4469] [table] Add support for user defined table f...

[wuchong]

Github user wuchong commented on the issue:

https://github.com/apache/flink/pull/2653
  
Regarding to the mixing parser Expression and Logical Node, how about to 
create a `LogicalParser` which is used to parse string to LogicalNode ? This 
can separate expressions and logical nodes, and keep aliases only work after 
table function call. 

What do you think ? @twalthr @fhueske


---
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 issue #2653: [FLINK-4469] [table] Add support for user defined table f...

[wuchong]

Github user wuchong commented on the issue:

https://github.com/apache/flink/pull/2653
  
Thank you @fhueske @twalthr for the review, I will update the PR 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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] flink issue #2653: [FLINK-4469] [table] Add support for user defined table f...

[fhueske]

Github user fhueske commented on the issue:

https://github.com/apache/flink/pull/2653
  
I think we should convert the tests for the Java String-based Table API to 
check the logical plans of queries against the logical plan of a corresponding 
Scala Expression-based Table API query.

That leaves the Scala Table API and SQL tests for batch and streaming. I 
think some of those can be checked with @twalthr's test framework. 


---
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 issue #2653: [FLINK-4469] [table] Add support for user defined table f...

[wuchong]

Github user wuchong commented on the issue:

https://github.com/apache/flink/pull/2653
  
Hi @fhueske , you mentioned two ways to reduce IT cases. One is comparing 
the logical plans of two tables, this can reduce Java IT cases. Another is 
using `TableTestBase` tool to write unit tests, this can reduce both Java and 
SQL IT cases I think. 

So which one do you suggest ?  The latter one or both ? 


---
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 issue #2653: [FLINK-4469] [table] Add support for user defined table f...

[fhueske]

Github user fhueske commented on the issue:

https://github.com/apache/flink/pull/2653
  
Maybe one more thing to add. @twalthr recently added some tooling to write 
unit tests that check the translation from SQL / Table API to `DataSetRel` and 
`DataStreamRel` nodes, i.e., exclude the final translation into DataSet and 
DataStream programs. See PR #2595.
A couple of ITCases could be replaced by such tests. We need of course a 
few end-to-end tests nonetheless, but maybe not as many as currently.

I also noticed that there are not tests that check for correct failures, 
e.g., check correct errors if a SQL query refers to a function in `FROM` that 
has not been registered or which is a ScalarFunction.


---
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 issue #2653: [FLINK-4469] [table] Add support for user defined table f...

[twalthr]

Github user twalthr commented on the issue:

https://github.com/apache/flink/pull/2653
  
@wuchong thanks for the PR. I will also review it the next days.


---
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 issue #2653: [FLINK-4469] [table] Add support for user defined table f...

[wuchong]

Github user wuchong commented on the issue:

https://github.com/apache/flink/pull/2653
  
Sounds great. Thanks Fabian .


---
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 issue #2653: [FLINK-4469] [table] Add support for user defined table f...

[fhueske]

Github user fhueske commented on the issue:

https://github.com/apache/flink/pull/2653
  
Hi @wuchong, I did a first high-level pass over the PR. From what I've seen 
it looks really good and I do not expect that major changes are necessary 👍. 
Will do a more thorough review in the next days.

Thanks, Fabian


---
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 issue #2653: [FLINK-4469] [table] Add support for user defined table f...

[wuchong]

Github user wuchong commented on the issue:

https://github.com/apache/flink/pull/2653
  
Hi @sunjincheng121 ,  thank you for the reviewing. I will update the PR 
according to your comments. 


---
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 issue #2653: [FLINK-4469] [table] Add support for user defined table f...

[fhueske]

Github user fhueske commented on the issue:

https://github.com/apache/flink/pull/2653
  
Thanks for the update @wuchong! Will have a look at 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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] flink issue #2653: [FLINK-4469] [table] Add support for user defined table f...

[wuchong]

Github user wuchong commented on the issue:

https://github.com/apache/flink/pull/2653
  
@fhueske I have updated this PR for the following changes.

1. Remove CROSS/OUTER APPLY support in SQL
2. Change Java Table API from `.crossApply("split(c)", "s")` to 
`.corssApply("split(c) as (s)")`
3. Make `ExpressionParser` can parse a string to `LogicalNode`

I think the PR is ready to be reviewed :)


---
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 issue #2653: [FLINK-4469] [table] Add support for user defined table f...

[shaoxuan-wang]

Github user shaoxuan-wang commented on the issue:

https://github.com/apache/flink/pull/2653
  
Jack,
Giving this for the second thought, I feel we can just use LATERAL and
INNER/LEFT JOIN (which is already supported by Calcite) instead of
introducing "CROSS/OUTER APPLY" . Let us sync offline and see how to revise
the current PR.


On Mon, Oct 24, 2016 at 11:07 AM, Jark  wrote:

> Hi @fhueske  @twalthr
>  , do you have any thoughts on this pull
> request ?
>
> —
> You are receiving this because you are subscribed to this thread.
> Reply to this email directly, view it on GitHub
> , or 
mute
> the thread
> 

> .
>



---
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 issue #2653: [FLINK-4469] [table] Add support for user defined table f...

[fhueske]

Github user fhueske commented on the issue:

https://github.com/apache/flink/pull/2653
  
HI @wuchong, thanks for this feature. This looks really interesting :-)

I haven't looked at the PR in detail yet. However, I would like to avoid 
customizing the parser if possible. I haven't tested it but would it be 
possible to express `OUTER APPLY` and `CROSS APPLY` with Calcite's `LATERAL` 
tables?

`CROSS APPLY` corresponds to: 

```
SELECT MyTable.*, t.s  FROM MyTable, LATERAL TABLE(split(MyTable.a)) AS t(s)
```

`OUTER APPLY` could be:

```
SELECT MyTable.*, t.s  
  FROM MyTable LEFT JOIN LATERAL TABLE(split(MyTable.a)) AS t(s)
```

Adding `crossApply` and `outerApply` for the Table API is 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 issue #2653: [FLINK-4469] [table] Add support for user defined table f...

[wuchong]

Github user wuchong commented on the issue:

https://github.com/apache/flink/pull/2653
  
Hi @fhueske @twalthr ,  do you have any thoughts on this pull request ? 


---
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 issue #2653: [FLINK-4469] [table] Add support for user defined table f...

[wuchong]

Github user wuchong commented on the issue:

https://github.com/apache/flink/pull/2653
  
Because SQL doesn't have standard user-defined table function, we introduce 
MS SQL Server's `CROSS/OUTER APPLY` to support UDTF , see 
https://technet.microsoft.com/en-us/library/ms175156(v=sql.105).aspx .

NOTE: In order to implement the `CROSS/OUTER APPLY`, we need to modify 
Calcite's SQL Parser. So we include `Parser.jj` from Calcite source code and 
add some modifications (search FLINK in this file). 

See 
[this](https://docs.google.com/document/d/15iVc1781dxYWm3loVQlESYvMAxEzbbuVFPZWBYuY1Ek/edit#)
 for more UDTF design, 


---
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 issue #2653: [FLINK-4469] [table] Add support for user defined table f...

[wuchong]

Github user wuchong commented on the issue:

https://github.com/apache/flink/pull/2653
  
Hi @twalthr @fhueske  , it will be great if you can review 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.
---