New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[FLINK-13433][table-planner-blink] Do not fetch data from LookupableTableSource if the JoinKey in left side of LookupJoin contains null value. #9285
Conversation
Thanks a lot for your contribution to the Apache Flink project. I'm the @flinkbot. I help the community Automated ChecksLast check on commit a10620b (Wed Aug 07 08:17:21 UTC 2019) Warnings:
Mention the bot in a comment to re-run the automated checks. Review Progress
Please see the Pull Request Review Guide for a full explanation of the review process. The Bot is tracking the review progress through labels. Labels are applied according to the order of the review items. For consensus, approval by a Flink committer of PMC member is required Bot commandsThe @flinkbot bot supports the following commands:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! I've left some minor comments, btw, would it be better to parameterize the synchronous/asynchronous mode for the it case ?
.../src/test/scala/org/apache/flink/table/planner/runtime/batch/sql/join/LookupJoinITCase.scala
Show resolved
Hide resolved
...link/src/test/scala/org/apache/flink/table/planner/runtime/stream/sql/LookupJoinITCase.scala
Outdated
Show resolved
Hide resolved
I am interested to sql: |
@JingsongLi At first glance, I guess the query is not supported. Good catch, I will looks into the problem. |
...er-blink/src/main/scala/org/apache/flink/table/planner/codegen/LookupJoinCodeGenerator.scala
Outdated
Show resolved
Hide resolved
...n/scala/org/apache/flink/table/planner/plan/rules/physical/common/CommonLookupJoinRule.scala
Outdated
Show resolved
Hide resolved
...blink/src/test/scala/org/apache/flink/table/planner/plan/batch/sql/join/LookupJoinTest.scala
Outdated
Show resolved
Hide resolved
3e6a308
to
5958000
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @beyond1920 , could you also update InMemoryAsyncLookupFunction#eval
and InMemoryLookupFunction#eval
to throw exception when one of the argument is null?
...er-blink/src/main/scala/org/apache/flink/table/planner/codegen/LookupJoinCodeGenerator.scala
Outdated
Show resolved
Hide resolved
...er-blink/src/main/scala/org/apache/flink/table/planner/codegen/LookupJoinCodeGenerator.scala
Outdated
Show resolved
Hide resolved
...er-blink/src/main/scala/org/apache/flink/table/planner/codegen/LookupJoinCodeGenerator.scala
Outdated
Show resolved
Hide resolved
...n/scala/org/apache/flink/table/planner/plan/rules/physical/common/CommonLookupJoinRule.scala
Outdated
Show resolved
Hide resolved
Hi, JarkWu, InMemoryAsyncLookupFunction and InMemoryLookupFunction lookup records with null value on the lookup key field when argument contains null, which complies with the contract mentioned in https://github.com/apache/flink/pull/9335/files, I think the behavior is also reasonable. |
I agree with you @beyond1920 , however, I want to have some test to cover we didn't push any nulls into |
…ableSource if the JoinKey in left side of LookupJoin contains null value.
…or an expanded version) in LookupJoin.
@wuchong , Ok |
Thanks @beyond1920 , looks good to me now. |
…ableSource if the JoinKey in left side of LookupJoin contains null value This closes #9285
BTW, could you also open a jira for empty key for hbase lookup function? hbase forbidden empty row key. |
@KurtYoung The hbase lookup function is updated in #9335. |
…ableSource if the JoinKey in left side of LookupJoin contains null value This closes apache#9285
…ableSource if the JoinKey in left side of LookupJoin contains null value This closes apache#9285
What is the purpose of the change
For LookupJoin, if joinKey in left side of a LeftOuterJoin/InnerJoin contains null values, there is no need to fetch data from
LookupableTableSource
.However, we don't shortcut the fetch function under the case at present, the correctness of results depends on the
TableFunction
implementation of eachLookupableTableSource
.Brief change log
LookupJoin
where lookupKeys contains null.Verifying this change
testcases
Does this pull request potentially affect one of the following parts:
@Public(Evolving)
: noDocumentation