-
Notifications
You must be signed in to change notification settings - Fork 13.7k
[FLINK-21985][table-api] Support Explain Query/Modifcation syntax in Calcite Parser #15402
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
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 b7efaba (Sat Aug 28 11:10:41 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 your conribution. I left some comments.
stmt = SqlQueryOrDml() { | ||
return new SqlRichExplain(getPos(),stmt); | ||
} | ||
} |
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.
Add a new line
public void testExplainAsJson() { | ||
// unsupport testExplainWithType now | ||
} |
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.
Add comment:
// TODO: FLINK-20562
if (i == 0) { | ||
statement = operand; | ||
} else { | ||
throw new AssertionError(i); |
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 use meaningful exception message.
case richExplain: SqlRichExplain => | ||
val validated = validator.validate(richExplain.getStatement) | ||
richExplain.setOperand(0,validated) | ||
richExplain |
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 forward before SqlExplain
? Do we really need SqlExplain?
public void testExplainAsXml() { | ||
// unsupport testExplainWithType now | ||
} |
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.
Add comment:
Don't supported feature. Escape the test.
public void testExplainJsonFormat() { | ||
// unsupport testExplainJsonFormat now | ||
} | ||
|
||
@Test | ||
public void testExplainWithImpl() { | ||
// unsupport testExplainWithImpl now | ||
} | ||
|
||
@Test | ||
public void testExplainWithoutImpl() { | ||
// unsupport testExplainWithoutImpl now | ||
} | ||
|
||
@Test | ||
public void testExplainWithType() { | ||
// unsupport testExplainWithType now | ||
} | ||
|
||
@Test | ||
public void testExplainAsXml() { | ||
// unsupport testExplainWithType now | ||
} | ||
|
||
@Test | ||
public void testExplainAsJson() { | ||
// unsupport testExplainWithType now |
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.
ditto
* Operation to describe an EXPLAIN statement. NOTES: currently, only default behavior(EXPLAIN xx) | ||
* is supported. |
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.
Explain plan for is also supported.
import org.apache.flink.sql.parser.ExtendedSqlNode | ||
import org.apache.flink.sql.parser.dql._ | ||
import org.apache.flink.table.api.{TableException, ValidationException} | ||
import org.apache.flink.table.catalog.CatalogReader | ||
import org.apache.flink.table.parse.CalciteParser |
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.
Rollback
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.
Look good to me in general.
.withIdentifierExpansion(true) | ||
// Disable implicit type coercion for now. | ||
.withTypeCoercionEnabled(false)) |
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 use 2 space in scala file.
schemaPath: util.List[String], | ||
viewPath: util.List[String]) | ||
: RelRoot = { | ||
: RelRoot = { |
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.
roll back
|
||
@Test | ||
public void testExplainJsonFormat() { | ||
// Don't supported feature. Escape the test. |
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.
Unsupported feature.
CREATE TABLE IF NOT EXISTS orders ( | ||
`user` BIGINT NOT NULl, | ||
product VARCHAR(32), | ||
amount INT, | ||
ts TIMESTAMP(3), | ||
ptime AS PROCTIME(), | ||
PRIMARY KEY(`user`) NOT ENFORCED, | ||
WATERMARK FOR ts AS ts - INTERVAL '1' SECONDS |
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 style the DDL as above. Add " " before fields
* Operation to describe an EXPLAIN statement. NOTES: currently, only default behavior(EXPLAIN [plan | ||
* for]* xx) is supported. |
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.
nit: Use UPPSER CASE "PLAN FOR"
case e: TableException => | ||
assertTrue(e.getMessage.contains("Only default behavior is supported now")) |
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.
Remove this. It seems no one will getinto this branch? In my local environment, the test passes.
case e: TableException => | ||
assertTrue(e.getMessage.contains("Only default behavior is supported now")) |
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.
Remove this. It seems no one will getinto this branch? In my local environment, the test passes.
import java.util.Collections; | ||
import java.util.List; | ||
|
||
/** EXPLAIN (plan for)* STATEMENT sql call. */ |
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.
Use upper case.
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.
LGTM. CC @wuchong
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.
LGTM.
… and Table API This closes apache#15402
…Calcite Parser
What is the purpose of the change
(For example: This pull request makes task deployment go through the blob server, rather than through RPC. That way we avoid re-transferring them on each deployment (during recovery).)
Brief change log
(for example:)
Verifying this change
(Please pick either of the following options)
This change is a trivial rework / code cleanup without any test coverage.
(or)
This change is already covered by existing tests, such as (please describe tests).
(or)
This change added tests and can be verified as follows:
(example:)
Does this pull request potentially affect one of the following parts:
@Public(Evolving)
: (yes / no)Documentation