Skip to content

Print arithmetic and comparison expressions in human-readable format.#12783

Merged
AkshayPall merged 1 commit intoprestodb:masterfrom
AkshayPall:pretty-print-expressions
May 10, 2019
Merged

Print arithmetic and comparison expressions in human-readable format.#12783
AkshayPall merged 1 commit intoprestodb:masterfrom
AkshayPall:pretty-print-expressions

Conversation

@AkshayPall
Copy link
Contributor

@AkshayPall AkshayPall commented May 8, 2019

Previously, all functions (CallExpressions) would be printed as $function_name$(ARG1, ARG2, ..., ARGN) but now arithmetic and comparison expressions will print with the operand between arguments eg, (#1) + (BIGINT 5). Additional changes were made to pass a FunctionManager instance to RowExpressionFormatter instead of instantiating one in the class.

Copy link

@highker highker left a comment

Choose a reason for hiding this comment

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

Looks great!! Minor comments only.

Copy link

Choose a reason for hiding this comment

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

It's ok to not have NotNull

Copy link

Choose a reason for hiding this comment

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

Let's use StandardFunctionResolution for the constructor; so it's more general

Copy link

Choose a reason for hiding this comment

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

else is redundant

Copy link

Choose a reason for hiding this comment

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

Same, remove NotNull

Copy link

Choose a reason for hiding this comment

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

Use FunctionMetadataManager to make it generic.

Copy link

Choose a reason for hiding this comment

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

No need for this comment.

Copy link

Choose a reason for hiding this comment

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

Can you add more complicated test cases to invoke recursive printing? For example ((a + b) * c) > 0 OR length(d) > e

Copy link

Choose a reason for hiding this comment

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

Just call it expression. We usually spell out a word completely than using abbreviations.

Copy link

Choose a reason for hiding this comment

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

Could you also handle the CAST case?
For CAST(a) we can print out as CAST(a AS <return_type>). This helps us to understand the type info.

Also, can we handle NEGATION and SUBSCRIPT as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure thing, should I also include AND and OR.

Copy link

Choose a reason for hiding this comment

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

sounds good!

Copy link

Choose a reason for hiding this comment

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

nit: toList -> toImmutableList

Copy link

@highker highker left a comment

Choose a reason for hiding this comment

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

For commit message, we usually bound each line (for body or title) within 72 words.

@AkshayPall AkshayPall requested a review from highker May 9, 2019 20:46
@highker
Copy link

highker commented May 9, 2019

Could you squash the commits into one and do a force push? @AkshayPall

@AkshayPall AkshayPall force-pushed the pretty-print-expressions branch from 3361068 to e9c199b Compare May 9, 2019 21:16
Copy link

@highker highker left a comment

Choose a reason for hiding this comment

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

Looks great! Minor nits only.

Copy link

Choose a reason for hiding this comment

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

use node.getType().getTypeSignature(); lower case is fine.

Copy link

Choose a reason for hiding this comment

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

Nit: put all variables in one line or one variable per line. For example:

private static void printSubPlan(
    SubPlan plan,
    Map<PlanFragmentId, PlanFragment> fragmentsById,
    ...

Copy link

Choose a reason for hiding this comment

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

We don't usually use numbers in variable names; but it's fine in tests.

Previously, all functions (CallExpressions) would be printed as
'$function_name$(ARG1, ARG2, ..., ARGN)' but now some expressions will
print in human format eg, '(prestodb#1) + (BIGINT 5)'.
Additional changes were made to pass a `FunctionManager` instance to
`RowExpressionFormatter` instead of instantiating one in the class.
@AkshayPall AkshayPall force-pushed the pretty-print-expressions branch from e9c199b to b1c7e5c Compare May 9, 2019 21:30
@AkshayPall AkshayPall requested a review from highker May 10, 2019 00:17
@AkshayPall AkshayPall merged commit d23dc3e into prestodb:master May 10, 2019
@AkshayPall AkshayPall deleted the pretty-print-expressions branch May 10, 2019 16:19
@AkshayPall AkshayPall mentioned this pull request May 13, 2019
16 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants