Skip to content
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

Add unparser for the C++ version #11

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

kaikalur
Copy link

@kaikalur kaikalur commented Feb 5, 2021

Bring C++ somewhat on-par with Java.

@kaikalur kaikalur changed the title Added the unparser for cpp Add unparser for the C++ version Feb 5, 2021
@kaikalur kaikalur force-pushed the cpp_unparse branch 2 times, most recently from aed76e1 to 7eff287 Compare February 8, 2021 23:28
@kaikalur kaikalur requested a review from rongrong February 17, 2021 22:25
Copy link

@rongrong rongrong left a comment

Choose a reason for hiding this comment

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

Is there any tests running the logic already?

AstNode* GetChild(int i) { return static_cast<AstNode*>(jjtGetChild(i)); }
JJString toString(const JJString& prefix) const {

int Kind() const { return id; }

Choose a reason for hiding this comment

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

Should function name start with lower case?

Copy link
Author

Choose a reason for hiding this comment

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

Yeah - it's one of the weird things that happened when we did C++ first and then java. Will fix it.

Copy link
Author

Choose a reason for hiding this comment

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

Actually, in C++ the style is upper case starting letter. So not changing it here.

@@ -111,7 +111,7 @@ void presto_aggregation_function():
{
"NUMERIC_HISTOGRAM"
| "HISTOGRAM"
| "APPROEX_PERCENTILE"
| "APPROX_PERCENTILE"

Choose a reason for hiding this comment

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

This fix should probably be a separate commit/PR?

Copy link
Author

Choose a reason for hiding this comment

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

Done

@kaikalur kaikalur force-pushed the cpp_unparse branch 3 times, most recently from 186bd59 to 71765a1 Compare March 15, 2021 14:40
@kaikalur kaikalur requested a review from rongrong March 15, 2021 15:56
@kaikalur kaikalur force-pushed the cpp_unparse branch 3 times, most recently from 1222619 to 5aad9e0 Compare March 22, 2021 15:57
@kaikalur
Copy link
Author

Build now! Also, looks like we lost Leiqing's changes? Somehow the pom.xml in main doesn't have the sonatype stuff. So looks like some other merges clobbered. You will see them here now.

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.

2 participants