-
Notifications
You must be signed in to change notification settings - Fork 13k
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-22303][table-planner-blink] FlinkRelMdFilteredColumnInterval should remapping the columnIndex of the inputRel otherwise may cause IllegalArgumentException or get incorrectly metadata #15641
Conversation
…hould remapping the columnIndex of the inputRel otherwise may cause IllegalArgumentException or get incorrectly metadata
Thanks a lot for your contribution to the Apache Flink project. I'm the @flinkbot. I help the community Automated ChecksLast check on commit f61e767 (Fri May 28 09:09:52 UTC 2021) 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.
Thanks for the contribution @lincoln-lil , I left some minor comments
@@ -132,11 +143,12 @@ class FlinkRelMdFilteredColumnInterval private extends MetadataHandler[FilteredC | |||
} else { | |||
val filterRef = calc.getProgram.getProjectList.get(filterArg) | |||
val condition = calc.getProgram.expandLocalRef(filterRef) | |||
val columnRef = calc.getProgram.getProjectList.get(columnIndex).getIndex |
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.
we need the similar logic with Project ? (should use the ref index of expandLocalRef result)
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.
good catch!
@@ -236,7 +248,9 @@ class FlinkRelMdFilteredColumnInterval private extends MetadataHandler[FilteredC | |||
mq: RelMetadataQuery, | |||
columnIndex: Int, | |||
filterArg: Int): ValueInterval = { | |||
checkArgument(filterArg == -1) | |||
if (filterArg != -1) { |
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.
please add a test case to cover this change
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.
convered by the newly added case testGetColumnIntervalOnAggregateWithFilter
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 for the quick update, LGTM overall except two minor comments
* additional predicate expression, if the given column is a RexInputRef then the final interval | ||
* is intersect with the predicate, if the given column is a RexLiteral then the final interval | ||
* is the literal itself, otherwise return null. | ||
* This can be improved (e.g., for RexCall) later and refactor this meta data to a utility class |
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.
move refactor this meta data to a utility class because it only serves for calculating the monotonicity of some specific aggregate functions.
to class javadoc ?
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.
ok
@@ -36,6 +36,7 @@ import org.apache.calcite.schema.Schema.TableType | |||
import org.apache.calcite.schema.{Schema, SchemaPlus, Table} | |||
import org.apache.calcite.sql.{SqlCall, SqlNode} | |||
|
|||
import java.lang.{String => JString} |
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.
Scala String and Java String is totally same
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.
ok
…hould remapping the columnIndex of the inputRel otherwise may cause IllegalArgumentException or get incorrectly metadata This closes #15641
What is the purpose of the change
fix FlinkRelMdFilteredColumnInterval: remapping the columnIndex of the inputRel avoid IllegalArgumentException or
incorrect metadata
Brief change log
Verifying this change
Added ut
Does this pull request potentially affect one of the following parts:
-The S3 file system connector: ( no )
Documentation