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 ==, !=, >, <, and string equality computed columns #957

Merged
merged 3 commits into from
Mar 6, 2020

Conversation

sc1f
Copy link
Contributor

@sc1f sc1f commented Feb 28, 2020

This PR adds and tests several new computed column operators:

  • ==, !=, >, and < for comparing numeric columns
  • is for comparing string columns

For numeric operations, when either value is null the comparator will return null. However, for strings, if either value is null the comparator returns false. This is due to an issue with null handling inside computed columns in general, which has been fixed in the new computed column API.

@sc1f sc1f added enhancement Feature requests or improvements JS labels Feb 28, 2020
@sc1f sc1f requested a review from texodus February 28, 2020 21:13
@sc1f sc1f self-assigned this Feb 28, 2020
Copy link
Member

@texodus texodus 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 PR! Great test coverage!

Can you please take a look at the string comparison note when you get a chance?

cpp/perspective/src/cpp/computed_function.cpp Outdated Show resolved Hide resolved
Copy link
Member

@texodus texodus 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! Thanks for the PR!

@texodus texodus merged commit e23988b into master Mar 6, 2020
@texodus texodus deleted the computed-comparators branch March 6, 2020 08:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Feature requests or improvements JS
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants