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

PHOENIX-5181 support Math sin/cos/tan functions #458

Closed
wants to merge 1 commit into from

Conversation

yanxinyi
Copy link
Contributor

@yanxinyi yanxinyi commented Mar 7, 2019

No description provided.

Copy link
Contributor

@dbwong dbwong left a comment

Choose a reason for hiding this comment

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

How do we handle decimal type in general in Phoenix? In my previous databases since the percision is possibly much higher than a double special mathematical considerations had to be done or decimal disallowed?

@ChinmaySKulkarni
Copy link
Contributor

@yanxinyi overall looks good. I think the offline discussions we had regarding precision and rounding errors are the main points of concern. Can you add tests that reflect such cases?

@yanxinyi
Copy link
Contributor Author

yanxinyi commented Mar 9, 2019

Offline synced with @ChinmaySKulkarni about the precision issue, and Phoenix PDecimal class already takes care of it. Other similar Math functions, such as power, implemented the same way as I do, but
@twdsilva can we have your input here, thanks.

@karanmehta93
Copy link
Member

Can you add some IT's that test the grammar as well? @yanxinyi

@yanxinyi
Copy link
Contributor Author

@karanmehta93 added IT tests

@yanxinyi
Copy link
Contributor Author

yanxinyi commented Apr 8, 2019

Offline discussed with @karanmehta93, used @Parameters feature for better code quality in unit test functions as he suggested.

Copy link
Contributor

@gjacoby126 gjacoby126 left a comment

Choose a reason for hiding this comment

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

+1.

@gjacoby126
Copy link
Contributor

@karanmehta93 , were there any other changes you wanted to the trig function feature?

ddl =
"CREATE TABLE " + tableName + " (k VARCHAR NOT NULL PRIMARY KEY, doub DOUBLE)";
conn.createStatement().execute(ddl);
conn.commit();
Copy link
Member

Choose a reason for hiding this comment

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

Commit is not needed after ddl statements.


literal = LiteralExpression.newConstant(value, dataType, SortOrder.DESC);
boolean ret2 = testExpression(literal, expectedResult, testedFunction);
assertEquals(ret1, ret2);
Copy link
Member

Choose a reason for hiding this comment

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

Why is this required? If both ret1 and ret2 return false, this would still pass. Is that intended?

Copy link
Member

Choose a reason for hiding this comment

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

As discussed offline, @yanxinyi will fix this in another Jira.

Copy link
Member

@karanmehta93 karanmehta93 left a 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 and addressing the comments. Looks good to merge.

@yanxinyi yanxinyi closed this Apr 25, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants