Skip to content

[FLINK-3192] Add explain support to print ast and sql physical execution. - #1477

Closed
gallenvara wants to merge 4 commits into
apache:masterfrom
gallenvara:SqlExplanation
Closed

[FLINK-3192] Add explain support to print ast and sql physical execution.#1477
gallenvara wants to merge 4 commits into
apache:masterfrom
gallenvara:SqlExplanation

Conversation

@gallenvara

Copy link
Copy Markdown
Contributor

Table API doesn't support sql-explanation now. Add the explain support to print ast (abstract syntax tree) and the physical execution of sql.

@gallenvara
gallenvara force-pushed the SqlExplanation branch 3 times, most recently from 3622263 to 1a6a993 Compare January 4, 2016 05:43
Comment thread flink-staging/flink-scala-shell/pom.xml Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need this dependency?

@fhueske

fhueske commented Jan 4, 2016

Copy link
Copy Markdown
Contributor

Hi @gallenvara, thanks for this PR!
Explain is a very nice feature for the Table API (and later on for the SQL interface).

IMO, it would be better to reuse the existing JSON plan and generate the HIVE visualization locally inside of the Table API. That would avoid to replicate quite a bit of functionality just for the sake of a different visualization. Would that work as well?

In general, it is a good idea to discuss changes that modify user-facing APIs and touch several components/modules before starting to code as pointed out in our contribution guidelines.

@gallenvara

Copy link
Copy Markdown
Contributor Author

@fhueske ,thanks a lot for the review work! I'll modify the code and update the PR according to your advice.

@fhueske

fhueske commented Jan 6, 2016

Copy link
Copy Markdown
Contributor

Thanks @gallenvara.
Flink uses Jackson to handle JSON data. You can use it to parse the JSON String.

Looking forward to your update :-)

@gallenvara
gallenvara force-pushed the SqlExplanation branch 3 times, most recently from 38be686 to 7a5f4b9 Compare January 7, 2016 02:34
@gallenvara

Copy link
Copy Markdown
Contributor Author

@fhueske , codes has been finished. I have drop previous method of plan-generator and rewrite a new parser named PlanJsonParser to parse the existing JSON plan. Could you help with review work? :)

Comment thread flink-staging/flink-table/pom.xml Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you use the ${jackson.version} variable here?

@fhueske

fhueske commented Jan 7, 2016

Copy link
Copy Markdown
Contributor

Hi @gallenvara, the PR looks much better. I added a few comment inline.

Thanks, Fabian

@gallenvara

Copy link
Copy Markdown
Contributor Author

@fhueske , @rmetzger , thanks for review work. I have modified related code as you adviced and submitted a new commit.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's better to use some meaningful variable name here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants