Re: [PR] [FLINK-32986][test] Fix createTemporaryFunction type inference error [flink]
jeyhunkarimov commented on PR #23586:
URL: https://github.com/apache/flink/pull/23586#issuecomment-1840305544
Does it make any difference? yes and no IMHO.
Yes, because imagine `test1()` executes the statement
`createTemporarySystemFunction("add", new TestAddWithOpen)` (also
`createTemporaryFunction`). In this case, `add` function is visible to
`test2()`.
No, because, this (creating a new table for each test) would be overkill
IMHO. We just need to be sure that we are testing the functions that are
defined within the scope of a particular test (independent whether we use
`createTemporarySystemFunction` or `createTemporaryFunction`).
Some alternatives would be:
- we could explicitly drop functions in each test
- we could create a method to upsert a function (if a function exists, drop
and register the new one)
- we could use global functions in `before` via
`createTemporarySystemFunction` and use the functions that should live within
the scope of the test via `createTemporaryFunction` (we should still have some
mechanism to drop them)
In any case, the scope of this PR was diverged quite a bit (the main issue
was already resolved in the meantime). If you guys agree, I would close this PR
and mark the issue as resolved.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@flink.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] [FLINK-32986][test] Fix createTemporaryFunction type inference error [flink]
snuyanzin commented on PR #23586: URL: https://github.com/apache/flink/pull/23586#issuecomment-1839102748 does it really make sense if for each test in `@BeforeEach` there a new `tEnv` is created like ``` this.tEnv = StreamTableEnvironment.create(env, setting) ``` ? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@flink.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [FLINK-32986][test] Fix createTemporaryFunction type inference error [flink]
jeyhunkarimov commented on PR #23586: URL: https://github.com/apache/flink/pull/23586#issuecomment-1838902148 > > Mainly because of this comment: https://github.com/apache/flink/blob/7eaa5db30244b8b5d9cdc6ab0cb327e255d6fadc/flink-table/flink-table-planner/src/test/scala/org/apache/flink/table/planner/runtime/stream/sql/LookupJoinITCase.scala#L430 > > IMHO then it would make sense first to answer the question what is correct approach and why Thanks for the comment @snuyanzin My main reasoning behind the refactor is that `createTemporarySystemFunction` creates global functions. These (permanent) functions are overriden by temporary functions. To make sure that we are testing the correct function, it might make sense to force override the global functions (if they exists). WDYT? - -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@flink.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [FLINK-32986][test] Fix createTemporaryFunction type inference error [flink]
jeyhunkarimov commented on PR #23586: URL: https://github.com/apache/flink/pull/23586#issuecomment-1838875420 > @jeyhunkarimov Thanks for your contribution. The PR description should be updated too. Especially the `Brief change log` part. WDYT? @JingGe sure thing. I will update the description -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@flink.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [FLINK-32986][test] Fix createTemporaryFunction type inference error [flink]
snuyanzin commented on PR #23586: URL: https://github.com/apache/flink/pull/23586#issuecomment-1838854821 > Mainly because of this comment: https://github.com/apache/flink/blob/7eaa5db30244b8b5d9cdc6ab0cb327e255d6fadc/flink-table/flink-table-planner/src/test/scala/org/apache/flink/table/planner/runtime/stream/sql/LookupJoinITCase.scala#L430 then it would make sense first to answer the question what is correct approach and why -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@flink.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [FLINK-32986][test] Fix createTemporaryFunction type inference error [flink]
JingGe commented on PR #23586: URL: https://github.com/apache/flink/pull/23586#issuecomment-1838849272 @jeyhunkarimov Thanks for your contribution. The PR description should be updated too. Especially the `Brief change log` part. WDYT? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@flink.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [FLINK-32986][test] Fix createTemporaryFunction type inference error [flink]
jeyhunkarimov commented on PR #23586: URL: https://github.com/apache/flink/pull/23586#issuecomment-1838835287 Thanks for the comment @snuyanzin Mainly because of this comment: https://github.com/apache/flink/blob/7eaa5db30244b8b5d9cdc6ab0cb327e255d6fadc/flink-table/flink-table-planner/src/test/scala/org/apache/flink/table/planner/runtime/stream/sql/LookupJoinITCase.scala#L430 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@flink.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [FLINK-32986][test] Fix createTemporaryFunction type inference error [flink]
snuyanzin commented on PR #23586: URL: https://github.com/apache/flink/pull/23586#issuecomment-1838813283 @jeyhunkarimov could you please clarify why are you going to change to `createTemporaryFunction`? while at the same time in javadoc it is mentioned to use `createTemporarySystemFunction` instead https://github.com/apache/flink/blob/7eaa5db30244b8b5d9cdc6ab0cb327e255d6fadc/flink-table/flink-table-api-scala-bridge/src/main/scala/org/apache/flink/table/api/bridge/scala/StreamTableEnvironment.scala#L72-L78 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@flink.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [FLINK-32986][test] Fix createTemporaryFunction type inference error [flink]
jeyhunkarimov commented on PR #23586: URL: https://github.com/apache/flink/pull/23586#issuecomment-1838720871 @lincoln-lil yes you are right! I tested, it seems the issue got fixed in the meantime. So, this PR becomes just a refactor in this case. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@flink.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [FLINK-32986][test] Fix createTemporaryFunction type inference error [flink]
lincoln-lil commented on PR #23586: URL: https://github.com/apache/flink/pull/23586#issuecomment-1838229022 @jeyhunkarimov One interesting thing is that I reverted to the original version of `TestAddWithOpen`, and the previous exception is no longer reproduced on the master branch. Can you confirm if the function change is still needed? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@flink.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [FLINK-32986][test] Fix createTemporaryFunction type inference error [flink]
jeyhunkarimov commented on PR #23586: URL: https://github.com/apache/flink/pull/23586#issuecomment-1837590317 > @jeyhunkarimov thanks for fixing this! Only one comment. Thanks a lot for the review @lincoln-lil . I addressed your comment -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@flink.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [FLINK-32986][test] Fix createTemporaryFunction type inference error [flink]
lincoln-lil commented on code in PR #23586:
URL: https://github.com/apache/flink/pull/23586#discussion_r1413081174
##
flink-table/flink-table-planner/src/test/scala/org/apache/flink/table/planner/runtime/utils/UserDefinedFunctionTestUtils.scala:
##
@@ -382,6 +382,13 @@ object UserDefinedFunctionTestUtils {
eval(a, b.asInstanceOf[Long])
}
+def eval(a: java.lang.Long, b: java.lang.Long): java.lang.Long = {
Review Comment:
Just use `JLong` instead of `java.lang.Long`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@flink.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] [FLINK-32986][test] Fix createTemporaryFunction type inference error [flink]
jeyhunkarimov commented on PR #23586: URL: https://github.com/apache/flink/pull/23586#issuecomment-1831282659 > @jeyhunkarimov Thanks for contributing to Flink! Can you sort out the conflict that has happened? > > @lincoln-lil may be the best person to review this PR. @jnh5y Thanks! Now conflicts resolved, CI is green. Sure, I will wait for @lincoln-lil -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@flink.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [FLINK-32986][test] Fix createTemporaryFunction type inference error [flink]
jeyhunkarimov commented on PR #23586: URL: https://github.com/apache/flink/pull/23586#issuecomment-1830956237 @flinkbot run azure -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@flink.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [FLINK-32986][test] Fix createTemporaryFunction type inference error [flink]
jnh5y commented on PR #23586: URL: https://github.com/apache/flink/pull/23586#issuecomment-1828429647 @jeyhunkarimov Thanks for contributing to Flink! Can you sort out the conflict that has happened? @lincoln-lil may be the best person to review this PR. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@flink.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [FLINK-32986][test] Fix createTemporaryFunction type inference error [flink]
jeyhunkarimov commented on PR #23586: URL: https://github.com/apache/flink/pull/23586#issuecomment-1819093323 Hi @jnh5y @dawidwys , could you guys check this PR? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@flink.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [FLINK-32986][test] Fix createTemporaryFunction type inference error [flink]
jeyhunkarimov commented on PR #23586: URL: https://github.com/apache/flink/pull/23586#issuecomment-1794335640 Hi @MartijnVisser could you please check this PR? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@flink.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [FLINK-32986][test] Fix createTemporaryFunction type inference error [flink]
jeyhunkarimov commented on PR #23586: URL: https://github.com/apache/flink/pull/23586#issuecomment-1777952204 Hi @snuyanzin Could you please review the PR? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@flink.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [FLINK-32986][test] Fix createTemporaryFunction type inference error [flink]
flinkbot commented on PR #23586: URL: https://github.com/apache/flink/pull/23586#issuecomment-1777288158 ## CI report: * 802b741f9f76c8d86c98df88f8cf76f9d15b442a UNKNOWN Bot commands The @flinkbot bot supports the following commands: - `@flinkbot run azure` re-run the last Azure build -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@flink.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[PR] [FLINK-32986][test] Fix createTemporaryFunction type inference error [flink]
jeyhunkarimov opened a new pull request, #23586: URL: https://github.com/apache/flink/pull/23586 ## What is the purpose of the change This pull request fixes tests in `LookupJoinITCase`. The errors in tests occured when changing `registerFunction` to `createTemporaryFunction `. The main issue was that existing `UserDefinedFunctionTestUtils.TestAddWithOpen` class's `eval` method was enforcing `NOT NULL` on types, because `Int` and `Long` types cannot be null in `scala`. This was creating a discrepancy between table schema and the registered function. ## Brief change log - Add an extra method to `UserDefinedFunctionTestUtils.TestAddWithOpen` to support `nullable` types ## Verifying this change This change is already covered by existing tests, such as `UserDefinedFunctionTestUtils`. ## 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, Kubernetes/Yarn, ZooKeeper: no - The S3 file system connector: no ## Documentation - Does this pull request introduce a new feature? no -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@flink.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
